impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5338: Fix Kudu timestamp column default values
Date Wed, 31 May 2017 19:27:30 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-5338: Fix Kudu timestamp column default values
......................................................................


Patch Set 2:

(9 comments)

could you also add tosql tests?

http://gerrit.cloudera.org:8080/#/c/6936/2/be/src/exprs/timestamp-functions.h
File be/src/exprs/timestamp-functions.h:

Line 106:   static TimestampVal TimestampFromUnixMicros(FunctionContext* context,
the other functions don't have a "Timestamp" prefix, leave it out here as well.


http://gerrit.cloudera.org:8080/#/c/6936/2/be/src/runtime/timestamp-value.inline.h
File be/src/runtime/timestamp-value.inline.h:

Line 46:   tm temp_tm = boost::posix_time::to_tm(temp);
i pointed this out as a potential performance problem and asked you to leave a todo with a
pointer to a code snippet. could you please add that?

have there been any perf tests that quantify the cost of these conversions?


http://gerrit.cloudera.org:8080/#/c/6936/2/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java:

Line 271:               "cast to a default value for a TIMESTAMP column.", defaultValue_.toSql()));
i'd say "to a TIMESTAMP literal', just to be absolutely clear


Line 274:  
trailing


Line 296:               "Error converting timestamp default value: " + defaultValue_.toSql(),
e);
so you're converting an internal exception to an analysis exception here? an analysis exception
should be actionable for the user (e.g., fix your syntax), but it doesn't look like that's
the case here.


http://gerrit.cloudera.org:8080/#/c/6936/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java
File fe/src/main/java/org/apache/impala/util/KuduUtil.java:

Line 168:    * Returns the actual value of the specified defaultValue literal. Checks that
i still don't understand what the return types could be (looks like T<>Literal, but
I need to look at the code for that).


Line 221:       throw new InternalException("Error converting timestamp expression: " +
can this happen?


http://gerrit.cloudera.org:8080/#/c/6936/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

Line 680:   public void TestAlterTableSetCached() {
why can this go? or did it accidentally get dropped?


http://gerrit.cloudera.org:8080/#/c/6936/2/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

Line 650:   def test_timestamp_default_value(self, cursor):
can't you do this with a simple .test file? that would be easier to read and maintain.


-- 
To view, visit http://gerrit.cloudera.org:8080/6936
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I655910fb4805bb204a999627fa9f68e43ea8aaf2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message