From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2002929F82 for ; Tue, 23 Apr 2019 15:58:36 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Y9jnbm8ooIxZ for ; Tue, 23 Apr 2019 15:58:36 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9ACA329284 for ; Tue, 23 Apr 2019 15:58:35 -0400 (EDT) From: "n.pettik" Message-Id: <528116B3-186F-4E76-B772-93CABDB1E409@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_4E7D31E8-8C3F-408F-A033-B2C8F78DB4C2" Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 5/9] sql: improve type determination for column meta Date: Tue, 23 Apr 2019 22:58:33 +0300 In-Reply-To: <72727290-ccad-85b7-69da-32b59be955a6@tarantool.org> References: <9edb1536-e63c-d9ab-5130-1f38b822d698@tarantool.org> <4D657191-4BC6-43F0-97A5-64B784DB0DE8@tarantool.org> <72727290-ccad-85b7-69da-32b59be955a6@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy --Apple-Mail=_4E7D31E8-8C3F-408F-A033-B2C8F78DB4C2 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 >> Secondly, suggested approach simply makes code cleaner and easier >> to maintain: we shouldn=E2=80=99t care about adding on each branch of = switch >> appropriate type (since it is already done in sql_expr_type()). >>=20 >> Eventually, sqlExprCodeTarget() doesn=E2=80=99t take into = consideration >> types of child nodes. This drawback will appear when we extend >> integer type with small/big ints: >>=20 >> Column a is of type smallint. >>=20 >> SELECT 1 + a; >>=20 >> Should result in =E2=80=98smallint' type, not common =E2=80=98number=E2= =80=99. >> Without tree traversal we can=E2=80=99t do that. >>=20 >> Price is a primitive traversal through the expr tree. This traversal = (but in more >> sophisticated implementation and being called only once saving in = each node >> its type) is anyway required to implement this task: >> https://github.com/tarantool/tarantool/issues/3103 = = > >=20 > I do not have anything against traversal. I just don't like multiple > traversal and seemingly free and fast call of deceptive = sql_expr_type() - > it is expensive in fact, just look at this huge 'switch-case'. >=20 > Your patch is ok, it does not aggravate anything. But sql_expr_type() > is called in very many places. Probably, in scope of 3103 we should > set Expr type always before optimizer and Vdbe-coder are started. Just > thoughts. >=20 >> In fact, we should once visit each node (probably after names = resolution) >> and save its type depending on child=E2=80=99s types. >>=20 >>> If you are not sure about the price, we could ask Alexander to run >>> benches on your branch before pushing into the master. >>=20 >> This patch is auxiliary to main-patchset. I=E2=80=99ve added it to = avoid >> abusing changing result files like sql/types.result etc. Can drop it = tho. >=20 > Now I see, that there is nothing to bench. Almost pure refactoring. >=20 >>>> So, to improve accuracy >>>> of type determination, let's always call that method and remove = type >>>> assignment in sqlExprCodeTarget(). >>>=20 >>> This patch fixes the issue >>> https://github.com/tarantool/tarantool/issues/4126 = . >>=20 >> Ok, added label. >>=20 >=20 > During the second review I found some orphan type assignments > in codetarget(). I dropped them and the tests passed. The diff is > on the branch. Thx, applied as obvious. --Apple-Mail=_4E7D31E8-8C3F-408F-A033-B2C8F78DB4C2 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

Secondly, suggested approach simply makes code cleaner and = easier
to maintain: we shouldn=E2=80=99t care about adding = on each branch of switch
appropriate type (since it is = already done in sql_expr_type()).

Eventually,= sqlExprCodeTarget() doesn=E2=80=99t take into consideration
types of child nodes. This drawback will appear when we = extend
integer type with small/big ints:

Column a is of type smallint.

SELECT 1 + a;

Should result in = =E2=80=98smallint' type, not common =E2=80=98number=E2=80=99.
Without tree traversal we can=E2=80=99t do that.

Price is a primitive traversal through the = expr tree. This traversal (but in more
sophisticated = implementation and being called only once saving in each node
its type) is anyway required to implement this task:
https://github.com/tarantool/tarantool/issues/3103 <https://github.com/tarantool/tarantool/issues/3103>

I do not have anything against traversal. I just don't like = multiple
traversal and = seemingly free and fast call of deceptive sql_expr_type() -
it is expensive in fact, just = look at this huge 'switch-case'.

Your patch is ok, it does not aggravate anything. But = sql_expr_type()
is called in very many places. Probably, in scope of 3103 we = should
set Expr type = always before optimizer and Vdbe-coder are started. Just
thoughts.

In = fact, we should once visit each node (probably after names = resolution)
and save its type depending on child=E2=80=99s = types.

If you are not sure about the price, we could ask Alexander = to run
benches on your branch before pushing into the = master.

This patch is = auxiliary to main-patchset. I=E2=80=99ve added it to avoid
abusing changing result files like sql/types.result etc. Can = drop it tho.

Now I see, that there is nothing to bench. Almost pure = refactoring.

So, to improve accuracy
of type determination, = let's always call that method and remove type
assignment = in sqlExprCodeTarget().

This = patch fixes the issue
https://github.com/tarantool/tarantool/issues/4126.

Ok, added label.


During the second review I found some orphan type = assignments
in = codetarget(). I dropped them and the tests passed. The diff is
on the branch.

Thx, applied as = obvious.

= --Apple-Mail=_4E7D31E8-8C3F-408F-A033-B2C8F78DB4C2--