Jim Apple has posted comments on this change. Change subject: IMPALA-1903: Add support for partitioning by TIMESTAMP ......................................................................
Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/1621/5//COMMIT_MSG Commit Message: Line 17: it missing > nit: "is" is missing :) Done 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 Done 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? Changed to Parse, since TimestampParser::ParseTimestamp is a little too poetic for me. http://gerrit.cloudera.org:8080/#/c/1621/5/common/thrift/Types.thrift File common/thrift/Types.thrift: Line 24: unrelated enums > Such as? I don't know. This was from http://gerrit.cloudera.org:8080/2194 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? It's overkill - create is just going to branch on the type again. 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. Done 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 Done 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 Done 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? Done 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 Done -- 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
