You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rafafrdz (via GitHub)" <gi...@apache.org> on 2026/02/12 11:37:29 UTC

[PR] feat: add support `crc32` expression [datafusion-comet]

rafafrdz opened a new pull request, #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498

   **Summary**
   - Added support for Spark `crc32` by mapping `Crc32` to the native scalar function `crc32` in expression serde.
   - Extended hash-function coverage in `CometExpressionSuite` to include `crc32` on string and casted numeric inputs.
   - Updated spark_expressions_support.md to mark `crc32` as supported.
   
   **Why**
   This closes one of the missing DataFusion 50 migration functions from issue #2443 and reduces fallback to Spark for crc32 queries.
   
   Part of #2443 and #240


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "rafafrdz (via GitHub)" <gi...@apache.org>.
rafafrdz commented on PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#issuecomment-3896074509

   @andygrove Sorry I missed push the commit where I register the udf... 馃槄
   Also I built and tested locally onto the matrix. For that, I'm using `mise` to handle the different version of java and scala. And I'm using the following command:
   
   `./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite <test>" -Pspark-<spark_version> -Pscala-<scala_version>`
   
   for example:
   
   ./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite crc32" -Pspark-4.0 -Pscala-2.13
   ./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite crc32" -Pspark-3.5 -Pscala-2.13
   ./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite crc32" -Pspark-3.5 -Pscala-2.12
   
   and changing the java version: `mise use java@corretto-17` or `mise use java@corretto-11`
   
   Please, let me know if I'm missing something or doing something wrong. And apologize for the inconveniences


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "rafafrdz (via GitHub)" <gi...@apache.org>.
rafafrdz commented on code in PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#discussion_r2824666817


##########
spark/src/test/resources/sql-tests/expressions/hash/crc32.sql:
##########
@@ -0,0 +1,32 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
+-- crc32 function
+statement
+CREATE TABLE test(col string, a int, b float) USING parquet
+
+statement
+INSERT INTO test VALUES ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999), ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999)
+
+query
+SELECT crc32(col), crc32(cast(a as string)), crc32(cast(b as string)) FROM test
+
+-- literal arguments
+query ignore(https://github.com/apache/datafusion-comet/issues/3340)

Review Comment:
   sorry @andygrove my mistake. It was copy-paste. I deleted it and it still pass the sql test file



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#discussion_r2804721227


##########
spark/src/test/resources/sql-tests/expressions/hash/crc32.sql:
##########
@@ -0,0 +1,31 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
+-- crc32 function
+statement
+CREATE TABLE test(col string, a int, b float) USING parquet
+
+statement
+INSERT INTO test VALUES ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999), ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999)
+
+query
+SELECT crc32(cast(a as string)), crc32(cast(b as string)) FROM test

