impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lars Volker ...@cloudera.com>
Subject Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Mon, 08 May 2017 23:02:10 GMT
On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker <marcel@cloudera.com>
wrote:

>
>
> On Mon, May 8, 2017 at 9:50 AM, Dimitris Tsirogiannis <
> dtsirogiannis@cloudera.com> wrote:
>
>> I believe it sends the wrong message and is probably confusing to throw
>> an error when someone writes a CREATE TABLE with an empty SORT BY() but
>> allow the same clause in an ALTER. No doubt the users can read the
>> documentation and figure it out but its an extra step. Also, as Marcel
>> mentions, scripting may be easier as you don't have to figure out whether
>> to add a clause or not. Anyways, the patch is great as is, I just wanted to
>> mention this.
>>
>
> Sounds like we should allow an empty list then.
>

I will change the parser accordingly. On a related note, ALTER TABLE SORT
BY () will leave an empty property 'sort.by.columns'. Should it remove the
property altogether instead?

>
>
>>
>>
>> Dimitris
>>
>> On Mon, May 8, 2017 at 7:50 AM, Marcel Kornacker <marcel@cloudera.com>
>> wrote:
>>
>>>
>>>
>>> On Sat, May 6, 2017 at 7:57 PM, Dimitris Tsirogiannis <
>>> dtsirogiannis@cloudera.com> wrote:
>>>
>>>> For consistency, I believe we should at least allow an empty SORT BY()
>>>> clause in the CREATE TABLE statement, but I'll defer the decision to Alex
>>>> or Marcel.
>>>>
>>>
>>> Do we think there'll be scripts generating create table statements with
>>> empty sort-by clauses? I'm not sure there's a benefit to supporting that
>>> (but happy to stand corrected).
>>>
>>>
>>>>
>>>> Dimitris
>>>>
>>>> On Sat, May 6, 2017 at 8:16 AM, Lars Volker (Code Review) <
>>>> gerrit@cloudera.org> wrote:
>>>>
>>>>> Lars Volker has posted comments on this change.
>>>>>
>>>>> Change subject: IMPALA-4166: Add SORT BY sql clause
>>>>> ......................................................................
>>>>>
>>>>>
>>>>> Patch Set 20:
>>>>>
>>>>> > There is still one thing that is not clear to me. Why is it allowed
>>>>>  > to do an ALTER TABLE with an empty SORT BY clause but not a CREATE
>>>>>  > TABLE?
>>>>>
>>>>> The easiest way to specify an empty list of sort by columns during
>>>>> CREATE TABLE is to omit the SORT BY clause altogether. To keep things
>>>>> simple I did not add additional support for an empty SORT BY () clause.
>>>>>
>>>>> For ALTER TABLE there needs to be a way to specify an empty list of
>>>>> columns, but since SORT BY is used to identify the command, the most
simple
>>>>> form seemed to be an empty column list().
>>>>>
>>>>> Do you think we should allow CREATE TABLE SORT BY() in addition to
>>>>> specify an empty set of column?
>>>>>
>>>>> --
>>>>> To view, visit http://gerrit.cloudera.org:8080/6495
>>>>> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>>>>>
>>>>> Gerrit-MessageType: comment
>>>>> Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75
>>>>> Gerrit-PatchSet: 20
>>>>> Gerrit-Project: Impala-ASF
>>>>> Gerrit-Branch: master
>>>>> Gerrit-Owner: Lars Volker <lv@cloudera.com>
>>>>> Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
>>>>> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
>>>>> Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
>>>>> Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
>>>>> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
>>>>> Gerrit-HasComments: No
>>>>>
>>>>
>>>>
>>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message