refactor: use TypeSignature::Coercible for crypto functions #14826

Chen-Yuan-Lai · 2025-02-22T15:08:12Z

Which issue does this PR close?

Closes TypeSignature::Coercible for crypto functions #14762 .

Rationale for this change

What changes are included in this PR?

Use TypeSignature::Coercible and logical type (string & binary) for crypto functions
Add DataType::BinaryView for utf8_or_binary_to_binary_type to support binaryView
Make digest_binary_array can digest binary and binaryView
Implement digest_binary_array_impl, which can be used by digest_binary_array
Make digest_process to support binaryView

Are these changes tested?

Are there any user-facing changes?

Chen-Yuan-Lai · 2025-02-24T17:05:16Z

Hi @jayzhan211, I encountered some CI failures in the digest function. It seems the Exact(vec![Utf8View, Utf8View]) case can't be replaced with TypeSignature::Coercible because TypeSignature::Coercible processes each parameter independently, without considering the types of other parameters

[SQL] EXPLAIN SELECT
  digest(column1_utf8view, 'md5') as c
FROM test;
[Diff] (-expected|+actual)
    logical_plan
-   01)Projection: digest(test.column1_utf8view, Utf8View("md5")) AS c
+   01)Projection: digest(test.column1_utf8view, Utf8("md5")) AS c
    02)--TableScan: test projection=[column1_utf8view]

Should we:

Implement a new TypeSignature::Coercible method to support this feature?
Mixing the usage TypeSignature::Coercible and Exact(vec![Utf8View, Utf8View]) in digest function?

I'm not sure which one is better, could you give me some suggestions or other best practices? thanks very much!!

jayzhan211 · 2025-02-25T01:03:31Z

[SQL] EXPLAIN SELECT
  digest(column1_utf8view, 'md5') as c
FROM test;
[Diff] (-expected|+actual)
    logical_plan
-   01)Projection: digest(test.column1_utf8view, Utf8View("md5")) AS c
+   01)Projection: digest(test.column1_utf8view, Utf8("md5")) AS c
    02)--TableScan: test projection=[column1_utf8view]

This looks like an improvement, we don't need to cast it to utf8view

alamb · 2025-03-03T21:31:07Z

CI appears to be failing, so marking this one as a draft

Chen-Yuan-Lai · 2025-03-09T04:13:51Z

Hi @jayzhan211, It seems all the CI checks were passed (including sqlogicaltest), but when I created and printed a table by datafusion-cli , I got empty result

DataFusion CLI v45.0.0
> CREATE TABLE t_source(
    column1 String,
    column2 String,
    column3 String,
    column4 String
) AS VALUES
('one', 'one', 'one', 'one'),
('two', 'two', '', 'two'),
(NULL, NULL, NULL, NULL),
('', '', '', ''),
('three', 'three', 'three', '');

0 row(s) fetched. 
Elapsed 0.005 seconds.

> select * from t_source;
+---------+---------+---------+---------+
| column1 | column2 | column3 | column4 |
+---------+---------+---------+---------+
+---------+---------+---------+---------+
5 row(s) fetched. 
Elapsed 0.001 seconds.

That is weird because I only modified the implementation of crypto functions. Why is the display for the basic operation is changed? Could you give me some help or suggestions? Thx!

Chen-Yuan-Lai · 2025-03-09T14:58:33Z

Hi @jayzhan211, It seems all the CI checks were passed (including sqlogicaltest), but when I created and printed a table by datafusion-cli , I got empty result

Oh! I found the problem has solved after I reinstalled datafusion-cli 😆

datafusion/functions/src/crypto/basic.rs

Chen-Yuan-Lai · 2025-03-09T15:15:07Z

datafusion/functions/src/crypto/basic.rs

+    pub fn digest_binary_array_impl<'a, BinaryArrType>(
+        self,
+        input_value: BinaryArrType,
+    ) -> ArrayRef
+    where
+        BinaryArrType: BinaryArrayType<'a>,
+    {
+        match self {
+            Self::Md5 => digest_to_array!(Md5, input_value),
+            Self::Sha224 => digest_to_array!(Sha224, input_value),
+            Self::Sha256 => digest_to_array!(Sha256, input_value),
+            Self::Sha384 => digest_to_array!(Sha384, input_value),
+            Self::Sha512 => digest_to_array!(Sha512, input_value),
+            Self::Blake2b => digest_to_array!(Blake2b512, input_value),
+            Self::Blake2s => digest_to_array!(Blake2s256, input_value),
+            Self::Blake3 => {
+                let binary_array: BinaryArray = input_value
+                    .iter()
+                    .map(|opt| {
+                        opt.map(|x| {
+                            let mut digest = Blake3::default();
+                            digest.update(x);
+                            Blake3::finalize(&digest).as_bytes().to_vec()
+                        })
+                    })
+                    .collect();
+                Arc::new(binary_array)
+            }
+        }
+    }