Review Comment:
   Should there also be a test for `crc32(col)`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#issuecomment-3891775129

   > Thanks for adding crc32 support @rafafrdz!
   > 
   > I think `SparkCrc32` from `datafusion-spark` needs to be registered in the session context. If you look at `register_datafusion_spark_function()` in `native/core/src/execution/jni_api.rs`, you'll see that `SparkSha1` and `SparkSha2` are both explicitly imported and registered there, but `SparkCrc32` isn't yet. Without that, the fallback `registry.udf("crc32")` lookup won't find the function. Something like this should do it:
   > 
   > ```rust
   > use datafusion_spark::function::hash::crc32::SparkCrc32;
   > // ...
   > session_ctx.register_udf(ScalarUDF::new_from_impl(SparkCrc32::default()));
   > ```
   > 
   > For the tests, Comet now has a SQL file-based testing framework (CometSqlFileTestSuite) that's preferred for expression tests. Could you add `crc32.sql` there, following instructions in https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html?
   
   Tests currently fail because the function isn't registered, as expected. @rafafrdz how did you test this locally?
   
   ```
   - hash functions *** FAILED *** (386 milliseconds)
     org.apache.spark.SparkException: Job aborted due to stage failure: Task 2 in stage 3753.0 failed 1 times, most recent failure: Lost task 2.0 in stage 3753.0 (TID 12906) (localhost executor driver): org.apache.comet.CometNativeException: Error from DataFusion: There is no UDF named "crc32" in the registry. Use session context `register_udf` function to register a custom UDF.
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#discussion_r2804716262


##########
spark/src/test/resources/sql-tests/expressions/hash/crc32.sql:
##########
@@ -0,0 +1,31 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
+-- crc32 function
+statement
+CREATE TABLE test(col string, a int, b float) USING parquet
+
+statement
+INSERT INTO test VALUES ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999), ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999)
+
+query
+SELECT crc32(cast(a as string)), crc32(cast(b as string)) FROM test
+
+-- literal arguments                                                                                                                                                                                                                                                                                                                                                                                                                                                                               query ignore(https://github.com/apache/datafusion-comet/issues/3340)

Review Comment:
   
   It looks like a newline is missing here?
   
   ```suggestion
   -- literal arguments
   query ignore(https://github.com/apache/datafusion-comet/issues/3340)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#discussion_r2820332242


##########
spark/src/test/resources/sql-tests/expressions/hash/crc32.sql:
##########
@@ -0,0 +1,32 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
+-- crc32 function
+statement
+CREATE TABLE test(col string, a int, b float) USING parquet
+
+statement
+INSERT INTO test VALUES ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999), ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999)
+
+query
+SELECT crc32(col), crc32(cast(a as string)), crc32(cast(b as string)) FROM test
+
+-- literal arguments
+query ignore(https://github.com/apache/datafusion-comet/issues/3340)

Review Comment:
   This issue is for `sha2`. Is there a separate issue for `crc32` with literal arguments that should be linked to here?
   
   What happens if you remove the ignore from this test?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#issuecomment-3891681870

   Thanks for adding crc32 support @rafafrdz!
                                                                                                                                                                                                             
   I think `SparkCrc32` from `datafusion-spark` needs to be registered in the session context. If you look at `register_datafusion_spark_function()` in `native/core/src/execution/jni_api.rs`, you'll see that `SparkSha1` and `SparkSha2` are both explicitly imported and registered there, but `SparkCrc32` isn't yet. Without that, the fallback `registry.udf("crc32")` lookup won't find the function. Something like this should do it:
   
   ```rust
   use datafusion_spark::function::hash::crc32::SparkCrc32;
   // ...
   session_ctx.register_udf(ScalarUDF::new_from_impl(SparkCrc32::default()));
   ```
   
   For the tests, Comet now has a SQL file-based testing framework (CometSqlFileTestSuite) that's preferred for expression tests. Could you add `crc32.sql` there, following instructions in https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add support `crc32` expression [datafusion-comet]

Posted by "rafafrdz (via GitHub)" <gi...@apache.org>.
rafafrdz commented on code in PR #3498:
URL: https://github.com/apache/datafusion-comet/pull/3498#discussion_r2817886324


##########
spark/src/test/resources/sql-tests/expressions/hash/crc32.sql:
##########
@@ -0,0 +1,31 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
+-- crc32 function
+statement
+CREATE TABLE test(col string, a int, b float) USING parquet
+
+statement
+INSERT INTO test VALUES ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999), ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999)
+
+query
+SELECT crc32(cast(a as string)), crc32(cast(b as string)) FROM test

Review Comment:
   done!



##########
spark/src/test/resources/sql-tests/expressions/hash/crc32.sql:
##########
@@ -0,0 +1,31 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
+-- crc32 function
+statement
+CREATE TABLE test(col string, a int, b float) USING parquet
+
+statement
+INSERT INTO test VALUES ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999), ('Spark SQL  ', 10, 1.2), (NULL, NULL, NULL), ('', 0, 0.0), ('鑻规灉鎵嬫満', NULL, 3.999999)
+
+query
+SELECT crc32(cast(a as string)), crc32(cast(b as string)) FROM test
+
+-- literal arguments                                                                                                                                                                                                                                                                                                                                                                                                                                                                               query ignore(https://github.com/apache/datafusion-comet/issues/3340)

Review Comment:
   done!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org