impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2546: Fix handeling of invalid timezones
Date Fri, 16 Jun 2017 23:47:48 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-2546: Fix handeling of invalid timezones
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7216/1//COMMIT_MSG
Commit Message:

PS1, Line 7: handeling
typo


http://gerrit.cloudera.org:8080/#/c/7216/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

PS1, Line 5115:   TestError("from_utc_timestamp('2015-10-13 09:15:34.101', 'FOOBAR')");
              :   TestError("to_utc_timestamp('2015-10-13 09:15:34.101', 'FOOBAR')");
this looks good, but it'd also be good to verify the error message is what we expect it to
be so we know we're hitting the expected code path.

you could modify this test code to optionally take an error, but our end-to-end tests already
handle this functionality and we shouldn't try to rebuild everything in expr-test.

Can you add these test cases to testdata/workloads/functional-query/queries/QueryTest/exprs.test
?

There are some examples where there is an expected error, search for CATCH


http://gerrit.cloudera.org:8080/#/c/7216/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:

Line 110:     context->AddWarning(msg.c_str());
while you're adding the end-to-end test cases, it'd be nice to add a test to check these warnings
as well.

See testdata/workloads/functional-query/queries/DataErrorsTest/hdfs-scan-node-errors.test
for an example of a test that has a query that completes successfully but a warning is set
(the 'ERRORS' section).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I587a16bb8e835a6eea02e1f87fd4033686e84d0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message