feat(spark): Adds negative spark function #20006
Conversation
SubhamSinghal
changed the title
|
Running CI |
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @SubhamSinghal
Please add .slt tests for negative functions, the simplest case is https://spark.apache.org/docs/latest/api/sql/#negative
However you can generate bunch of other edge cases with LLM
Co-authored-by: Jeffrey Vo <[email protected]>
Co-authored-by: Jeffrey Vo <[email protected]>
Co-authored-by: Jeffrey Vo <[email protected]>
Co-authored-by: Oleks V <[email protected]>
Co-authored-by: Oleks V <[email protected]>
Co-authored-by: Oleks V <[email protected]>
| | DataType::UInt8 | ||
| | DataType::UInt16 | ||
| | DataType::UInt32 | ||
| | DataType::UInt64 => Ok(args[0].clone()), |
There was a problem hiding this comment.
I wonder whether this should be an error instead.
I cannot find how others do it: Spark does not support unsigned types, Postgres/DuckDB do not provide negative() function.
There was a problem hiding this comment.
I don't see a need to throw an error here; it's easier to just allow it
There was a problem hiding this comment.
@Jefffrey this doesn't seem right to me... semantically you're not returning the negative of the number here?
There was a problem hiding this comment.
Yes you're right, I'm sorry; I think my brain was thinking of abs for some reason here 😅
There was a problem hiding this comment.
@xanderbailey thanks for pointing this out. Fixed this now.
Co-authored-by: Martin Grigorov <[email protected]>
Co-authored-by: Martin Grigorov <[email protected]>
Done |
There was a problem hiding this comment.
Thanks @SubhamSinghal IMO the PR is good to go, appreciate the contribution
UPD: CI is failing now for tests, I'll wait for green CI, however as I said before the PR looks good overall
Co-authored-by: Oleks V <[email protected]>
|
@comphead Changes Signature from Signature::numeric to Signature::any to allow IntervalType. Can you run CI again? |
|
@SubhamSinghal some tests are still failing |
|
5 participants
What changes are included in this PR?
Adds support for negative spark function in data fusion.
Are these changes tested?
yes, using UTs
Are there any user-facing changes?
yes, adds new function.