On Tue, May 9, 2017 at 2:25 AM, Dimitris Tsirogiannis < [email protected]> 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 <[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 >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
