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 <[email protected]> wrote: > > > On Tue, May 9, 2017 at 1:24 AM, Dimitris Tsirogiannis < > [email protected]> 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 <[email protected]> wrote: >> >>> >>> On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker <[email protected]> >>> wrote: >>> >>>> >>>> >>>> On Mon, May 8, 2017 at 9:50 AM, Dimitris Tsirogiannis < >>>> [email protected]> 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 <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Sat, May 6, 2017 at 7:57 PM, Dimitris Tsirogiannis < >>>>>> [email protected]> 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) < >>>>>>> [email protected]> 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 <[email protected]> >>>>>>>> Gerrit-Reviewer: Alex Behm <[email protected]> >>>>>>>> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> >>>>>>>> Gerrit-Reviewer: Lars Volker <[email protected]> >>>>>>>> Gerrit-Reviewer: Marcel Kornacker <[email protected]> >>>>>>>> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> >>>>>>>> Gerrit-HasComments: No >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
