[tarantool-patches] Re: [PATCH 2/9] sql: disallow text values participate in sum() aggregate
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Apr 22 21:02:22 MSK 2019
Hi! Thanks for the fixes!
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index b86a95d9a..9adfeec67 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1495,24 +1495,34 @@ static void
> sumStep(sql_context * context, int argc, sql_value ** argv)
> {
> SumCtx *p;
> - int type;
> assert(argc == 1);
> UNUSED_PARAMETER(argc);
> p = sql_aggregate_context(context, sizeof(*p));
> - type = sql_value_numeric_type(argv[0]);
> - if (p && type != SQL_NULL) {
> - p->cnt++;
> - if (type == SQL_INTEGER) {
> - i64 v = sql_value_int64(argv[0]);
> - p->rSum += v;
> - if ((p->approx | p->overflow) == 0
> - && sqlAddInt64(&p->iSum, v)) {
> - p->overflow = 1;
> - }
> - } else {
> - p->rSum += sql_value_double(argv[0]);
> - p->approx = 1;
> + assert(p != NULL);
Why are you sure, that p != NULL? sql_aggregate_context()
on first invocation allocates memory, and it can fail.
> + int type = sql_value_type(argv[0]);
> + if (type == SQL_NULL)
> + return;
I've fixed the comment above and also I see, that sumStep is
rewritten almost completely, so it is time to convert it to
Tarantool style. See the diff below and on the branch in a
separate commit.
(Note, 'sql_value' below is not prepended with 'struct' because it
is a typedef - compiler curses).
==================================================================
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9adfeec67..0f9e228dc 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1492,15 +1492,13 @@ struct SumCtx {
* it overflows an integer.
*/
static void
-sumStep(sql_context * context, int argc, sql_value ** argv)
+sum_step(struct sql_context *context, int argc, sql_value **argv)
{
- SumCtx *p;
assert(argc == 1);
UNUSED_PARAMETER(argc);
- p = sql_aggregate_context(context, sizeof(*p));
- assert(p != NULL);
+ struct SumCtx *p = sql_aggregate_context(context, sizeof(*p));
int type = sql_value_type(argv[0]);
- if (type == SQL_NULL)
+ if (type == SQL_NULL || p == NULL)
return;
if (type != SQL_FLOAT && type != SQL_INTEGER) {
if (mem_apply_numeric_type(argv[0]) != 0) {
@@ -1514,10 +1512,10 @@ sumStep(sql_context * context, int argc, sql_value ** argv)
}
p->cnt++;
if (type == SQL_INTEGER) {
- i64 v = sql_value_int64(argv[0]);
+ int64_t v = sql_value_int64(argv[0]);
p->rSum += v;
if ((p->approx | p->overflow) == 0 &&
- sqlAddInt64(&p->iSum, v)) {
+ sqlAddInt64(&p->iSum, v) != 0) {
p->overflow = 1;
}
} else {
@@ -1870,9 +1868,12 @@ sqlRegisterBuiltinFunctions(void)
FUNCTION(zeroblob, 1, 0, 0, zeroblobFunc, FIELD_TYPE_SCALAR),
FUNCTION_COLL(substr, 2, 0, 0, substrFunc),
FUNCTION_COLL(substr, 3, 0, 0, substrFunc),
- AGGREGATE(sum, 1, 0, 0, sumStep, sumFinalize, FIELD_TYPE_NUMBER),
- AGGREGATE(total, 1, 0, 0, sumStep, totalFinalize, FIELD_TYPE_NUMBER),
- AGGREGATE(avg, 1, 0, 0, sumStep, avgFinalize, FIELD_TYPE_NUMBER),
+ AGGREGATE(sum, 1, 0, 0, sum_step, sumFinalize,
+ FIELD_TYPE_NUMBER),
+ AGGREGATE(total, 1, 0, 0, sum_step, totalFinalize,
+ FIELD_TYPE_NUMBER),
+ AGGREGATE(avg, 1, 0, 0, sum_step, avgFinalize,
+ FIELD_TYPE_NUMBER),
AGGREGATE2(count, 0, 0, 0, countStep, countFinalize,
SQL_FUNC_COUNT, FIELD_TYPE_INTEGER),
AGGREGATE(count, 1, 0, 0, countStep, countFinalize,
More information about the Tarantool-patches
mailing list