-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-41319][CONNECT][PYTHON] Implement Column.{when, otherwise} and Function when
#38935
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
Conversation
0f0b5c9 to
d210b4c
Compare
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.
[error] /home/runner/work/spark/spark/connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:594:13: type mismatch;
[error] found : scala.collection.mutable.Buffer[(org.apache.spark.sql.catalyst.expressions.Expression, org.apache.spark.sql.catalyst.expressions.Expression)]
[error] required: Seq[(org.apache.spark.sql.catalyst.expressions.Expression, org.apache.spark.sql.catalyst.expressions.Expression)]
[error] .map(b => (transformExpression(b.getCondition), transformExpression(b.getValue))),
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.
guessing you need a .toSeq?
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.
thanks
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.
Why not make this an unresolved_function? All it takes is that we add a constructor to CaseWhen.
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.
Alternatively you can rewrite this into a series of ifs.
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.
hmm for unresolved_function, how to deal with the optional else?
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.
If the the number of expressions is uneven, the last expression is the else expression.
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.
If we use this way then have to document it clear for even/odd assumption on the server side.
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.
Yeah we do. I am just trying to see if we can keep the number of expressions in the proto to a bare minimum. In this case you can get away with it because it sort of makes sense.
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.
I am not sure whether unresolved_function when can support otherwise, let me take a deeper look
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.
Otherwise is the elseValue in CaseWhen.
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.
got it, let me have a try, thanks
d210b4c to
110d7e7
Compare
|
close this one in favor of #38956 |
What changes were proposed in this pull request?
Implement
Column.{when, otherwise}and FunctionwhenWhy are the changes needed?
For API coverage
Does this PR introduce any user-facing change?
yes
How was this patch tested?
added UT