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 <l...@cloudera.com> wrote: > >> >> On Mon, May 8, 2017 at 11:41 PM, Marcel Kornacker <mar...@cloudera.com> >> 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 <mar...@cloudera.com> >>>> 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 <l...@cloudera.com> >>>>>>> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> >>>>>>> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> >>>>>>> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> >>>>>>> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> >>>>>>> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> >>>>>>> Gerrit-HasComments: No >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
