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:33:33 GMT
On Tue, May 9, 2017 at 1:24 AM, Dimitris Tsirogiannis <
dtsirogiannis@cloudera.com> wrote:

> The other alternative would be to populate it with the partitioning
> columns, if any. Thoughts?
>
>
Adding the partitioning columns to the sort by list is not supported. They
can be added to the pre-insert sort not using the clustered hint. I'm not
sure though if I understood your statement correctly.

My question was what to do if a user executes "ALTER TABLE SORT BY();",
meaning to remove all sort by columns. Should we remove all columns from
the property, or remove the property altogether?


> Dimitris
>
> On Mon, May 8, 2017 at 4:02 PM, Lars Volker <lv@cloudera.com> wrote:
>
>>
>> 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