Allow default value on lag/lead if they are coerceable to value #8308
Conversation
github-actions
bot
added
physical-expr
| || (dtype.is_integer() && default_value_type.is_integer()) | ||
| { | ||
|
|
||
| if comparison_coercion(&default_value_type, dtype).is_some() { |
There was a problem hiding this comment.
It seems that you just need to change integer, since I'm not sure having pub fn comparison_binary_numeric_coercion is a good idea or not. At least, we can add the comment for now.
There was a problem hiding this comment.
Sorry, I didn't understand what you mean by "change integer". Could you rephrase, or provide a suggestion for a change?
There was a problem hiding this comment.
Sorry, I mean coerce, not change.
There was a problem hiding this comment.
Code from your previous commit
if can_coerce_from(dtype, &default_value_type)
|| (dtype.is_integer() && default_value_type.is_integer())Comment similar to
// We only check coercion between integer.
There was a problem hiding this comment.
So just to be clear, are you suggesting that the comparison should be reverted to
if can_coerce_from(dtype, &default_value_type)
// We explicitely allow a lossy conversion between signed and unsigned integers
|| (dtype.is_integer() && default_value_type.is_integer())?
There was a problem hiding this comment.
I suggest comparison_coercion. I read the doc, it does not seem lag/lead should only be integer.
There was a problem hiding this comment.
Note that coerced_type might not be the same of dtype or default_value_type. So we need to cast both of them to coerced_type
if let(coerced_type) = comparison_coercion(dtype, &default_value_type) {}
For example, if we have (dtype: UInt64, default_value_type: Int32), coerced_type is Int64
There was a problem hiding this comment.
cc @alamb to confirm my suggestion.
There was a problem hiding this comment.
I think coercing during physical evaluation is too late in the process -- most other coercion happens in the analyze phase so that subsequent transformations / optimizations use the same rules.
Would it be possible to move this coercion earlier into
?
| || (dtype.is_integer() && default_value_type.is_integer()) | ||
| { | ||
|
|
||
| if comparison_coercion(&default_value_type, dtype).is_some() { |
There was a problem hiding this comment.
I think coercing during physical evaluation is too late in the process -- most other coercion happens in the analyze phase so that subsequent transformations / optimizations use the same rules.
Would it be possible to move this coercion earlier into
?
|
(p.s sorry for the late review) |
|
Marking as draft as i think this is no longer waiting on feedback. Please mark it ready for review if/when it is ready for another look |
Which issue does this PR close?
Closes #8307.
Rationale for this change
Allow other default values than Int64 on lag/lead window functions.
What changes are included in this PR?
If a default value is specified, it is now coerced if possible.
Before this PR only Int64 was allowed.
Are these changes tested?
Yes, added some slt tests.
Are there any user-facing changes?
Yes, more default values are now accepted.