Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-1903: Add support for partitioning by TIMESTAMP ......................................................................
Patch Set 5: (11 comments) Minor comments. The main issue left is the behavior when invalid timestamp values are used. http://gerrit.cloudera.org:8080/#/c/1621/5//COMMIT_MSG Commit Message: Line 17: it missing nit: "is" is missing :) http://gerrit.cloudera.org:8080/#/c/1621/5/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 5509: TestValue("nonnullvalue(cast('2010-01-01' as timestamp))", TYPE_BOOLEAN, true); For completion, let's also add one timestamp with date and time but without the fractional part. http://gerrit.cloudera.org:8080/#/c/1621/5/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: Line 359: CheckParse Or maybe ParseTimestamp? http://gerrit.cloudera.org:8080/#/c/1621/5/common/thrift/Types.thrift File common/thrift/Types.thrift: Line 24: unrelated enums Such as? http://gerrit.cloudera.org:8080/#/c/1621/5/fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java File fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java: Line 126: result = new TimestampLiteral(exprNode.timestamp_literal.value); Why don't you call the LiteralExpr.create() as we do in the other cases? http://gerrit.cloudera.org:8080/#/c/1621/5/fe/src/main/java/com/cloudera/impala/analysis/TimestampLiteral.java File fe/src/main/java/com/cloudera/impala/analysis/TimestampLiteral.java: Line 84: public int compareTo(LiteralExpr o) As we discussed offline, let's remove this. http://gerrit.cloudera.org:8080/#/c/1621/5/fe/src/main/java/com/cloudera/impala/catalog/ScalarType.java File fe/src/main/java/com/cloudera/impala/catalog/ScalarType.java: Line 278: if (!isSupported() || isComplexType()) { : return false; : } nit: single line http://gerrit.cloudera.org:8080/#/c/1621/5/fe/src/main/java/com/cloudera/impala/planner/DataSourceScanNode.java File fe/src/main/java/com/cloudera/impala/planner/DataSourceScanNode.java: Line 136: // TODO: we support DECIMAL but no way to specify it in SQL. I am not sure I understand what this TODO means. Can you check with Alex or maybe MJ? http://gerrit.cloudera.org:8080/#/c/1621/5/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 1262: nit: remove empty line http://gerrit.cloudera.org:8080/#/c/1621/5/testdata/workloads/functional-query/queries/QueryTest/timestamp-partitions.test File testdata/workloads/functional-query/queries/QueryTest/timestamp-partitions.test: Line 29: select * from t : where ts != '1999-12-31'; If they fit, can you put each statement in a single line? http://gerrit.cloudera.org:8080/#/c/1621/5/tests/metadata/test_recover_partitions.py File tests/metadata/test_recover_partitions.py: Line 209: nit: extra space -- To view, visit http://gerrit.cloudera.org:8080/1621 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icad7dcdc1b199cce9483dc414072bbe24efd625c Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
