[SPARK-43018][SQL] Fix bug for INSERT commands with timestamp literals #40652
Conversation
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
Contributor
Author
|
Hi @gengliangwang here is the correctness bug fix 🙏 |
sql/core/src/test/scala/org/apache/spark/sql/ResolveDefaultColumnsSuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/ResolveDefaultColumnsSuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveDefaultColumnsSuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDefaultColumns.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDefaultColumns.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDefaultColumns.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveDefaultColumnsSuite.scala
Outdated
Show resolved
Hide resolved
dtenedor
changed the title
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDefaultColumns.scala
Outdated
Show resolved
Hide resolved
gengliangwang
approved these changes
Apr 5, 2023
Member
gengliangwang
left a comment
gengliangwang
left a comment
There was a problem hiding this comment.
LGTM except one minor comment
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDefaultColumns.scala
Show resolved
Hide resolved
gengliangwang
approved these changes
Apr 5, 2023
Member
|
Thanks, merging to master/branch-3.4 |
gengliangwang
pushed a commit
that referenced
this pull request
Apr 6, 2023
### What changes were proposed in this pull request? This PR fixes a correctness bug for INSERT commands with timestamp literals. The bug manifests when: * An INSERT command includes a user-specified column list of fewer columns than the target table. * The provided values include timestamp literals. The bug was that the long integer values stored in the rows to represent these timestamp literals were getting assigned back to `UnresolvedInlineTable` rows without the timestamp type. Then the analyzer inserted an implicit cast from `LongType` to `TimestampType` later, which incorrectly caused the value to change during execution. This PR fixes the bug by propagating the timestamp type directly to the output table instead. ### Why are the changes needed? This PR fixes a correctness bug. ### Does this PR introduce _any_ user-facing change? Yes, this PR fixes a correctness bug. ### How was this patch tested? This PR adds a new unit test suite. Closes #40652 from dtenedor/assign-correct-insert-types. Authored-by: Daniel Tenedorio <[email protected]> Signed-off-by: Gengliang Wang <[email protected]> (cherry picked from commit 9f0bf51) Signed-off-by: Gengliang Wang <[email protected]>
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.sql.catalyst.analysis |
Member
There was a problem hiding this comment.
Hi, @dtenedor and @gengliangwang . I have a question.
Although I understand this test suite provide a test coverage for org.apache.spark.sql.catalyst.analysis.ResolveDefaultColumns, it doesn't mean this test suite is belong to org.apache.spark.sql.catalyst.analysis package. This test suite exists in sql module and alone in this directory
$ tree sql/core/src/test/scala/org/apache/spark/sql/catalyst
sql/core/src/test/scala/org/apache/spark/sql/catalyst
└── analysis
└── ResolveDefaultColumnsSuite.scala
Is this intentional?
Contributor
Author
There was a problem hiding this comment.
Hi @dongjoon-hyun I don't think this is intentional, we could move the ResolveDefaultColumnsSuite to org.apache.spark.sql package. What do you think? If you want me to do this, I can prepare a PR.
Member
There was a problem hiding this comment.
Thank you. I made a PR.
| assert(asLocalRelation(result) == localRelation) | ||
| } | ||
|
|
||
| test("SPARK-43018: INSERT timestamp values into a table with column DEFAULTs") { |
Member
There was a problem hiding this comment.
Especially, this test case tests more than catalyst/analysis.
snmvaughan
pushed a commit
to snmvaughan/spark
that referenced
this pull request
Jun 20, 2023
### What changes were proposed in this pull request? This PR fixes a correctness bug for INSERT commands with timestamp literals. The bug manifests when: * An INSERT command includes a user-specified column list of fewer columns than the target table. * The provided values include timestamp literals. The bug was that the long integer values stored in the rows to represent these timestamp literals were getting assigned back to `UnresolvedInlineTable` rows without the timestamp type. Then the analyzer inserted an implicit cast from `LongType` to `TimestampType` later, which incorrectly caused the value to change during execution. This PR fixes the bug by propagating the timestamp type directly to the output table instead. ### Why are the changes needed? This PR fixes a correctness bug. ### Does this PR introduce _any_ user-facing change? Yes, this PR fixes a correctness bug. ### How was this patch tested? This PR adds a new unit test suite. Closes apache#40652 from dtenedor/assign-correct-insert-types. Authored-by: Daniel Tenedorio <[email protected]> Signed-off-by: Gengliang Wang <[email protected]> (cherry picked from commit 9f0bf51) Signed-off-by: Gengliang Wang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
This PR fixes a correctness bug for INSERT commands with timestamp literals. The bug manifests when:
The bug was that the long integer values stored in the rows to represent these timestamp literals were getting assigned back to
UnresolvedInlineTablerows without the timestamp type. Then the analyzer inserted an implicit cast fromLongTypetoTimestampTypelater, which incorrectly caused the value to change during execution.This PR fixes the bug by propagating the timestamp type directly to the output table instead.
Why are the changes needed?
This PR fixes a correctness bug.
Does this PR introduce any user-facing change?
Yes, this PR fixes a correctness bug.
How was this patch tested?
This PR adds a new unit test suite.