Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CAM7YqvP_b=JR3wj_eULq8qid+Vpx4wgej2gm7r=a172H0pE2iA@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>
From: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Date: Mon, 8 May 2017 09:50:38 -0700
Message-ID: <CAEtx3x0u4ST15Y20wu4PRfszR_XfMBt+sBh238hwe=7gecEa3Q@mail.gmail.com>
Subject: Re: [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
To: Marcel Kornacker <marcel@cloudera.com>
Cc: Lars Volker <lv@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=001a114118e42cce0f054f060b6b
archived-at: Mon, 08 May 2017 16:51:11 -0000

--001a114118e42cce0f054f060b6b
Content-Type: text/plain; charset=UTF-8

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.

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

--001a114118e42cce0f054f060b6b--