Fix strpos invocation with dictionary and null

[findepi]

In 1b3608d strpos signature was modified to indicate it supports dictionary as input argument, but the invoke method doesn't support them.

• fixes STRPOS doesn't support (Dictionary(Int32, Utf8) as input type #12670

[findepi]

This restores the code before 1b3608d . These are the types actually supported by invoke.

[Omega359]

Can we invert the order? From another function we have this comment:

// Planner attempts coercion to the target type starting with the most preferred candidate.
// For example, given input (Utf8View, Int64), it first tries coercing to (Utf8View, Int64).
// If that fails, it proceeds to (Utf8, Int64).

[findepi]

Good comment. I just restored the code that used to be here.
How function implementor can know what is the preferred order? Shouldn't this rather be engine's responsibility, if its given a choice? Unless of course the preferred candidate is function-specific.

[Omega359]

Great question and I don't have the answer. I just have followed that advice since I first saw it a month or two back. I agree that I would hope the coercing logic could select the most optimal candidate.

[findepi]

This query used to fail also before 1b3608d , i don't know why.

According to

datafusion/datafusion/expr/src/type_coercion/functions.rs

Lines 681 to 682 in 8db30e2

	// Any type can be coerced into strings
	(Utf8 | LargeUtf8, _) => Some(type_into.clone()),

, every type is implicitly coercible into a Utf8

[jayzhan211]

Isn't this function only accept string? Why do we return 1 now

[jayzhan211]

I think it is better to introduce Signature::String for this issue instead of the old approach that iterating all the possible types

[alamb]

Thank you @findepi

I think it is better to introduce Signature::String for this issue instead of the old approach that iterating all the possible types

I agree @jayzhan211 -- However, I also think we could do it as a follow on PR.

One of the challenges will be to clearly document which of the many potential combinations Signature::String can produce (as the function will have to handle all combinations)

[goldmedal]

Thanks @findepi for working this 👍
There are some TODO items related to this issue. Could you help to address them?

• enable the share test
  

  datafusion/datafusion/sqllogictest/test_files/string/string_query.slt.part

  Lines 954 to 955 in ed0949e

  	# TODO: DictionaryString does not support STRPOS. Enable this after fixing the issue
  	# see issue: https://github.com/apache/datafusion/issues/12670

• remove the type-specific tests
  

  datafusion/datafusion/sqllogictest/test_files/string/string.slt

  Line 66 in ed0949e

  # TODO: move it back to `string_query.slt.part` after fixing the issue

  
  

  datafusion/datafusion/sqllogictest/test_files/string/large_string.slt

  Line 75 in ed0949e

  # TODO: move it back to `string_query.slt.part` after fixing the issue

  
  

  datafusion/datafusion/sqllogictest/test_files/string/string_view.slt

  Line 53 in ed0949e

  # TODO: move it back to `string_query.slt.part` after fixing the issue

[alamb]

There are some TODO items related to this issue. Could you help to address them?

I will do this as a follow on PR

Update in #12739

[alamb]

Thank you @findepi and @goldmedal

[findepi]

thanks @alamb for the merge and sorry for not following on @goldmedal 's #12712 (comment) yet.

[alamb]

thanks @alamb for the merge and sorry for not following on @goldmedal 's #12712 (comment) yet.

No worries -- thanks for the PR -- I am just trying to keep the code flowing and figured I would just make a small PR to get this one in

[jayzhan211]

@alamb @findepi Does anyone know why select position(1 in 1) is not failing now? Isn't this a regression? The change of the signature is actually incorrect IMO.

Upd: Can check out #12751 for the fix

[findepi]

Does anyone know why select position(1 in 1) is not failing now?

i called this out in #12712 (comment)

in fact, i don't understand why it was failing before -- #12308 (comment)

In DataFusion everything is coercible into Utf8 so if position(a in b) works for strings, it should work for numbers too (unfortunately).

Upd: Can check out #12751 for the fix

Thanks, will take a look!

[jayzhan211]

In DataFusion everything is coercible into Utf8 so if position(a in b) works for strings, it should work for numbers

I don't believe this is accurate. Ideally, the behavior should be customizable, with default behavior similar to Postgres. In this case, the query should raise an error rather than automatically coercing the value to a string.

[findepi]

i based my comment on this piece of code

datafusion/datafusion/expr/src/type_coercion/functions.rs

Lines 681 to 682 in 8db30e2

	// Any type can be coerced into strings
	(Utf8 | LargeUtf8, _) => Some(type_into.clone()),

[jayzhan211]

i based my comment on this piece of code

datafusion/datafusion/expr/src/type_coercion/functions.rs

Lines 681 to 682 in 8db30e2

	// Any type can be coerced into strings
	(Utf8 | LargeUtf8, _) => Some(type_into.clone()),

Great, that is the code path not used for signature like user-defined, numeric and so on. It is historic reason, I hope we can deprecate coerce_from in the future.

datafusion/datafusion/expr/src/type_coercion/functions.rs

Lines 170 to 182 in 84c9409

	fn is_well_supported_signature(type_signature: &TypeSignature) -> bool {
	if let TypeSignature::OneOf(signatures) = type_signature {
	return signatures.iter().all(is_well_supported_signature);
	}
	matches!(
	type_signature,
	TypeSignature::UserDefined
	| TypeSignature::Numeric(_)
	| TypeSignature::Coercible(_)
	| TypeSignature::Any(_)
	)
	}

[findepi]

coerced_from is used for "most signatures" starting from here:

datafusion/datafusion/functions/src/core/nullif.rs

Lines 84 to 86 in c6f0d3c

	let coerced_types = datafusion_expr::type_coercion::functions::data_types(
	arg_types,
	&self.signature,

including Exact, which seems the most typical signature definiiotn, so i understood it as the default coercion rules.

But you're right, this function is not used for UserDefined, Numeric and Coercible signatures. Why so?

datafusion/datafusion/expr/src/type_coercion/functions.rs

Lines 170 to 182 in 84c9409

	fn is_well_supported_signature(type_signature: &TypeSignature) -> bool {
	if let TypeSignature::OneOf(signatures) = type_signature {
	return signatures.iter().all(is_well_supported_signature);
	}
	matches!(
	type_signature,
	TypeSignature::UserDefined
	| TypeSignature::Numeric(_)
	| TypeSignature::Coercible(_)
	| TypeSignature::Any(_)
	)
	}

thanks for the link.
What is the definition of is_well_supported_signature? What is the difference between "well supported signature" and "not so well supported one"?

[jayzhan211]

The main difference is that that don't call coerce_from.

The reason I introduce those new signature that avoid coerce_from is that, coerce_from is a single large coercion rule function, any change of it has unknown effect for all the downstream project as well, and it is also not designed for extensibility. I think we can use UserDefined by default and introduce more signature like Numeric, String, Coercible as a general signature, if we found the coercion rule fit more than one function.

After replacing all the function with well_supported_signature, I think the coercion rule will be simplified and easy to reason, any change of it is also more in control.

BUT, the whole plan seems better if we have logical type supported #12622, therefore I didn't push the plan for now.