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<T, T>), 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<T, T>), 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<FROM_TYPE>(); > 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 <daniel.bec...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Tue, 23 Apr 2024 13:03:07 +0000 Gerrit-HasComments: Yes
