Tim Armstrong 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: (7 comments) I did a pass over this. I think it's pretty close but I might not be able to finish the review (going on holiday for a couple of weeks). I think Lars or Bharath could finish this. 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? 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 don't make sense for HBase inserts, so we don't want to add nonsensical hints, right? 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 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. 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? 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 and Kudu to prove that the hints do not and do take effect, respectively. I don't think testing the full matrix below is necessary. 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 variations in whitespace around the hints here so we have testing to catch regressions in that. Or we could also be stricter about the whitespace. -- 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 <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 28 Jun 2019 16:48:29 +0000 Gerrit-HasComments: Yes
