[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950: Improve error message in case of out-of-range numeric conversions .. IMPALA-12950: Improve error message in case of out-of-range numeric conversions IMPALA-12035 introduced checks for numeric conversions that are unsafe and can fail (if the target type cannot store the value, the behaviour is undefined): - from floating-point types to integer types - from double to float However, it can be difficult to trace which part of the query caused this based on the error message. This change adds the source type, the destination type and the value to be converted to the error message. Unfortunately, at this point in the BE, the original SQL is not available, so we cannot reference that. Testing: - extended existing tests in expr-test.cc. Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Reviewed-on: http://gerrit.cloudera.org:8080/21331 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/exprs/cast-functions-ir.cc M be/src/exprs/expr-test.cc M be/src/udf/udf.h 3 files changed, 85 insertions(+), 24 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa
[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950: Improve error message in case of out-of-range numeric conversions .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Comment-Date: Thu, 25 Apr 2024 16:26:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950: Improve error message in case of out-of-range numeric conversions .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10579/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Comment-Date: Thu, 25 Apr 2024 11:22:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950: Improve error message in case of out-of-range numeric conversions .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Comment-Date: Thu, 25 Apr 2024 08:36:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Peter Rozsa has posted comments on this change. (
http://gerrit.cloudera.org:8080/21331 )
Change subject: IMPALA-12950: Improve error message in case of out-of-range
numeric conversions
..
Patch Set 3: Code-Review+1
(1 comment)
Thank you, Daniel!
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc
File be/src/exprs/cast-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc@76
PS2, Line 76: } else {
> It's a good point. These are the only types we'd like to cover here, so ori
I think we have a bunch of mediocre options to choose from:
1. Use a default constant that will be printed when we pass an unknown type:
makes the whole type enforcing a bit weaker as its main goal is to provide a
name just for the defined types.
2. Using the static_assert trick with SFINAE yields ill-formed code
3. Using throw clause: ill-formed code
4. Using DCHECK: needs to duplicate the type names, and I'm not sure that the
internals of DCHECK are feasible for constexpr functions
+1: consteval from C++20 :)
In my opinion, the least concerning is the third one, what you added in the
third patchset, in the future, in case of an upgrade to C++20 it could be
replaced with a consteval function.
Any further inputs from other reviewers are welcome, as it's a tough choice.
--
To view, visit http://gerrit.cloudera.org:8080/21331
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32
Gerrit-Change-Number: 21331
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Peter Rozsa
Gerrit-Comment-Date: Tue, 23 Apr 2024 14:55:11 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950: Improve error message in case of out-of-range numeric conversions .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/15995/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Comment-Date: Tue, 23 Apr 2024 13:27:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Daniel Becker has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950: Improve error message in case of out-of-range numeric conversions .. IMPALA-12950: Improve error message in case of out-of-range numeric conversions IMPALA-12035 introduced checks for numeric conversions that are unsafe and can fail (if the target type cannot store the value, the behaviour is undefined): - from floating-point types to integer types - from double to float However, it can be difficult to trace which part of the query caused this based on the error message. This change adds the source type, the destination type and the value to be converted to the error message. Unfortunately, at this point in the BE, the original SQL is not available, so we cannot reference that. Testing: - extended existing tests in expr-test.cc. Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 --- M be/src/exprs/cast-functions-ir.cc M be/src/exprs/expr-test.cc M be/src/udf/udf.h 3 files changed, 85 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/21331/3 -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa
[Impala-ASF-CR] IMPALA-12950: Improve error message in case of out-of-range numeric conversions
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/21331 )
Change subject: IMPALA-12950: Improve error message in case of out-of-range
numeric conversions
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/21331/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21331/2//COMMIT_MSG@7
PS2, Line 7: :
> nit: missing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/21331/2//COMMIT_MSG@12
PS2, Line 12: floating-point
> nit: floating-point
Done
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc
File be/src/exprs/cast-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc@76
PS2, Line 76: } else {
> The default case is missing, it could be added as "UNKNOWN TYPE" or somethi
It's a good point. These are the only types we'd like to cover here, so
originally I wanted to add
static_assert(false);
but it doesn't compile. I could write
static_assert(!std::is_same_v), which is always false, however this
link suggests even that may be ill-formed:
https://stackoverflow.com/questions/38304847/constexpr-if-and-static-assert
On the other hand, this latter approach seems to work in practice, but I'm not
sure we should do that if it's not guaranteed. Leaving out the default case
deterministically leads to a warning about no return statement so I did it like
this.
Maybe adding a DCHECK would be best, but I don't like that there doesn't seem
to be a clean and concise way of doing it compile time without repeating the
types (e.g. static_assert that T is one of the types, or some SFINAE magic).
In the new patch set I went with static_assert(!std::is_same_v), it
correctly fires when I try to instantiate the template with a different type,
but I don't know if it's guaranteed or not. What do you think?
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc@182
PS2, Line 182: constexpr const char* FROM_TYPE_NAME =
TypeToName();
> Could you please add test cases to cover each condition?
Extended existing tests and added some new ones in expr-test.cc.
--
To view, visit http://gerrit.cloudera.org:8080/21331
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32
Gerrit-Change-Number: 21331
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Peter Rozsa
Gerrit-Comment-Date: Tue, 23 Apr 2024 13:03:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12950:Improve error message in case of out-of-range numeric conversions
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950:Improve error message in case of out-of-range numeric conversions .. Patch Set 2: Code-Review+1 lgtm (besides Peter's comments) -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Comment-Date: Tue, 23 Apr 2024 06:20:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12950:Improve error message in case of out-of-range numeric conversions
Peter Rozsa has posted comments on this change. (
http://gerrit.cloudera.org:8080/21331 )
Change subject: IMPALA-12950:Improve error message in case of out-of-range
numeric conversions
..
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/21331/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21331/2//COMMIT_MSG@7
PS2, Line 7: :
nit: missing whitespace
http://gerrit.cloudera.org:8080/#/c/21331/2//COMMIT_MSG@12
PS2, Line 12: floating point
nit: floating-point
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc
File be/src/exprs/cast-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc@76
PS2, Line 76: }
The default case is missing, it could be added as "UNKNOWN TYPE" or something
similar.
http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc@182
PS2, Line 182: err = Substitute("NaN value of type $0 cannot be converted
to $1.",
Could you please add test cases to cover each condition?
--
To view, visit http://gerrit.cloudera.org:8080/21331
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32
Gerrit-Change-Number: 21331
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Peter Rozsa
Gerrit-Comment-Date: Mon, 22 Apr 2024 12:25:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12950:Improve error message in case of out-of-range numeric conversions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21331 ) Change subject: IMPALA-12950:Improve error message in case of out-of-range numeric conversions .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/15945/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 18 Apr 2024 15:32:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12950:Improve error message in case of out-of-range numeric conversions
Daniel Becker has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21331 Change subject: IMPALA-12950:Improve error message in case of out-of-range numeric conversions .. IMPALA-12950:Improve error message in case of out-of-range numeric conversions IMPALA-12035 introduced checks for numeric conversions that are unsafe and can fail (if the target type cannot store the value, the behaviour is undefined): - from floating point types to integer types - from double to float However, it can be difficult to trace which part of the query caused this based on the error message. This change adds the source type, the destination type and the value to be converted to the error message. Unfortunately, at this point in the BE, the original SQL is not available, so we cannot reference that. Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 --- M be/src/exprs/cast-functions-ir.cc M be/src/udf/udf.h 2 files changed, 35 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/21331/2 -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker
