impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Wed, 05 Apr 2017 05:13:43 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message