[SPARK-52444][SQL][CONNECT] Add support for Variant/Char/Varchar Literal #51215
Conversation
HyukjinKwon
changed the title
|
ping @hvanhovell to take a look. |
|
|
||
| message Char { | ||
| string value = 1; | ||
| optional int32 length = 2; |
There was a problem hiding this comment.
This is only needed when the length of the value and the intended data type do not match right? If so please document this.
There was a problem hiding this comment.
I think when using Char or Varchar, the length field is better to provide, or String type is preferred. We will use the length field if it's provided and do some validation, or use len(value) as a default when length is omitted. I added these descriptions to the documents.
There was a problem hiding this comment.
I was wondering whether we should just use DataType.Char.
There was a problem hiding this comment.
That's a good idea, using char in DataType will avoid duplication of meta info, but we already have some duplication in Decimal and Time, should we also make some adjustments for them? cc @hvanhovell
There was a problem hiding this comment.
I think it would be nice to make some adjustments for them as well. cc @HyukjinKwon and @zhengruifeng
| @@ -240,6 +243,21 @@ message Expression { | |||
| Strings strings = 6; | |||
| } | |||
| } | |||
|
|
|||
| message Variant { | |||
There was a problem hiding this comment.
Please make sure to provide a reference to the format that is used here.
There was a problem hiding this comment.
I added a reference to Spark's VariantVal, and I searched the docs directory without finding expressions-related docs, so I changed no documents in the docs directory.
dengziming
force-pushed
the
SPARK-52444
branch
2 times, most recently
from
b9cf82b to
dfbdf73
Compare
There was a problem hiding this comment.
@hvanhovell Thank you for your review, I'm sorry that I forgot to submit my reply, and I have resolved your comments, PTAL again.
| @@ -240,6 +243,21 @@ message Expression { | |||
| Strings strings = 6; | |||
| } | |||
| } | |||
|
|
|||
| message Variant { | |||
There was a problem hiding this comment.
I added a reference to Spark's VariantVal, and I searched the docs directory without finding expressions-related docs, so I changed no documents in the docs directory.
|
|
||
| message Char { | ||
| string value = 1; | ||
| optional int32 length = 2; |
There was a problem hiding this comment.
I think when using Char or Varchar, the length field is better to provide, or String type is preferred. We will use the length field if it's provided and do some validation, or use len(value) as a default when length is omitted. I added these descriptions to the documents.
dengziming
force-pushed
the
SPARK-52444
branch
2 times, most recently
from
bc7bdd6 to
bbdbbce
Compare
| // - If omitted, uses the actual length of `value`. | ||
| // - If provided but smaller than `value`'s length, an error will be thrown. | ||
| // - If provided and larger than `value`'s length, stores exact value without padding. | ||
| optional int32 length = 2; |
There was a problem hiding this comment.
I was wondering whether we should just use DataType.VarChar.
dengziming
force-pushed
the
SPARK-52444
branch
2 times, most recently
from
e98cfdb to
2d8db4b
Compare
What changes were proposed in this pull request?
We are currently missing support for a number of DataTypes, add Variant/Char/Varchar in this PR,
Strings with Collation, YearMonthIntervalType with begin/end and DayTimeIntervalType with begin/end require us to change the expression structure and will be done in another PR.
Why are the changes needed?
Align the gap between literal expressions and data types.
Does this PR introduce any user-facing change?
Yes.
fun.lit()can pass a VariantVal.How was this patch tested?
fun.litandfun.typedLit.Was this patch authored or co-authored using generative AI tooling?
No