Bharath Vissapragada 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 2: (3 comments) Nice test coverage. Can +2 once the nits are addressed. http://gerrit.cloudera.org:8080/#/c/13753/2/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/2/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@854 PS2, Line 854: planHints_.add(new PlanHint(hint)); nit: planHints_.add(new PlanHint(hint.trim())); http://gerrit.cloudera.org:8080/#/c/13753/2/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@842 PS2, Line 842: if (planHints_.isEmpty() && : ((table_ instanceof FeHBaseTable) || : !(analyzer.getQueryOptions().isSetDefault_hints_insert_statement()))) return; : : // Set up the plan hints from query option DEFAULT_HINTS_INSERT_STATEMENT. : // Default hint is ignored, if the original statement had hints. : if (planHints_.isEmpty() && : analyzer.getQueryOptions().isSetDefault_hints_insert_statement()) { : String defaultHints = : analyzer.getQueryOptions().getDefault_hints_insert_statement(); : for (String hint: defaultHints.trim().split(":")) { : hint = hint.trim(); : planHints_.add(new PlanHint(hint)); : } : } : : if (table_ instanceof FeHBaseTable) { : throw new AnalysisException(String.format("INSERT hints are only supported for " + : "inserting into Hdfs and Kudu tables: %s", getTargetTableName())); : } I think this can be formatted to be more readable. The checks on the hbase table seem scattered. How about something like? if (table_ instanceof HBaseTable) { if (!planHints.isEmpty()) throw AnalysisException() return; // Don't care about default hints, even if they are set. } // Now we know it is not an HBase table, populate the hints if (planHints_isEmpty() && defaultOptionsSet()) { parseDefaultOptions().. } http://gerrit.cloudera.org:8080/#/c/13753/2/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/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2038 PS2, Line 2038: AnalyzesOk(String.format("insert into table functional_kudu.alltypes " + Mind adding some checks that - verify that the query level hints override the default hints - default hints are actually applied during analysis when no query hints are provided. In either case you can do AnalysisCtx.getInsertStmt().getHints().contains() .. once you analyze the query. (I see that there is some e-e coverage in the test files for this, but probably good to add some asserts in the unit test) -- 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: 2 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@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: Tue, 02 Jul 2019 00:24:02 +0000 Gerrit-HasComments: Yes