I found that digest_binary_array_impl and digest_utf8_array_impl are similar, should we integrate them into one?

It would be nice to

I found that these two array types don't share a common trait that would allow implementing a single generic function. Since the key difference is in how bytes are accessed, I'll keep these functions separate to maintain type safety.

jayzhan211 · 2025-03-10T01:40:25Z

datafusion/functions/src/crypto/sha512.rs

-                vec![Utf8View, Utf8, LargeUtf8, Binary, LargeBinary],
+            signature: Signature::one_of(
+                vec![
+                    TypeSignature::Coercible(vec![Coercion::new_exact(


the return type is utf8_or_binary_to_binary_type
which means the signtaure of sha512 should be new_implicit(binary, string|binary)

Target is binary, and allowed source types are string and binary

I met another error after I used new_implicit 😭

External error: query failed: DataFusion error: Arrow error: Compute error: Internal Error: Cannot cast BinaryView to BinaryArray of expected type [SQL] select sha512(ascii_1) from test_basic_operator; at test_files/string/./string_query.slt.part:1732 at test_files/string/dictionary_utf8.slt:58

I think the error came from arrow-rs
https://github.com/apache/arrow-rs/blob/5b04ca7bf756c5ca9edfba220060017684e2ecc6/arrow-cast/src/cast/dictionary.rs#L107-#L128

BinaryView => { // `unpack_dictionary` can handle Utf8View/BinaryView types, but incurs unnecessary data copy of the value buffer. // we handle it here to avoid the copy. let dict_array = array .as_dictionary::<K>() .downcast_dict::<BinaryArray>() .ok_or_else(|| { ArrowError::ComputeError( "Internal Error: Cannot cast BinaryView to BinaryArray of expected type" .to_string(), ) })?; let binary_view = view_from_dict_values::<K, BinaryViewType, BinaryType>( dict_array.values(), dict_array.keys(), )?; Ok(Arc::new(binary_view)) } _ => unpack_di

When the dictionary is coverted to BinaryView, it can't be cast to BinaryArray

Chen-Yuan-Lai · 2025-03-26T09:34:53Z

@jayzhan211 I think the PR is ready to be reviewed : )

jayzhan211 · 2025-03-26T11:27:54Z

you can mark it as ready for review

jayzhan211 · 2025-03-26T11:39:31Z

datafusion/sqllogictest/test_files/string/string_view.slt

@@ -1068,7 +1068,7 @@ EXPLAIN SELECT
 FROM test;
 ----
 logical_plan
-01)Projection: digest(test.column1_utf8view, Utf8View("md5")) AS c
+01)Projection: digest(test.column1_utf8view, Utf8("md5")) AS c


we avoid casting from utf8 to utf8view

jayzhan211 · 2025-03-26T11:39:56Z

datafusion/sqllogictest/test_files/array.slt

 select count(*) from test WHERE array_has([needle], needle);
 ----
 100000

 # TODO: this should probably be possible to completely remove the filter as always true?
 query TT
-explain with test AS (SELECT substr(md5(i)::text, 1, 32) as needle FROM generate_series(1, 100000) t(i))
+explain with test AS (SELECT substr(md5(i::text)::text, 1, 32) as needle FROM generate_series(1, 100000) t(i))


md5(int64) is not supported, so the test is casted to string explicitly

jayzhan211 · 2025-03-27T09:44:27Z

Thanks @Chen-Yuan-Lai

…4826) * refactor: use TypeSignature::Coercible for crypto functions * fix * fix * fix string_view.slt * fix signatrue for sha256 * remove unsed import * support binaryView * modify signature if sha256 function * remove unsed import * clean unused codes * rewrite function using new_implicit for sha512 * remove unused import * rewrite signature with new_implicit for other crypto functions * support null for md5 function * modify sqllogictest to fit allowed input type for md5 function --------- Co-authored-by: Cheng-Yuan-Lai <a186235@g,ail.com> Co-authored-by: Ian Lai <[email protected]>

github-actions bot added functions Changes to functions implementation common Related to common crate labels Feb 22, 2025

Chen-Yuan-Lai force-pushed the use_coercible_for_crypto branch 3 times, most recently from 3cb1d36 to 2dcee88 Compare February 23, 2025 16:25

github-actions bot removed the common Related to common crate label Feb 23, 2025

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 25, 2025

alamb marked this pull request as draft March 3, 2025 21:31

Chen-Yuan-Lai force-pushed the use_coercible_for_crypto branch from d3efb81 to 03b40f4 Compare March 6, 2025 15:49

Chen-Yuan-Lai commented Mar 9, 2025

View reviewed changes

datafusion/functions/src/crypto/basic.rs Show resolved Hide resolved

Chen-Yuan-Lai commented Mar 9, 2025

View reviewed changes

jayzhan211 reviewed Mar 10, 2025

View reviewed changes

Chen-Yuan-Lai force-pushed the use_coercible_for_crypto branch 2 times, most recently from 2f5bcfe to 4f1d604 Compare March 11, 2025 03:04

github-actions bot added the logical-expr Logical plan and expressions label Mar 11, 2025

Chen-Yuan-Lai force-pushed the use_coercible_for_crypto branch from 0db2c5b to 38e3e41 Compare March 13, 2025 09:34

github-actions bot removed the logical-expr Logical plan and expressions label Mar 13, 2025

Chen-Yuan-Lai force-pushed the use_coercible_for_crypto branch from 38e3e41 to 6e6db4a Compare March 17, 2025 16:37

Chen-Yuan-Lai force-pushed the use_coercible_for_crypto branch from 6e6db4a to 2861ada Compare March 25, 2025 09:33

Cheng-Yuan-Lai added 6 commits March 26, 2025 08:06

refactor: use TypeSignature::Coercible for crypto functions

b557b7d

fix

01f6bcc

fix

556a8c3

fix string_view.slt

7af8087

fix signatrue for sha256

19d1f05

remove unsed import

771620e

Cheng-Yuan-Lai and others added 9 commits March 26, 2025 08:06

support binaryView

0d28d54

modify signature if sha256 function

cdbafcf

remove unsed import

093bfbf

clean unused codes

fdfabef

rewrite function using new_implicit for sha512

cd63fc6

remove unused import

4dfb4f6

rewrite signature with new_implicit for other crypto functions

c35d30c

support null for md5 function

eeaee64

modify sqllogictest to fit allowed input type for md5 function

c2e0ae8

Chen-Yuan-Lai force-pushed the use_coercible_for_crypto branch from 8faafb0 to c2e0ae8 Compare March 26, 2025 08:51

jayzhan211 marked this pull request as ready for review March 26, 2025 11:39

jayzhan211 approved these changes Mar 26, 2025

View reviewed changes

jayzhan211 merged commit fbf2ffc into apache:main Mar 27, 2025
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use TypeSignature::Coercible for crypto functions #14826

refactor: use TypeSignature::Coercible for crypto functions #14826

Chen-Yuan-Lai commented Feb 22, 2025 •

edited

Loading

Chen-Yuan-Lai commented Feb 24, 2025

jayzhan211 commented Feb 25, 2025 •

edited

Loading

alamb commented Mar 3, 2025

Chen-Yuan-Lai commented Mar 9, 2025 •

edited

Loading

Chen-Yuan-Lai commented Mar 9, 2025 •

edited

Loading

Chen-Yuan-Lai Mar 9, 2025

jayzhan211 Mar 10, 2025

Chen-Yuan-Lai Mar 26, 2025

jayzhan211 Mar 10, 2025 •

edited

Loading

Chen-Yuan-Lai Mar 11, 2025 •

edited

Loading

Chen-Yuan-Lai commented Mar 26, 2025 •

edited

Loading

jayzhan211 commented Mar 26, 2025 •

edited

Loading

jayzhan211 Mar 26, 2025

jayzhan211 Mar 26, 2025

jayzhan211 commented Mar 27, 2025

refactor: use TypeSignature::Coercible for crypto functions #14826

refactor: use TypeSignature::Coercible for crypto functions #14826

Conversation

Chen-Yuan-Lai commented Feb 22, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Chen-Yuan-Lai commented Feb 24, 2025

jayzhan211 commented Feb 25, 2025 • edited Loading

alamb commented Mar 3, 2025

Chen-Yuan-Lai commented Mar 9, 2025 • edited Loading

Chen-Yuan-Lai commented Mar 9, 2025 • edited Loading

Chen-Yuan-Lai Mar 9, 2025

Choose a reason for hiding this comment

jayzhan211 Mar 10, 2025

Choose a reason for hiding this comment

Chen-Yuan-Lai Mar 26, 2025

Choose a reason for hiding this comment

jayzhan211 Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Chen-Yuan-Lai Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Chen-Yuan-Lai commented Mar 26, 2025 • edited Loading

jayzhan211 commented Mar 26, 2025 • edited Loading

jayzhan211 Mar 26, 2025

Choose a reason for hiding this comment

jayzhan211 Mar 26, 2025

Choose a reason for hiding this comment

jayzhan211 commented Mar 27, 2025

Chen-Yuan-Lai commented Feb 22, 2025 •

edited

Loading

jayzhan211 commented Feb 25, 2025 •

edited

Loading

Chen-Yuan-Lai commented Mar 9, 2025 •

edited

Loading

Chen-Yuan-Lai commented Mar 9, 2025 •

edited

Loading

jayzhan211 Mar 10, 2025 •

edited

Loading

Chen-Yuan-Lai Mar 11, 2025 •

edited

Loading

Chen-Yuan-Lai commented Mar 26, 2025 •

edited

Loading

jayzhan211 commented Mar 26, 2025 •

edited

Loading