[BugFix] Fix check overflow for widening conversion (backport #51280) #51428
+266
−72
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CP from #51280.
Why I'm doing:
Introduced by PR #49707. It fixes an issue in the overflow check when casting a
double
to an integral type. And it abstractsCastExpr::NumberCheck
intocheck_number_overflow
, allowing the same issue to be resolved for both Json and Avro formats.However,
CastExpr::NumberCheck
only handles narrowing conversions, not widening conversions, because it is invoked only during narrowing conversions.When performing a widening conversion, the expression
value > (**FromType**)std::numeric_limits<ToType>::max()
may returns true. This occurs because converting the maximum value of a larger type (ToType
) into a smaller type (FromType
) results in an overflow and a negative value.Issues in Json Import:
double
toint64
might lead to undefined behavior.double
value toint64
will give the maximumint64
value, so no issue arises.double
value toint64
could result in the minimumint64
value, causing problems.Issues in Avro Import:
Special Case: Values in the Range$[2^{63}, 2^{64})$
When the value falls within the range$$[2^{63}, 2^{64})$$ , Json interprets the
FromType
asuint64_t
. In such cases, usingcheck_signed_number_overflow
remains correct:ToType
is any signed integer (int8_t
toint64_t
),check_signed_number_overflow
will returntrue
, indicating overflow.ToType
isfloat
,double
, orint128_t
,check_signed_number_overflow
will returnfalse
since the widening conversion path is followed.What I'm doing:
Fixes https://github.com/StarRocks/StarRocksTest/issues/8603
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check:
This is an automatic backport of pull request #51280 done by [Mergify](https://mergify.com). ## Why I'm doing:
Introduced by PR #49707. It fixes an issue in the overflow check when casting a
double
to an integral type. And it abstractsCastExpr::NumberCheck
intocheck_number_overflow
, allowing the same issue to be resolved for both Json and Avro formats.However,
CastExpr::NumberCheck
only handles narrowing conversions, not widening conversions, because it is invoked only during narrowing conversions.When performing a widening conversion, the expression
value > (**FromType**)std::numeric_limits<ToType>::max()
may returns true. This occurs because converting the maximum value of a larger type (ToType
) into a smaller type (FromType
) results in an overflow and a negative value.Issues in Json Import:
double
toint64
might lead to undefined behavior.double
value toint64
will give the maximumint64
value, so no issue arises.double
value toint64
could result in the minimumint64
value, causing problems.Issues in Avro Import:
Special Case: Values in the Range$[2^{63}, 2^{64})$
When the value falls within the range$$[2^{63}, 2^{64})$$ , Json interprets the
FromType
asuint64_t
. In such cases, usingcheck_signed_number_overflow
remains correct:ToType
is any signed integer (int8_t
toint64_t
),check_signed_number_overflow
will returntrue
, indicating overflow.ToType
isfloat
,double
, orint128_t
,check_signed_number_overflow
will returnfalse
since the widening conversion path is followed.What I'm doing:
Fixes https://github.com/StarRocks/StarRocksTest/issues/8603
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist: