Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause

[Lars Volker]

On Tue, May 9, 2017 at 2:25 AM, Dimitris Tsirogiannis <
dtsirogian...@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  wrote:
>
>>
>>
>> On Tue, May 9, 2017 at 1:24 AM, Dimitris Tsirogiannis <
>> dtsirogian...@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  wrote:
>>>

 On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker 
 wrote:

>
>
> On Mon, May 8, 2017 at 9:50 AM, Dimitris Tsirogiannis <
> dtsirogian...@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 > > wrote:
>>
>>>
>>>
>>> On Sat, May 6, 2017 at 7:57 PM, Dimitris Tsirogiannis <
>>> dtsirogian...@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) <
 ger...@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 
> Gerrit-Reviewer: Alex Behm 
> Gerrit-Reviewer: Dimitris Tsirogiannis 

Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause

[Dimitris Tsirogiannis]

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

>
>
> On Tue, May 9, 2017 at 1:24 AM, Dimitris Tsirogiannis <
> dtsirogian...@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  wrote:
>>
>>>
>>> On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker 
>>> wrote:
>>>


 On Mon, May 8, 2017 at 9:50 AM, Dimitris Tsirogiannis <
 dtsirogian...@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 
> wrote:
>
>>
>>
>> On Sat, May 6, 2017 at 7:57 PM, Dimitris Tsirogiannis <
>> dtsirogian...@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) <
>>> ger...@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 
 Gerrit-Reviewer: Alex Behm 
 Gerrit-Reviewer: Dimitris Tsirogiannis 
 Gerrit-Reviewer: Lars Volker 
 Gerrit-Reviewer: Marcel Kornacker 
 Gerrit-Reviewer: Thomas Tauber-Marshall 
 Gerrit-HasComments: No

>>>
>>>
>>
>

>>>
>>
>

Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause

[Lars Volker]

On Tue, May 9, 2017 at 1:24 AM, Dimitris Tsirogiannis <
dtsirogian...@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  wrote:
>
>>
>> On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker 
>> wrote:
>>
>>>
>>>
>>> On Mon, May 8, 2017 at 9:50 AM, Dimitris Tsirogiannis <
>>> dtsirogian...@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 
 wrote:

>
>
> On Sat, May 6, 2017 at 7:57 PM, Dimitris Tsirogiannis <
> dtsirogian...@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) <
>> ger...@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 
>>> Gerrit-Reviewer: Alex Behm 
>>> Gerrit-Reviewer: Dimitris Tsirogiannis 
>>> Gerrit-Reviewer: Lars Volker 
>>> Gerrit-Reviewer: Marcel Kornacker 
>>> Gerrit-Reviewer: Thomas Tauber-Marshall 
>>> Gerrit-HasComments: No
>>>
>>
>>
>

>>>
>>
>

Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause

[Dimitris Tsirogiannis]

The other alternative would be to populate it with the partitioning
columns, if any. Thoughts?

Dimitris

On Mon, May 8, 2017 at 4:02 PM, Lars Volker  wrote:

>
> On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker 
> wrote:
>
>>
>>
>> On Mon, May 8, 2017 at 9:50 AM, Dimitris Tsirogiannis <
>> dtsirogian...@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 
>>> wrote:
>>>


 On Sat, May 6, 2017 at 7:57 PM, Dimitris Tsirogiannis <
 dtsirogian...@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) <
> ger...@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 
>> Gerrit-Reviewer: Alex Behm 
>> Gerrit-Reviewer: Dimitris Tsirogiannis 
>> Gerrit-Reviewer: Lars Volker 
>> Gerrit-Reviewer: Marcel Kornacker 
>> Gerrit-Reviewer: Thomas Tauber-Marshall 
>> Gerrit-HasComments: No
>>
>
>

>>>
>>
>

Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause

[Lars Volker]

On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker 
wrote:

>
>
> On Mon, May 8, 2017 at 9:50 AM, Dimitris Tsirogiannis <
> dtsirogian...@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 
>> wrote:
>>
>>>
>>>
>>> On Sat, May 6, 2017 at 7:57 PM, Dimitris Tsirogiannis <
>>> dtsirogian...@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) <
 ger...@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 
> Gerrit-Reviewer: Alex Behm 
> Gerrit-Reviewer: Dimitris Tsirogiannis 
> Gerrit-Reviewer: Lars Volker 
> Gerrit-Reviewer: Marcel Kornacker 
> Gerrit-Reviewer: Thomas Tauber-Marshall 
> Gerrit-HasComments: No
>


>>>
>>
>

Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause

[Dimitris Tsirogiannis]

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.

Dimitris

On Sat, May 6, 2017 at 8:16 AM, Lars Volker (Code Review) <
ger...@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 
> Gerrit-Reviewer: Alex Behm 
> Gerrit-Reviewer: Dimitris Tsirogiannis 
> Gerrit-Reviewer: Lars Volker 
> Gerrit-Reviewer: Marcel Kornacker 
> Gerrit-Reviewer: Thomas Tauber-Marshall 
> Gerrit-HasComments: No
>