Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 7: (18 comments) 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: Set There is no KW_SET, right? Update the name? 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: getTargetTable() t? 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. Comment that no warning can be issued if warnings have been retrieved? 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: sortByColumns_. I think this can throw a NPE. Looking at the parser I see that CreateTableListStmt can pass null for sortByColumns_ and couldn't find where this changes. 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: propertyString inline 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 of 'table'. : */ : public static List<Integer> analyzeSortByColumns(Table table) throws AnalysisException { : return TablePropertyAnalyzer.analyzeSortByColumns( : table.getMetaStoreTable().getParameters(), table); : } : : /** : * 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 { : return TablePropertyAnalyzer.analyzeSortByColumns( : getSortByColumnsFromProperties(tblProperties), table); : } These are used in only one place, right? Maybe inline there? Line 76: } Precondition check for HBase? PS7, Line 127: private static List<String> getSortByColumnsFromProperties( : Map<String, String> tblProperties) { You may want to consider if it's better to have a getSortByColumn(Table table) function instead of this one. Just a though... 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? PS7, Line 541: !insertStmt.hasNoClusteredHint() If insertStmt.hasClusteredHint() is true doesn't that imply that hasNoClusteredHint() is false? I thought they are mutually exclusive. 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.size() > 0 use isEmpty() 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: Test for HBase and Kudu? Line 1054: } Same here (HBase + Kudu). You need to put all Kudu tests in a function and check for supported. PS7, Line 1904: most "must" typo PS7, Line 1920: // Kudu table tests are in TestCreateManagedKuduTable(). No need for that comment :) 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 in which the table options are specified, e.g. location bla sort by () 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: table select query 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? -- 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: 7 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
