impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Subject Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Tue, 09 May 2017 00:25:15 GMT
If you don't populate it for CREATE TABLE SORT BY() then I think you should
remove it altogether.

Dimitris

On Mon, May 8, 2017 at 4:33 PM, Lars Volker <lv@cloudera.com> wrote:

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