Deduplicate Spark function code with native/default datafusion function code

[Jefffrey]

Is your feature request related to a problem or challenge?

Some Spark functions have equivalent DataFusion functions, differing slightly in behaviour. Work towards reducing duplicated code, calling out any differences between the functions when we can't merge them together.

Functions with Spark on left, DF (closest) equivalent on right:

• length -> character_length
• like -> like refactor: refactor spark like function to use datafusion like #19324
  • Not much to deduplicate
• ilike -> ilike
• char -> chr
• ascii -> ascii
  • Done by minor: refactor Spark ascii function to reuse DataFusion ascii function code #17965, though we could explore further deduplication
• rint -> round (?)
• mod/pmod -> modulo (?)
• hex -> to_hex (?)
• factorial -> factorial
• expm1 -> exp
  • Won't do, see Deduplicate Spark function code with native/default datafusion function code #17964 (comment)
• map_from_arrays -> map (?)
• sha1/sha2 -> sha* - @Jefffrey planning to look into this
• avg -> avg
• array -> make_array chore: merge make_array and spark array #19006

Not all of the above are exact equivalents, they're just listed to make it easier to check if we can reduce duplication to some degree. I've listed the Spark functions I noticed that seem to have something similar in DataFusion already.

[rawwar]

I would like to work on this.

[Jefffrey]

I would like to work on this.

Cheers, feel free to pick a specific function to work on (I think it would be good to have a PR per function instead of one big PR for all)

[rawwar]

As the issue has multiple, I would like to first pick 5 tasks out of them and start working. Will raise a PR per task. If no one else picked others by the time I finish the 5, i'll pick the rest of them.

• char -> chr
• factorial -> factorial
• hex -> to_hex
• length -> character_length
• mod/pmod -> modulo

[jizezhang]

Hi @Jefffrey I was looking at some of the pairs and and have some questions:

• For rint -> round, from what I understand, rint uses float round_ties_even while round uses float round, which would produce difference results for floats half-way between integers. Is that intended, or what was the reason?
• For array -> make_array,
  • I did not seem to find unit tests in datafusion/functions-nested/src/make_array.rs or datafusion/spark/src/functions/spark_array.rs. Are tests kept in other files?
  • For coalesce_types, make_array uses type_union_resolution while spark_array uses comparison_coersion (with folding), both of which are implemented in datafusion_expr_common::type_coercion::binary. It seems to me that they are very similar for the purpose of coalescing types and make_array can also use comparison_coersion, but wanted to check in case I misunderstood anything. Also are there unit tests for type_union_resolution? did not seem to find under type_coercion::binary module.
  • make_array uses field name LIST_FIELD_DEFAULT_NAME (which is "item") while spark_array uses field name ARRAY_FIELD_DEFAULT_NAME (which is "element"). Are they intended to be different?
  • make_array does not handle case for DataType::LargeList while spark_array does. Is there a reason for that?

Thanks a lot!

[Jefffrey]

For rint -> round, from what I understand, rint uses float round_ties_even while round uses float round, which would produce difference results for floats half-way between integers. Is that intended, or what was the reason?

This is indeed the intention of the ticket; to find out these differences and ensure they are either eliminated if there is no reason for them to be different, or to document these differences so in future we know that they are meant to be different. In this case it's worth going through the commit/PR history to see if there are reasons given, otherwise falling back to testing against Spark to confirm if these are meant to be different.

I did not seem to find unit tests in datafusion/functions-nested/src/make_array.rs or datafusion/spark/src/functions/spark_array.rs. Are tests kept in other files?

We prefer keeping our tests as sqllogictests, see: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest

For coalesce_types, make_array uses type_union_resolution while spark_array uses comparison_coersion (with folding), both of which are implemented in datafusion_expr_common::type_coercion::binary. It seems to me that they are very similar for the purpose of coalescing types and make_array can also use comparison_coersion, but wanted to check in case I misunderstood anything. Also are there unit tests for type_union_resolution? did not seem to find under type_coercion::binary module.

Unfortunately I'm not sure of this myself.

make_array uses field name LIST_FIELD_DEFAULT_NAME (which is "item") while spark_array uses field name ARRAY_FIELD_DEFAULT_NAME (which is "element"). Are they intended to be different?

I'm inclined to believe they are meant to be different, but some investigation & accompanying documentation would be very helpful for us in the future to confirm the fact.

make_array does not handle case for DataType::LargeList while spark_array does. Is there a reason for that?

I actually think you may have spotted a bug here 🤔

The spark_array implementation does seem to suggest it may return a LargeList for certain inputs, but this conflicts with the stated return type which should always return a List (not a LargeList):

datafusion/datafusion/spark/src/function/array/spark_array.rs

Lines 75 to 108 in 9238779

	fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
	let mut expr_type = DataType::Null;
	for arg_type in arg_types {
	if !arg_type.equals_datatype(&DataType::Null) {
	expr_type = arg_type.clone();
	break;
	}
	}
	if expr_type.is_null() {
	expr_type = DataType::Int32;
	}
	Ok(DataType::List(Arc::new(Field::new(
	ARRAY_FIELD_DEFAULT_NAME,
	expr_type,
	true,
	))))
	}
	fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> {
	let data_types = args
	.arg_fields
	.iter()
	.map(|f| f.data_type())
	.cloned()
	.collect::<Vec<_>>();
	let return_type = self.return_type(&data_types)?;
	Ok(Arc::new(Field::new(
	"this_field_name_is_irrelevant",
	return_type,
	false,
	)))
	}

• Side note: it's also confusing how both return_type and return_field_from_args are implemented since usually it's one or the other, so could do with some refactoring here

Might be worth mocking an example test which could exhibit this bug (where it returns a LargeList in spite of it stating its return type as regular List)

See the tests for the spark version of array function here: https://github.com/apache/datafusion/blob/9238779f45418e07108079aeef3b51de85e9fb8f/datafusion/sqllogictest/test_files/spark/array/array.slt

And use this as reference for how we can construct a LargeList via SLT tests:

datafusion/datafusion/sqllogictest/test_files/array.slt

Lines 7946 to 7949 in 9238779

	query B
	select list_empty(arrow_cast(make_array(1), 'LargeList(Int64)'));
	----
	false

@jizezhang

[jizezhang]

@Jefffrey Thanks a lot for the detailed response!

For array I'll start with what you suggested:

Might be worth mocking an example test which could exhibit this bug (where it returns a LargeList in spite of it stating its return type as regular List)

For rint/round, I think this is the PR for rint and this is the initial PR for round. Neither seems to state the reason for choice of rounding function, though the commit for rint did include tests for floats half-way between integers while the commit forround did not.

[Jefffrey]

@Jefffrey Thanks a lot for the detailed response!

For array I'll start with what you suggested:

Might be worth mocking an example test which could exhibit this bug (where it returns a LargeList in spite of it stating its return type as regular List)

For rint/round, I think this is the PR for rint and this is the initial PR for round. Neither seems to state the reason for choice of rounding function, though the commit for rint did include tests for floats half-way between integers while the commit forround did not.

For rint it sounds like best approach is just to leave a comment explaining this is the intended functionality, calling out the difference in using round & round_ties_even so we have concrete reference