feat: support shiftrightunsigned expression#3590
Conversation
Register datafusion-spark's SparkShiftRightUnsigned UDF and add serde mapping in bitwiseExpressions.
andygrove
left a comment
There was a problem hiding this comment.
Nice and clean wiring. The serde change and UDF registration both look correct.
I have a couple of suggestions around the tests. There's already an existing bitwise.sql file at spark/src/test/resources/sql-tests/expressions/bitwise/bitwise.sql that covers shiftright and shiftleft. It would be a natural fit to add shiftrightunsigned tests there instead of in CometBitwiseExpressionSuite. The SQL file framework automatically compares Spark and Comet results, which is the preferred approach for expression tests.
On test coverage, the current tests only use Int values. The existing shiftright tests in CometBitwiseExpressionSuite test both Int and Long types (there's a second block starting around line 62 that creates a long column). It would be good to add Long coverage for shiftrightunsigned too, since Int and Long go through different code paths in the Rust implementation.
Also, the literal tests (shiftrightunsigned(-1, 1) and shiftrightunsigned(NULL, 1)) would benefit from disabling constant folding so they actually exercise the Comet code path rather than being optimized away by Spark. Something like wrapping them in withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> "org.apache.spark.sql.catalyst.optimizer.ConstantFolding"). Or better yet, these would just work naturally in the SQL file framework.
Summary
shiftrightunsignedfrom thedatafusion-sparkcrate (SparkShiftRightUnsigned) to Cometjni_api.rsand add serde mapping viaCometScalarFunctioninQueryPlanSerde.scala