Lars Volker has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 8: (18 comments) Thank you for the review, please see PS9. http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS7, Line 984: Sor > There is no KW_SET, right? Update the name? Done http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/AlterTableSetSortByColumnsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetSortByColumnsStmt.java: PS7, Line 73: > t? Done http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 2484: * Add a warning that will be displayed to the user. Ignores null messages. Once > Comment that no warning can be issued if warnings have been retrieved? Done http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java: PS7, Line 119: ortByColumns_ ! > I think this can throw a NPE. Looking at the parser I see that CreateTableL Thanks for catching this. I was able to trigger the NPE with an additional test in ToSqlTest.java. Fixed. http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: PS7, Line 296: > inline Done http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java File fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java: PS7, Line 41: : /** : * Analyzes the 'sort.by.columns' property in 'tblProperties' against the columns of : * 'table'. : */ : public static List<Integer> analyzeSortByColumns(Map<String, String> tblProperties, : Table table) throws AnalysisException { : if (!tblProperties.containsKey(TBL_PROP_SORT_BY_COLUMNS)) { : return Lists.newArrayList(); : } : : List<String> sortByCols = Lists.newArrayList( : Splitter.on(",").trimResults().omitEmptyStrings().split( : tblProperties.get(TBL_PROP_SORT_BY_COLUMNS))); : return TablePropertyAnalyzer.analyzeSortByColumns(sortByCols, table); : } : > These are used in only one place, right? Maybe inline there? I removed the first one. The second one is used in two places: InsertStmt.analyzeSortByColumns() (that's where I inlined the first one), and AlterTableSetTblProperties.analyze(). I'd keep the second one unless you lean strongly towards inlining it. Line 76: } > Precondition check for HBase? Done PS7, Line 127: : > You may want to consider if it's better to have a getSortByColumn(Table tab Inlined it instead. http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: PS7, Line 539: // If the table has a 'sort.by.columns' property and the query has a 'noclustered' : // hint, we issue a warning during analysis and ignore the 'noclustered' hint. > That comment doesn't seem relevant here. Remove? I put it there to explain the intent of the if statement. Without it one might think that the noclusteredhint gets ignored by mistake (see previous review comment by Thomas). Should I remove/rephrase it? PS7, Line 541: nsertStmt.hasClusteredHint() || > If insertStmt.hasClusteredHint() is true doesn't that imply that hasNoClust True, fixed. http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: PS7, Line 1793: !params.sort_by_columns.isEmpty() > use isEmpty() Done http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: Line 526: String long_property_key = ""; > Test for HBase and Kudu? Added tests for Kudu in testAlterKuduTable(). HBase tables do not support "ALTER TABLE SET" altogether, which is tested in L683. Line 1054: > Same here (HBase + Kudu). You need to put all Kudu tests in a function and Added test to check that HBase is not supported. Added Kudu tests to TestAlterKuduTable(). PS7, Line 1904: te t > "must" typo Done PS7, Line 1920: est > No need for that comment :) Done http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: PS7, Line 2347: // Sort by clause > How about some tests with additional table options? Try changing the order Done http://gerrit.cloudera.org:8080/#/c/6495/7/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test: PS7, Line 207: selec > select query Done http://gerrit.cloudera.org:8080/#/c/6495/7/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test File testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test: PS7, Line 194: sort by columns with a join > can you also add an order by clause? Done, though it needs a LIMIT, or else the analysis will fail. Now there's a TOP-N node in the plan. Let me know if we also should add a test that ends up having a sort node. Using DISABLE_OUTERMOST_TOPN should allow us to do that. -- To view, visit http://gerrit.cloudera.org:8080/6495 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
