chore: fix spark ansi sum incompatibility message#4111
Conversation
coderfender
force-pushed
the
fix_comet_sum_compatbility_level
branch
from
0d481f4 to
2fdb835
Compare
coderfender
left a comment
coderfender
left a comment
There was a problem hiding this comment.
added some comments to help with review
| object CometStringRepeat extends CometExpressionSerde[StringRepeat] { | ||
|
|
||
| override def getCompatibleNotes(): Seq[String] = Seq( | ||
| override def getIncompatibleReasons(): Seq[String] = Seq( |
There was a problem hiding this comment.
getIncompatibleReasons needs to be removed now that this issue has been fixed in #4017
There was a problem hiding this comment.
Actually that was the wrong issue ... why does this PR rename this method? There is no getSupportLevel so this expression is not marked as incompatible
There was a problem hiding this comment.
True ! Thank you for pointing this out. I fixed it in the latest commit
There was a problem hiding this comment.
some comments got lost I think due to GitHub outages ... I don't understand why this PR is removing getCompatibleNotes and adding getIncompatibleReasons
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
I want to make sure this PR does not get merged accidentally before feedback is addressed
| query | ||
| SELECT repeat('hi', 3), repeat('', 5), repeat('a', 0), repeat(NULL, 3) | ||
|
|
||
| -- non-positive literal count |
There was a problem hiding this comment.
can you add non-literal test as well - that does not seem to be covered in the code change
|
@coderfender This PR has the title |
| Literal.create(null, StringType), | ||
| Literal(UTF8String.EMPTY_UTF8, StringType)) | ||
| return exprToProtoInternal(ifExpr, inputs, binding) | ||
| case _ => |
There was a problem hiding this comment.
we could consider just falling back to Spark if the second arg is not a literal
|
Seems like negative repeat edge case is fixed in DF v53 ? https://github.com/apache/datafusion/blame/4a41173ba3df9b5d47638599c819a1e6e46ad92b/datafusion/functions/src/string/repeat.rs#L165 |
2 participants
Which issue does this PR close?
Closes #4074
CometSum: removed the misleading ANSI-mode incompatibility message (since we do support ANSI mode)Rationale for this change
What changes are included in this PR?
How are these changes tested?