Use storageAssighmentPolicy for casts in Delta DML commands #1938
Conversation
This file contains 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
c27kwan
reviewed
Jul 26, 2023
spark/src/test/scala/org/apache/spark/sql/delta/ImplicitDMLCastingSuite.scala
Outdated
Show resolved
Hide resolved
spark/src/test/scala/org/apache/spark/sql/delta/ImplicitDMLCastingSuite.scala
Outdated
Show resolved
Hide resolved
spark/src/test/scala/org/apache/spark/sql/delta/ImplicitDMLCastingSuite.scala
Outdated
Show resolved
Hide resolved
fred-db
reviewed
Jul 26, 2023
| } | ||
| } | ||
|
|
||
| case class CheckOverflowInTableWrite(child: Expression, columnName: String) |
There was a problem hiding this comment.
Seems to be almost the same as CheckOverflowInTableInsert in Spark, could we inherit it?
There was a problem hiding this comment.
I was indeed looking into it, but there are some problems with that
- This case is a case class and CheckOverflowInTableInsert is one as well, so Scala does not like that
- I cannot refactor CheckOverflowInTableInsert because it is part of the OSS Spark imports. This also inhibits me from making methods like getCast protected or the error throwing function configurable
- Even if I could overwrite methods, with doGenCodeWithBetterErrorMsg and eval I would have to overwrite the main ones so most of the code would not be shared (this is more of a minor thing though)
|
Oh wow! We were letting overflows happen silently. This is bad. So glad you caught it and fixed it. This is definitely a tiny bit breaking change...but for the better. I really think we should get this into Delta 3.0 since major releases are good opportunities to do such breaking changes. |
tdas
approved these changes
Jul 26, 2023
5 tasks
5 tasks
vkorukanti
pushed a commit
to vkorukanti/delta
that referenced
this pull request
Sep 6, 2023
delta-io#1938 changed the casting behavior in MERGE and UPDATE to follow the value of the `storeAssignmentPolicy` config instead of the `ansiEnabled` one, making the behavior consistent with INSERT. This change breaks MERGE and UPDATE operations that contain a cast when `storeAssignmentPolicy` is set to `STRICT`, throwing an internal error during analysis. The cause is the `UpCast` expression(s) added by `PreprocessTableMerge` and `PreprocessTableUpdate` when processing assignments. `UpCast` is meant to be replaced by a regular after performing checks by the `ResolveUpCast` rule that runs during the resolution phase **before** `PreprocessTableMerge` and `PreprocessTableUpdate` introduce the expression. The fix is to run the `ResolveUpCast` rule once more after `PreprocessTableMerge` and `PreprocessTableUpdate` have run. Missing tests covering cast behavior for the different values of`storeAssignmentPolicy` for UPDATE and MERGE are added, covering: - Invalid implicit cast (string -> boolean), valid implicit cast (string -> int), upcast (int -> long) - UPDATE, MERGE - storeAssignmentPolicy = LEGACY, ANSI, STRICT Closes delta-io#1998 GitOrigin-RevId: 5b18006b9cb8efa522cda0a4cfa6e981a0b40d28
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
Which Delta project/connector is this regarding?
Description
Follow spark.sql.storeAssignmentPolicy instead of spark.sql.ansi.enabled for casting behaviour in Delta UPDATE and MERGE. This will by default error out at runtime when an overflow happens because the default for storeAssignmentPolicy ins ANSI while ANSI is not enabled by default.
How was this patch tested?
Tests for casting behavior added
Does this PR introduce any user-facing changes?
The UPDATE and MERGE command will error out by default if a cast that is on the write path overflows. The spark.databricks.delta.updateMergeLegacyCasting.enabled can be used to revert to the old behaviour.