From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Nikita Pettik <korablev@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias Date: Fri, 27 Dec 2019 14:53:22 +0300 [thread overview] Message-ID: <0aac937f-d9d1-d7d1-47f0-434994e5d32b@tarantool.org> (raw) In-Reply-To: <20191226112411.GE18639@tarantool.org> Thanks for the discussion! >>>>> diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result >>>>> index 7c2982682..8f0bced36 100644 >>>>> --- a/test/sql/full_metadata.result >>>>> +++ b/test/sql/full_metadata.result >>>>> @@ -118,6 +121,35 @@ execute("SELECT * FROM t;") >>>>> | - [1, 1, 'aSd'] >>>>> | ... >>>>> >>>>> +-- Alias is always set in extended metadata. If column label in >>>>> +-- form of AS clause is set, then this alias is presented in >>>>> +-- metadata. Otherwise, alias is just the same as name. >>>> >>>> 4. But it is not true. The alias is not set always: >>>> >>>> ========================================================= >>>> >>>> tarantool> box.execute('PRAGMA full_metadata = true;') >>>> --- >>>> - row_count: 0 >>>> ... >>>> >>>> tarantool> box.execute('SELECT 1') >>>> --- >>>> - metadata: >>>> - name: '1' >>>> type: integer >>>> rows: >>>> - [1] >>>> ... >>>> >>>> ========================================================= >>>> >>>> As you can see, no AS = no alias. >>> >>> Always displaying alias (or label - call it whatever you want) >>> even in full_metadata mode doesn't seem to be reasonable for me. >>> Users can easily handle it in their code. Sending one map field >>> with string value duplicating the name and decoding it may affect >>> performance much more significant than one NULL check (IMHO). >> >> As I said already, I don't propose to send a duplicate name. I >> propose to handle it on the client side. In Lua you can push one >> string 2 times, and it will cost nothing - they will reference >> one string object. >> >> In IProto you still can send one name, when possible, and on the >> netbox side push both names. > > From performance point of view this is ok. But still some > users may wonder why alias/label is displayed while there's > no explicit AS clause (even in full_metadata mode). When it is called 'label', I don't see a problem in showing it always. It does not say, that it is something after AS. Now we show 'name', even for expressions, and IMO this is more confusing. Because expressions have no name, strictly speaking. > Moreover, now client only decodes received metadata, but in your > suggestion it is assumed that full_metadata mode is the same on > client and server. Otherwise, it would look inconsistent: I don't propose to introduce client-side full_metadata, it makes no sense. But I do understand the problem. Then we could send the not specified 'label' as MP_NIL. That will occupy a couple of bytes per column, and will mean, that it is the same as the 'name'. Or we could send a flag in IPROTO_FLAGS, that the response contains full metadata even if it does not look so. It depends on whether we need to know fullness of the metadata on the client for anything else or not. > server (full_metadata = true), client (full_metadata = off): > server sends name, is_autoincrement, but client ignores extended > metadata and displays only name; > > server (full_metadata = false), clien (full_metadata = true): > server sends name (but in full_metadata mode there's also is_autoincrement > for instance), but client displays only half of extended metadata (alias, > but not is_autoincrement or other fields). > > Surely, we can send server's metadata status, but is it worth it? I don't know. This is purely subjective thing - should we show both name and label in full_metadata always or not. I think we should, and is worth doing. You think we shouldn't. I provide arguments, that it will simplify client's code. You provide that without this we don't need to send MP_NIL as a not specified name / shouldn't push 2 names, when there is only one, and it is maybe faster. Also my argument is that client-side code is not so perf sensitive as server's. So even if it will show sometimes 2 names instead of 1, it won't really affect anything. You can try asking in the red chat, what behaviour they expect. >>>> This is what I don't like >>>> in this patch the most. A user does not have a reliable always >>>> present column with a displayable name: either the column >>>> span or its alias after AS. He can't use name, because it >>>> changes its behavour depending on AS. And he can't use alias, >>>> because sometimes it is not present. >>> >>>> That leads to confusion, >>>> and necessity to branch in user code a lot. >>> >>> Yep, if user wants (for some reason; I don't have statistics how >>> often alias value can be useful or/and is used in production code) >>> to get alias value, one should make a small effort and add one >>> 'if' condition: >>> >>> if (is_full_metadata && alias == NULL) >>> alias = name; >>> >>> That's it. >>> >> >> Why, if we can remove that effort easily? I still don't see any >> real arguments why can't we always push a label. The argument >> about sending duplicate names is really weak - you can avoid >> sending two names, and push them on the client side, as I said >> above. No extra payload, not extra MessagePack decoding. >> >> Even the standard driver, to which you appeal so much, always >> returns a label. >>> Alternatively (I don't take into consideration option of swapping >>> name and alias as you suggest) we can change current default behaviour >>> and in 'casual metadata mode' return name as name (not alias): >>> >>> SELECT 1 AS x; -- would return '1' as name (now it returns 'x' as name). >>> >>> But I guess some users may turn out to be confused (in case they >>> haven't red about full_metadata pragma in docs). >>> >>>> The same example shows, that alias is a bad name. Here you need >>>> to return it, but it is not alias like AS. 'Label' would fit >>>> better here. >>> >>> I don't care much how to call it. If you prefer 'label', I will rename it. >> >> Yes, I prefer label. Not because this is my personal taste. I provided >> reasons, why it can't be called 'alias'. If you think alias is better, >> they say why. That is a discussion. > > There are no strict definitions under these terms, so it is just > matter of taste. Let's call it label, I am not against (I guess > you assume that label = (name ∪ alias), meanwhile alias is supposed > only to be idenfitier after AS clause). Yes, this is why I vote for label.
next prev parent reply other threads:[~2019-12-27 11:53 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-11 13:44 [Tarantool-patches] [PATCH v2 0/6] sql: extended metadata Nikita Pettik [not found] ` <cover.1576071711.git.korablev@tarantool.org> 2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 1/6] sql: refactor resulting set metadata Nikita Pettik 2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 2/6] sql: fix possible null dereference in sql_expr_coll() Nikita Pettik 2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation Nikita Pettik 2019-12-18 0:29 ` Vladislav Shpilevoy 2019-12-24 0:26 ` Nikita Pettik 2019-12-24 15:30 ` Vladislav Shpilevoy 2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability Nikita Pettik 2019-12-18 0:29 ` Vladislav Shpilevoy 2019-12-24 0:26 ` Nikita Pettik 2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement Nikita Pettik 2019-12-18 0:29 ` Vladislav Shpilevoy 2019-12-24 0:26 ` Nikita Pettik 2019-12-24 15:30 ` Vladislav Shpilevoy 2019-12-25 12:17 ` Nikita Pettik 2019-12-25 15:40 ` Vladislav Shpilevoy 2019-12-25 23:18 ` Nikita Pettik 2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias Nikita Pettik 2019-12-18 0:29 ` Vladislav Shpilevoy 2019-12-24 0:26 ` Nikita Pettik 2019-12-24 15:34 ` Vladislav Shpilevoy 2019-12-26 11:24 ` Nikita Pettik 2019-12-27 11:53 ` Vladislav Shpilevoy [this message] 2019-12-27 23:44 ` Nikita Pettik 2019-12-28 9:29 ` Vladislav Shpilevoy
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=0aac937f-d9d1-d7d1-47f0-434994e5d32b@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias' \ /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