-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support LargeUtf8 to Temporal Coercion #8357
Conversation
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
} | ||
|
||
match_rule(lhs_type, rhs_type).or_else(|| match_rule(rhs_type, lhs_type)) |
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.
without this it will be too hard to check if the code cover one type.
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.
This is a neat construction 👍
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.
Very cool @jayzhan211 -- thank you for the contribution
} | ||
|
||
match_rule(lhs_type, rhs_type).or_else(|| match_rule(rhs_type, lhs_type)) |
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.
This is a neat construction 👍
Which issue does this PR close?
Ref #8302
Rationale for this change
There are two reason why I want to support this coercion.
One is that
LargeUtf8
toTimestamp
is supported incoerced_from
. We need the same so I can replace it withstring_temporal_coercion
Second is that comment in
string_temporal_coercion
mentionedLargeUtf8
but the code itself does not.What changes are included in this PR?
Are these changes tested?
I didnt find coercion test in slt. I put it in
scalar.slt
for now.Are there any user-facing changes?