Dan Hecht has posted comments on this change. Change subject: IMPALA-3579: Strict handling of numeric overflow in text parsing ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/3150/1//COMMIT_MSG Commit Message: Line 13: invalid data, i.e. NULL is returned. this doesn't seem right. when the option is set, error is returned (not NULL). Line 21: However, integer and floating point parsing never returns : an underflow today. what cases would be integer and floating point underflow? For integer, we treat val < min_val as overflow already, right? so integer seems completely handled. Is there a case we're missing for floating point? http://gerrit.cloudera.org:8080/#/c/3150/1/be/src/exec/text-converter.cc File be/src/exec/text-converter.cc: Line 249: (though the latter are only possible for : // DECIMAL which isn't codegen'd) is underflow only possible given the way the code works today or by definition for other types? in either case, I think this part is more confusing than it's worth - how about deleting it? http://gerrit.cloudera.org:8080/#/c/3150/1/be/src/exec/text-converter.inline.h File be/src/exec/text-converter.inline.h: Line 171: // TODO: add warnings what is this TODO about at this point? shouldn't it be the caller would do the warning, anyway? http://gerrit.cloudera.org:8080/#/c/3150/1/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 200: // Additional strict handling of invalid values in parsing. I think we'll want this option to have broader meaning soon enough, so maybe reword this to more closely match it's final meaning? http://gerrit.cloudera.org:8080/#/c/3150/1/testdata/workloads/functional-query/queries/QueryTest/strict-mode-abort.test File testdata/workloads/functional-query/queries/QueryTest/strict-mode-abort.test: Line 32 how about testing negative values for all these cases as well -- To view, visit http://gerrit.cloudera.org:8080/3150 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7409c31ec0cb6fe0b2d9842b9f58fe1670914836 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
