[SPARK-59643][SQL] Tighten CheckOverflow nullability to match MakeDecimal - #58913
Open
david-mollitor-db wants to merge 1 commit into
Open
david-mollitor-db wants to merge 1 commit into
david-mollitor-db wants to merge 1 commit into
Conversation
…imal CheckOverflow declared `nullable = true` unconditionally, while its sibling MakeDecimal (immediately above it in the same file) already declares the accurate form `child.nullable || nullOnOverflow`. CheckOverflow returns null only when its input is null, or when an overflow occurs with `nullOnOverflow = true` (under ANSI, `nullOnOverflow` is false and an overflow throws). Align CheckOverflow with MakeDecimal: override def nullable: Boolean = child.nullable || nullOnOverflow and, in doGenCode, only re-derive `isNull` from the value when the expression is nullable (mirroring MakeDecimal) -- otherwise nullSafeCodeGen makes `ev.isNull` a literal and the assignment would be invalid. No behavioral change: overflow still throws under ANSI and returns null otherwise. Only the declared nullability tightens, so a CheckOverflow-wrapped decimal over non-null inputs is now reported non-nullable under ANSI. Co-authored-by: Isaac <no-reply@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What changes were proposed in this pull request?
CheckOverflowdeclaredoverride def nullable: Boolean = trueunconditionally. Its siblingMakeDecimal(immediately above it indecimalExpressions.scala) already declares the accurate formchild.nullable || nullOnOverflowand guards its generatedisNullassignment accordingly. This PR alignsCheckOverflowwithMakeDecimal:and, in
doGenCode, only re-derivesisNullfrom the value when the expression is nullable (mirroringMakeDecimal) — otherwisenullSafeCodeGenmakesev.isNulla literal and the assignment would be invalid:Why are the changes needed?
CheckOverflowrounds a decimal to a target precision/scale; on overflow it returnsnullwhennullOnOverflowis true and throws when it is false (ANSI). So the result isnullonly when the input isnull, or whennullOnOverflowis true and an overflow occurs — i.e.child.nullable || nullOnOverflow. Declaring it unconditionally nullable is inaccurate (a decimal expression wrapped inCheckOverflowover non-nullable inputs is reported nullable under ANSI when it can never be null) and inconsistent withMakeDecimal, which already does this.Does this PR introduce any user-facing change?
Schema metadata only: a decimal expression wrapped in
CheckOverflowover non-nullable inputs may now reportnullable = falseunder ANSI, whereas before it reportednullable = true. Computed values and error behavior are unchanged — overflow still throws under ANSI and returnsnullotherwise.How was this patch tested?
Existing suites, all pass with no golden-file changes:
DecimalExpressionSuite(directly exercisesCheckOverflow),ArithmeticExpressionSuiteSQLQueryTestSuite:decimalArithmeticOperations.sql.sql.outgolden files are unaffected because their schema lines usedf.schema.catalogString, which does not encode nullability.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Isaac
This pull request and its description were written by Isaac.