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 Tue, 09 May 2017 12:06:25 GMT
On Tue, May 9, 2017 at 2:25 AM, Dimitris Tsirogiannis <
dtsirogiannis@cloudera.com> wrote:

> If you don't populate it for CREATE TABLE SORT BY() then I think you
> should remove it altogether.
>

We don't set it there. On second look, we don't support removing table
properties and would need to add that capability to maintain consistency
here. Should we do this? As an alternative, we can set the table property
to an empty value for CREATE TABLE SORT BY()?

I'm good with either of these. Even leaving it set to an empty value had it
ever been set seems fine to me, since it's an internal implementation
detail and won't affect functionality.


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