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 <l...@cloudera.com> 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 <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 >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
