Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9153 )
Change subject: IMPALA-5293: Turn insert clustering on by default ...................................................................... Patch Set 4: (5 comments) Thanks for the review. Please see my inline comments and PS4. http://gerrit.cloudera.org:8080/#/c/9153/3/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/9153/3/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@831 PS3, Line 831: analyzer.addWarning("Clustering is enabled by default, use 'noclustered' to " + > I'm wondering if the warning is necessary. I don't think we generally warn Adding the hint should not have any effect since we don't ensure to add a clustering sort if it is present. Would you prefer to re-add it for Kudu tables (see Planner.java)? http://gerrit.cloudera.org:8080/#/c/9153/3/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/9153/3/fe/src/main/java/org/apache/impala/planner/Planner.java@615 PS3, Line 615: primar > primary Done http://gerrit.cloudera.org:8080/#/c/9153/3/testdata/workloads/functional-planner/queries/PlannerTest/empty.test File testdata/workloads/functional-planner/queries/PlannerTest/empty.test: http://gerrit.cloudera.org:8080/#/c/9153/3/testdata/workloads/functional-planner/queries/PlannerTest/empty.test@302 PS3, Line 302: insert into functional.alltypes partition(year, month) > IMO this seems ok, the benefit of optimising this case would be very margin Thanks for the feedback. I double checked with Alex and removed the todo. http://gerrit.cloudera.org:8080/#/c/9153/3/testdata/workloads/functional-planner/queries/PlannerTest/insert.test File testdata/workloads/functional-planner/queries/PlannerTest/insert.test: http://gerrit.cloudera.org:8080/#/c/9153/3/testdata/workloads/functional-planner/queries/PlannerTest/insert.test@101 PS3, Line 101: # IMPALA-5293: noclustered hint prevents adding sort node > Do we also need to test the case when there's a noclustered hint and sort.c Insert tests for tables with sort columns are in their own file, the particular case is covered in testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test:49. http://gerrit.cloudera.org:8080/#/c/9153/3/testdata/workloads/functional-query/queries/QueryTest/insert.test File testdata/workloads/functional-query/queries/QueryTest/insert.test: http://gerrit.cloudera.org:8080/#/c/9153/3/testdata/workloads/functional-query/queries/QueryTest/insert.test@949 PS3, Line 949: # IMPALA-6280: clustered (default) with outer join, inline view, and TupleisNullPredicate > It looks like this one should still be a clustered insert, since IMPALA-628 Thanks for catching this. I also added a test for this query to PlannerTest/insert.test to make sure we get the plan right. -- To view, visit http://gerrit.cloudera.org:8080/9153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbf2368cf4415e6ecfa65058daf6ff87ef62f9d9 Gerrit-Change-Number: 9153 Gerrit-PatchSet: 4 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Jan 2018 20:06:33 +0000 Gerrit-HasComments: Yes
