-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-46949][SQL] Support CHAR/VARCHAR through ResolveDefaultColumns #44991
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM (from my side)
Thanks you very much @dongjoon-hyun |
|CREATE TABLE t( | ||
| key int, | ||
| v VARCHAR(6) DEFAULT 'apache', | ||
| c CHAR(5) DEFAULT 'spark') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we do char padding for default values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does padding only get involved with the read path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For safety reasons, I added a new test at L245.
I created #44993 to resolve the test failure, separating it as it might get a chance for 3.5.
|
This comment was marked as spam.
This comment was marked as spam.
Thank you @dongjoon-hyun @HyukjinKwon @cloud-fan , merged to master |
What changes were proposed in this pull request?
We have issues resolving column definitions with default values, i.e.,
c CHAR(5) DEFAULT 'foo'
,v VARCHAR(10) DEFAULT 'bar'
. The reason is that CAHR/VARCHAR types in schemas are not supplanted with STRING type to align with the value expression.This PR fixes these issues in
ResolveDefaultColumns
, which seems to only cover the v1 tables. When I applied some related tests to v2 tables, they had the same issues. But beyond that, there are some other front-loading needs to be addressed. In this case, I'd like to separate v2 from the v1 PR.Why are the changes needed?
bugfix
Does this PR introduce any user-facing change?
no, bugfix
How was this patch tested?
new tests
Was this patch authored or co-authored using generative AI tooling?
no