[Impala-ASF-CR] IMPALA-8673: Add query option to force plan hints for insert queries

["Abhishek Rawat (Code Review)" <ge...@cloudera.org>]

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 <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jul 2019 22:02:22 +0000
Gerrit-HasComments: Yes