You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2017/06/16 23:21:25 UTC
[Impala-ASF-CR] IMPALA-2546: Fix handeling of invalid timezones
Bikramjeet Vig has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7216
Change subject: IMPALA-2546: Fix handeling of invalid timezones
......................................................................
IMPALA-2546: Fix handeling of invalid timezones
The bug was that when invalid timezones were passed to udfs
"to_utc_timestamp" and "from_utc_timestamp", a warning was shown and
original values were returned. The correct behaviour is that an error
should be returned instead.
Testing:
Added expression tests for both udfs
Change-Id: I587a16bb8e835a6eea02e1f87fd4033686e84d0f
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
2 files changed, 5 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7216/1
--
To view, visit http://gerrit.cloudera.org:8080/7216
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I587a16bb8e835a6eea02e1f87fd4033686e84d0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-2546: Fix handling of invalid timezones
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change.
Change subject: IMPALA-2546: Fix handling of invalid timezones
......................................................................
Patch Set 1:
(1 comment)
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());
> I don't see an end-to-end test that covers this.
its at exprs.test:2618
and the test for the similar code path in to_utc_timestamp is at exprs.test:2630
--
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2546: Fix handling of invalid timezones
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has abandoned this change.
Change subject: IMPALA-2546: Fix handling of invalid timezones
......................................................................
Abandoned
Abandoning for now - Greg pointed out in the JIRA that the current behavior of these functions make sense.
--
To view, visit http://gerrit.cloudera.org:8080/7216
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I587a16bb8e835a6eea02e1f87fd4033686e84d0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-2546: Fix handling of invalid timezones
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-2546: Fix handling of invalid timezones
......................................................................
Patch Set 1:
(1 comment)
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());
> The test case for this code path has already been added as a part of IMPALA
I don't see an end-to-end test that covers this.
--
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2546: Fix handling of invalid timezones
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-2546: Fix handling of invalid timezones
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
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());
> its at exprs.test:2618
Ah thanks. Not sure what you meant by it was part of your IMPALA-3504 commit though but I see the test.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2546: Fix handling of invalid timezones
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has uploaded a new patch set (#2).
Change subject: IMPALA-2546: Fix handling of invalid timezones
......................................................................
IMPALA-2546: Fix handling of invalid timezones
The bug was that when invalid timezones were passed to udfs
"to_utc_timestamp" and "from_utc_timestamp", a warning was shown and
original values were returned. The correct behaviour is that an error
should be returned instead.
Testing:
Added expression tests for both udfs
Change-Id: I587a16bb8e835a6eea02e1f87fd4033686e84d0f
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
3 files changed, 16 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7216/2
--
To view, visit http://gerrit.cloudera.org:8080/7216
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I587a16bb8e835a6eea02e1f87fd4033686e84d0f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-2546: Fix handeling of invalid timezones
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2546: Fix handling of invalid timezones
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change.
Change subject: IMPALA-2546: Fix handling 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
Done
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
Done
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 t
The test case for this code path has already been added as a part of IMPALA-3504 commit
--
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes