From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E070D6EC40; Sat, 25 Sep 2021 14:32:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E070D6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632569554; bh=eAk/u/RQKSeEvvex63mrNnZcb6HRbxZPSxxQtOmGqvc=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=b8XIUOdn7HsKY6lwcM1oB5VvUWupiSL6DUMy61LHnEePYEvw5NBPCEh67vRXTnpyI G93jhPA8Qx4aC99fMBKLvHt+jlkfEDmmYXjpGo6ffP+nw7Z68yDRQhZFGHk0rSv3Nj 4Fh1sgotWhY9DBzMsvfpq8V38b3On20ETqrSPaeA= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7FC346EC40 for ; Sat, 25 Sep 2021 14:32:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7FC346EC40 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mU5ux-0004zG-Kr; Sat, 25 Sep 2021 14:32:32 +0300 Date: Sat, 25 Sep 2021 14:32:30 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210925113230.GF290467@tarantool.org> References: <48ee23c47c696c5e002347841b652677ca877507.1632220375.git.imeevma@gmail.com> <62bdd807-6f60-4307-1ed8-0badb6c014ae@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <62bdd807-6f60-4307-1ed8-0badb6c014ae@tarantool.org> X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD96A58C36AA2E99649E150573C3DCDB299A56307BC7D0E2EC5182A05F53808504054C83B65DFBCC8F426FD9F547F712CA3C4E0F4B6D0B414C20BF2F19163E8E4B1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D4A169723F56FEDEEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378F586D843116CFB2EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F22C48B1054E86DDC30D4D84912438EE23CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637CAEE156C82D3D7D9389733CBF5DBD5E9B5C8C57E37DE458B9E9CE733340B9D5F3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7354E672349037D5FA5C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5CDD161950988187251444AA476777FD193765D8603B576DFD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75BFC02AB3DF06BA5A410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D345C110A855FC09999C0F77A02F10AFEA7DD1A08A6CD5DAA1D0429496B6305A690707FE85B9CC5381C1D7E09C32AA3244C2C324034E4709189B68E8E113ED20C9A3A76366E8A9DE7CA729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojqHUEAOTxd4gFKAUUV6FDng== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D783E540A55875E91A50E09E30A1E8ACB83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 09/15] sql: rework AVG() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answers, diff and new patch below. On Thu, Sep 23, 2021 at 12:48:58AM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index 12a6a5a2c..b5f154fb1 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > @@ -102,6 +102,54 @@ fin_total(struct sql_context *ctx) > > mem_copy_as_ephemeral(ctx->pOut, ctx->pMem); > > } > > > > +/** Implementation of the AVG() function. */ > > +static void > > +step_avg(struct sql_context *ctx, int argc, struct Mem **argv) > > +{ > > + assert(argc == 1); > > + (void)argc; > > + assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem)); > > + if (argv[0]->type == MEM_TYPE_NULL) > > + return; > > + struct Mem *mem; > > + uint32_t *count; > > + if (ctx->pMem->type == MEM_TYPE_NULL) { > > + uint32_t size = sizeof(struct Mem) + sizeof(uint32_t); > > + mem = sqlDbMallocRawNN(sql_get(), size); > > 1. Where is it deleted? Can't find. > It will be set as allocated to MEM and will be released during mem_destroy(). However, I agree that it is obvious that this new MEM shouldn't be cleared first. I added mem_destroy() for this mem, though it will work as mem_clear() due to numbers being scalars. > > + if (mem == NULL) { > > + ctx->is_aborted = true; > > + return; > > + } > > + count = (uint32_t *)(mem + 1); > > + mem_create(mem); > > + *count = 1; > > + mem_copy_as_ephemeral(mem, argv[0]); > > + mem_set_bin_allocated(ctx->pMem, (char *)mem, size); > > + return; > > + } > > diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua > > index 507d06549..08a63b86d 100755 > > --- a/test/sql-tap/built-in-functions.test.lua > > +++ b/test/sql-tap/built-in-functions.test.lua > > @@ -605,4 +605,33 @@ test:do_execsql_test( > > } > > ) > > > > +-- Make sure AVG() accepts and returns DOUBLE by default. > > +test:do_test( > > + "builtins-4.1.1", > > + function() > > + return box.execute([[SELECT AVG(?);]], {1}).metadata > > + end, { > > + {name = "COLUMN_1", type = "double"}, > > + }) > > + > > +test:do_test( > > + "builtins-4.1.2", > > + function() > > + local res = {pcall(box.execute, [[SELECT AVG(?);]], {-1ULL})} > > + return {tostring(res[3])} > > + end, { > > + "Type mismatch: can not convert integer(18446744073709551615) to double" > > + }) > > + > > +-- Make sure AVG() works with DECIMAL properly. > > +test:do_execsql_test( > > + "builtins-4.1.3", > > + [[ > > + SELECT AVG(cast(column_2 as DECIMAL)) from (values(1), (123.432)); > > 2. I don't understand how does it work. Why not column_1? Why does it fail? > > tarantool> box.execute('SELECT column_1 from (values(1), (123.432));') > --- > - null > - Can’t resolve field 'COLUMN_1' > ... > > Isn't there 2 tuples [1], and [123.432] with just 1 column each? > And why does it work when I delete one of the values? > > tarantool> box.execute('SELECT column_1 from (values(1));') > --- > - metadata: > - name: COLUMN_1 > type: integer > rows: > - [1] > ... > I believe this is a bug that was introduced in patch 7bfcf57e44028ea425d0d274b9ca402c196716f9 . Before patch: tarantool> box.execute([[values(1), (2), (3);]]) --- - metadata: - name: column1 type: integer rows: - [1] - [2] - [3] ... tarantool> box.execute([[values(1), (2), (3), (4);]]) --- - metadata: - name: column1 type: integer rows: - [1] - [2] - [3] - [4] ... After patch: tarantool> box.execute([[values(1), (2), (3);]]) --- - metadata: - name: COLUMN_3 type: integer rows: - [1] - [2] - [3] ... tarantool> box.execute([[values(1), (2), (3), (4);]]) --- - metadata: - name: COLUMN_4 type: integer rows: - [1] - [2] - [3] - [4] ... Not sure if there is an issue for this. > > + ]], > > + { > > + dec.new(62.216) > > + } > > +) > > + > > test:finish_test() > > Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 77a8d637a..e436ffbe1 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -108,12 +108,12 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv) { assert(argc == 1); (void)argc; - assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem)); - if (argv[0]->type == MEM_TYPE_NULL) + assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem)); + if (mem_is_null(argv[0])) return; struct Mem *mem; uint32_t *count; - if (ctx->pMem->type == MEM_TYPE_NULL) { + if (mem_is_null(ctx->pMem)) { uint32_t size = sizeof(struct Mem) + sizeof(uint32_t); mem = sqlDbMallocRawNN(sql_get(), size); if (mem == NULL) { @@ -138,15 +138,19 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv) static void fin_avg(struct sql_context *ctx) { - assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem)); - if (ctx->pMem->type == MEM_TYPE_NULL) + assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem)); + if (mem_is_null(ctx->pMem)) return mem_set_null(ctx->pOut); - struct Mem *mem = (struct Mem *)ctx->pMem->z; - uint32_t *count = (uint32_t *)(mem + 1); - struct Mem mem_count; - mem_create(&mem_count); - mem_set_uint(&mem_count, *count); - if (mem_div(mem, &mem_count, ctx->pOut) != 0) + struct Mem *tmp = (struct Mem *)ctx->pMem->z; + uint32_t *count_val = (uint32_t *)(tmp + 1); + struct Mem sum; + mem_create(&sum); + mem_copy_as_ephemeral(&sum, tmp); + mem_destroy(tmp); + struct Mem count; + mem_create(&count); + mem_set_uint(&count, *count_val); + if (mem_div(&sum, &count, ctx->pOut) != 0) ctx->is_aborted = true; } New patch: commit 2b5ae06ceadff5e121e4508ffc2b7913f6d84e79 Author: Mergen Imeev Date: Thu Sep 9 18:19:53 2021 +0300 sql: rework AVG() This patch makes AVG() accept DOUBLE values by default. Also, after this patch AVG() will be able to work with DECIMAL values. Part of #4145 Part of #6355 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index d0606744c..e436ffbe1 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -102,6 +102,58 @@ fin_total(struct sql_context *ctx) mem_copy_as_ephemeral(ctx->pOut, ctx->pMem); } +/** Implementation of the AVG() function. */ +static void +step_avg(struct sql_context *ctx, int argc, struct Mem **argv) +{ + assert(argc == 1); + (void)argc; + assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem)); + if (mem_is_null(argv[0])) + return; + struct Mem *mem; + uint32_t *count; + if (mem_is_null(ctx->pMem)) { + uint32_t size = sizeof(struct Mem) + sizeof(uint32_t); + mem = sqlDbMallocRawNN(sql_get(), size); + if (mem == NULL) { + ctx->is_aborted = true; + return; + } + count = (uint32_t *)(mem + 1); + mem_create(mem); + *count = 1; + mem_copy_as_ephemeral(mem, argv[0]); + mem_set_bin_allocated(ctx->pMem, (char *)mem, size); + return; + } + mem = (struct Mem *)ctx->pMem->z; + count = (uint32_t *)(mem + 1); + ++*count; + if (mem_add(mem, argv[0], mem) != 0) + ctx->is_aborted = true; +} + +/** Finalizer for the AVG() function. */ +static void +fin_avg(struct sql_context *ctx) +{ + assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem)); + if (mem_is_null(ctx->pMem)) + return mem_set_null(ctx->pOut); + struct Mem *tmp = (struct Mem *)ctx->pMem->z; + uint32_t *count_val = (uint32_t *)(tmp + 1); + struct Mem sum; + mem_create(&sum); + mem_copy_as_ephemeral(&sum, tmp); + mem_destroy(tmp); + struct Mem count; + mem_create(&count); + mem_set_uint(&count, *count_val); + if (mem_div(&sum, &count, ctx->pOut) != 0) + ctx->is_aborted = true; +} + static const unsigned char * mem_as_ustr(struct Mem *mem) { @@ -1656,69 +1708,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv) } } -/* - * An instance of the following structure holds the context of a - * sum() or avg() aggregate computation. - */ -typedef struct SumCtx SumCtx; -struct SumCtx { - struct Mem mem; - uint32_t count; -}; - -/* - * Routines used to compute the sum, average, and total. - * - * The SUM() function follows the (broken) SQL standard which means - * that it returns NULL if it sums over no inputs. TOTAL returns - * 0.0 in that case. In addition, TOTAL always returns a float where - * SUM might return an integer if it never encounters a floating point - * value. TOTAL never fails, but SUM might through an exception if - * it overflows an integer. - */ -static void -sum_step(struct sql_context *context, int argc, sql_value **argv) -{ - assert(argc == 1); - UNUSED_PARAMETER(argc); - struct SumCtx *p = sql_aggregate_context(context, sizeof(*p)); - if (p == NULL) { - context->is_aborted = true; - return; - } - if (p->count == 0) { - mem_create(&p->mem); - assert(context->func->def->returns == FIELD_TYPE_INTEGER || - context->func->def->returns == FIELD_TYPE_DOUBLE); - if (context->func->def->returns == FIELD_TYPE_INTEGER) - mem_set_uint(&p->mem, 0); - else - mem_set_double(&p->mem, 0.0); - } - if (argv[0]->type == MEM_TYPE_NULL) - return; - ++p->count; - assert(mem_is_num(argv[0])); - if (mem_add(&p->mem, argv[0], &p->mem) != 0) - context->is_aborted = true; -} - -static void -avgFinalize(sql_context * context) -{ - SumCtx *p; - p = sql_aggregate_context(context, 0); - if (p == NULL || p->count == 0) { - mem_set_null(context->pOut); - return; - } - struct Mem mem; - mem_create(&mem); - mem_set_uint(&mem, p->count); - if (mem_div(&p->mem, &mem, context->pOut) != 0) - context->is_aborted = true; -} - /* * The following structure keeps track of state information for the * count() aggregate function. @@ -2015,8 +2004,9 @@ struct sql_func_definition { static struct sql_func_definition definitions[] = { {"ABS", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, absFunc, NULL}, {"ABS", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, absFunc, NULL}, - {"AVG", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, sum_step, avgFinalize}, - {"AVG", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, sum_step, avgFinalize}, + {"AVG", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_avg, fin_avg}, + {"AVG", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_avg, fin_avg}, + {"AVG", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DECIMAL, step_avg, fin_avg}, {"CHAR", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_STRING, charFunc, NULL}, {"CHAR_LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, lengthFunc, NULL}, diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua index 507d06549..08a63b86d 100755 --- a/test/sql-tap/built-in-functions.test.lua +++ b/test/sql-tap/built-in-functions.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(58) +test:plan(61) local dec = require('decimal') @@ -477,7 +477,7 @@ test:do_test( local res = {pcall(box.execute, [[SELECT AVG(?);]], {'1'})} return {tostring(res[3])} end, { - "Type mismatch: can not convert string('1') to integer" + "Type mismatch: can not convert string('1') to double" }) test:do_catchsql_test( @@ -605,4 +605,33 @@ test:do_execsql_test( } ) +-- Make sure AVG() accepts and returns DOUBLE by default. +test:do_test( + "builtins-4.1.1", + function() + return box.execute([[SELECT AVG(?);]], {1}).metadata + end, { + {name = "COLUMN_1", type = "double"}, + }) + +test:do_test( + "builtins-4.1.2", + function() + local res = {pcall(box.execute, [[SELECT AVG(?);]], {-1ULL})} + return {tostring(res[3])} + end, { + "Type mismatch: can not convert integer(18446744073709551615) to double" + }) + +-- Make sure AVG() works with DECIMAL properly. +test:do_execsql_test( + "builtins-4.1.3", + [[ + SELECT AVG(cast(column_2 as DECIMAL)) from (values(1), (123.432)); + ]], + { + dec.new(62.216) + } +) + test:finish_test()