Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CAM_aD9eL2Vxr2NMFSao64YjeSkJ1aAtFBYSsO=V_+6=iRkj_dA@mail.gmail.com>
References: <gerrit.1490644362000.I08834f38a941786ab45a4381c2732d929a934f75@gerrit.cloudera.org>
 <201705061516.v46FGUCT009148@ip-10-146-233-104.ec2.internal>
 <CAEtx3x0S2RakGcjbqbKN2g1hY3j8th6+y_PEkHSRTpYMsE6ztw@mail.gmail.com>
 <CAM7YqvP_b=JR3wj_eULq8qid+Vpx4wgej2gm7r=a172H0pE2iA@mail.gmail.com>
 <CAEtx3x0u4ST15Y20wu4PRfszR_XfMBt+sBh238hwe=7gecEa3Q@mail.gmail.com>
 <CAM7YqvM=_j5W4jry48pH6AiptsyKstzsNwABq3FSC=o3ij7Dvg@mail.gmail.com> <CAM_aD9eL2Vxr2NMFSao64YjeSkJ1aAtFBYSsO=V_+6=iRkj_dA@mail.gmail.com>
From: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Date: Mon, 8 May 2017 16:24:19 -0700
Message-ID: <CAEtx3x3NDErsGrWRjxSU3Q0xpdbayWCEw5Q1Q+bOzGjVc_bvyA@mail.gmail.com>
Subject: Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
To: Lars Volker <lv@cloudera.com>
Cc: Marcel Kornacker <marcel@cloudera.com>, impala-cr <impala-cr@cloudera.com>,
	reviews@impala.incubator.apache.org,
	Thomas Tauber-Marshall <tmarshall@cloudera.com>, Alex Behm <alex.behm@cloudera.com>
Content-Type: multipart/alternative; boundary=94eb2c1abcee147d84054f0b8b84
archived-at: Mon, 08 May 2017 23:24:46 -0000

--94eb2c1abcee147d84054f0b8b84
Content-Type: text/plain; charset=UTF-8

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

--94eb2c1abcee147d84054f0b8b84--