Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/13753 )
Change subject: IMPALA-8673: Add query option to force plan hints for insert queries ...................................................................... Patch Set 1: (8 comments) Thanks for review comments. http://gerrit.cloudera.org:8080/#/c/13753/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13753/1//COMMIT_MSG@28 PS1, Line 28: > Can you mention which table formats this applies to - HDFS/Kudu/HBase? Done http://gerrit.cloudera.org:8080/#/c/13753/1/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/13753/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@841 PS1, Line 841: ((table_ instanceof FeHBaseTable) || > I think the HBase bit deserves some explanation. The idea is that the hints Yeah the insert hints are not supported for HBase table. And if the new query option is set and we come across a HBase table we should ignore the default hints. Added a comment. http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@844 PS1, Line 844: Setup > Set up Done http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@850 PS1, Line 850: toks > nit: could skip the intermediate value and inline the RHS on the below line Done http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@852 PS1, Line 852: hint.trim(); > Is there a test to make sure that whitespace is ignored? Updated some tests to have whitespaces. http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2042 PS1, Line 2042: insertCtx.getQueryOptions().setDefault_hints_insert_statement("NOCLUSTERED:noshuffle"); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@190 PS1, Line 190: @Test > The tests are pretty comprehensive, but can you add a sanity test for HBase Done http://gerrit.cloudera.org:8080/#/c/13753/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@193 PS1, Line 193: options.setDefault_hints_insert_statement("clustered"); > You added the .trim() to make this whitespace-insensitive. Maybe add some v Added white spaces. -- To view, visit http://gerrit.cloudera.org:8080/13753 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c3f213402b8e4d1940f96738ad21edf800fa43a Gerrit-Change-Number: 13753 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 01 Jul 2019 22:02:22 +0000 Gerrit-HasComments: Yes
