Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 5/9] sql: improve type determination for column meta
Date: Thu, 18 Apr 2019 20:54:48 +0300	[thread overview]
Message-ID: <4D657191-4BC6-43F0-97A5-64B784DB0DE8@tarantool.org> (raw)
In-Reply-To: <9edb1536-e63c-d9ab-5130-1f38b822d698@tarantool.org>

[-- Attachment #1: Type: text/plain, Size: 2526 bytes --]



> On 16 Apr 2019, at 17:12, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> On 14/04/2019 18:04, Nikita Pettik wrote:
>> In most cases we don't assign and store type of node of expression AST
>> (except for constant literals).  To determine type of node  we use
>> sql_expr_type() function, which implements logic of quite simple
>> recursive tree traversal. Before this patch we set type of node after
>> code generation in sqlExprCodeTarget() without any traversal. This
>> approach is way worse even then sql_expr_type().
> 
> Why? How is recursive scan of the tree better than simple access to
> a struct's attribute? I see, that your patch improves type determination
> precision, but I do not understand how, and what a perf price for that?

Firstly, it seems that sqlExprCodeTarget() can be called on copy of
original expr (when query contains order/group by clauses): I see
that types assigned in that function are different from types in expr
passed to generateColumnNames().

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

Eventually, sqlExprCodeTarget() doesn’t 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 ‘smallint' type, not common ‘number’.
Without tree traversal we can’t 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>

In fact, we should once visit each node (probably after names resolution)
and save its type depending on child’s 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’ve added it to avoid
abusing changing result files like sql/types.result etc. Can drop it tho.

>> 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.


[-- Attachment #2: Type: text/html, Size: 3904 bytes --]

  reply	other threads:[~2019-04-18 17:54 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-14 15:03 [tarantool-patches] [PATCH 0/9] Introduce type BOOLEAN in SQL Nikita Pettik
2019-04-14 15:03 ` [tarantool-patches] [PATCH 1/9] sql: refactor mem_apply_numeric_type() Nikita Pettik
2019-04-14 15:04 ` [tarantool-patches] [PATCH 2/9] sql: disallow text values participate in sum() aggregate Nikita Pettik
2019-04-16 14:12   ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-18 17:54     ` n.pettik
2019-04-22 18:02       ` Vladislav Shpilevoy
2019-04-23 19:58         ` n.pettik
2019-04-14 15:04 ` [tarantool-patches] [PATCH 3/9] sql: use msgpack types instead of custom ones Nikita Pettik
2019-04-16 14:12   ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-18 17:54     ` n.pettik
2019-04-22 18:02       ` Vladislav Shpilevoy
2019-04-23 19:58         ` n.pettik
2019-04-14 15:04 ` [tarantool-patches] [PATCH 4/9] sql: introduce type boolean Nikita Pettik
2019-04-16 14:12   ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-18 17:54     ` n.pettik
2019-04-22 18:02       ` Vladislav Shpilevoy
2019-04-23 19:58         ` n.pettik
2019-04-23 21:06           ` Vladislav Shpilevoy
2019-04-14 15:04 ` [tarantool-patches] [PATCH 5/9] sql: improve type determination for column meta Nikita Pettik
2019-04-16 14:12   ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-18 17:54     ` n.pettik [this message]
2019-04-22 18:02       ` Vladislav Shpilevoy
2019-04-23 19:58         ` n.pettik
2019-04-14 15:04 ` [tarantool-patches] [PATCH 6/9] sql: make comparison predicate return boolean Nikita Pettik
2019-04-16 14:12   ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-18 17:54     ` n.pettik
2019-04-14 15:04 ` [tarantool-patches] [PATCH 7/9] sql: make predicates accept and " Nikita Pettik
2019-04-16 14:12   ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-18 17:55     ` n.pettik
2019-04-14 15:04 ` [tarantool-patches] [PATCH 9/9] sql: make <search condition> accept only boolean Nikita Pettik
2019-04-16 14:12   ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-18 17:55     ` n.pettik
2019-04-22 18:02       ` Vladislav Shpilevoy
2019-04-23 19:59         ` n.pettik
2019-04-23 21:06           ` Vladislav Shpilevoy
2019-04-23 22:01             ` n.pettik
     [not found] ` <b2a84f129c2343d3da3311469cbb7b20488a21c2.1555252410.git.korablev@tarantool.org>
2019-04-16 14:12   ` [tarantool-patches] Re: [PATCH 8/9] sql: make LIKE predicate return boolean result Vladislav Shpilevoy
2019-04-18 17:55     ` n.pettik
2019-04-22 18:02       ` Vladislav Shpilevoy
2019-04-23 19:58         ` n.pettik
2019-04-24 10:28 ` [tarantool-patches] Re: [PATCH 0/9] Introduce type BOOLEAN in SQL Vladislav Shpilevoy
2019-04-25  8:46 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D657191-4BC6-43F0-97A5-64B784DB0DE8@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH 5/9] sql: improve type determination for column meta' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox