Feat: support bit_count function #1602
Conversation
| val table = "bitwise_count_test" | ||
| withTable(table) { | ||
| sql(s"create table $table(col1 long, col2 int, col3 short, col4 byte) using parquet") | ||
| sql(s"insert into $table values(1111, 2222, 17, 7)") |
There was a problem hiding this comment.
Do you mind adding random number cases?
There was a problem hiding this comment.
Yep. Added tests with random data.
There was a problem hiding this comment.
Can we also add a test with a Parquet file not created by Spark (see makeParquetFileAllTypes) and see if this reports correct results with unsigned int columns?
There was a problem hiding this comment.
CometTestBase.makeParquetFileAllTypes has all the integer types you may want to test.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1602 +/- ##
============================================
+ Coverage 56.12% 59.47% +3.34%
- Complexity 976 1147 +171
============================================
Files 119 128 +9
Lines 11743 12532 +789
Branches 2251 2356 +105
============================================
+ Hits 6591 7453 +862
+ Misses 4012 3889 -123
- Partials 1140 1190 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> { | ||
| let arg = self.arg.evaluate(batch)?; | ||
| match arg { | ||
| ColumnarValue::Array(array) => { |
There was a problem hiding this comment.
Any possibility that we get a dictionary?
There was a problem hiding this comment.
It seems impossible to me, but if you have a case study on how this can be tested, I'm ready to do it.
There was a problem hiding this comment.
You will need to have many repeated values in order to create dictionary values
If you use something like https://github.com/apache/datafusion-comet/blob/main/spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala#L940
you should be able to test it
If this is the case, can we delegate to a |
# Conflicts: # native/spark-expr/src/comet_scalar_funcs.rs
|
@mbutrovich I couldn't find a built-in implementation of bit_count in the DataFusion project, but i rewrote it using scalarFunc without adding a new proto expr. |
| let operand = $OPERAND | ||
| .as_any() | ||
| .downcast_ref::<$DT>() | ||
| .expect("compute_op failed to downcast array"); |
There was a problem hiding this comment.
Perhaps report $DT as well in case of failure?
| val table = "bitwise_count_test" | ||
| withTable(table) { | ||
| sql(s"create table $table(col1 long, col2 int, col3 short, col4 byte) using parquet") | ||
| sql(s"insert into $table values(1111, 2222, 17, 7)") |
There was a problem hiding this comment.
Can we also add a test with a Parquet file not created by Spark (see makeParquetFileAllTypes) and see if this reports correct results with unsigned int columns?
| } | ||
| } | ||
|
|
||
| test("bitwise_count - random values (native parquet gen)") { |
There was a problem hiding this comment.
CometTestBase.makeParquetFileAllTypeshas all the integer types you may want to test.
@parthchandra I've already added the test using makeParquetFileAllTypes, or does it still need to be improved?
There was a problem hiding this comment.
Oh, I must have missed this. This is sufficient, I think. (Thank you!)
| } | ||
| } | ||
|
|
||
| test("bitwise_count - random values (native parquet gen)") { |
There was a problem hiding this comment.
Oh, I must have missed this. This is sufficient, I think. (Thank you!)
|
Thank you for the feedback! @kazuyukitanimura @parthchandra |
|
Merged Thank you @kazantsev-maksim @parthchandra @mbutrovich |
Which issue does this PR close?
Related to Epic: #240
bit_count: SELECT bit_count(0) => 0
DataFusionComet bit_count has same behavior with Spark 's bit_count function
Spark: https://spark.apache.org/docs/latest/api/sql/index.html#bit_count
Closes #.
Rationale for this change
Defined under Epic: #240
What changes are included in this PR?
bitwise_count.rs: impl for bit_count function
planner.rs: Maps Spark 's bit_count function to DataFusionComet bit_count physical expression from Spark physical expression
expr.proto: bit_count has been added,
QueryPlanSerde.scala: bit_count pattern matching case has been added,
CometExpressionSuite.scala: A new UT has been added for bit_count function.
How are these changes tested?
A new UT has been added.