Improve error message when string functions receive Binary types

[lemorage]

Which issue does this PR close?

• Related to Improve error messages when coercion fails #19004 and type coercion error in clickbench50 query #19809.

What changes are included in this PR?

Change internal error to user-facing error when function type coercion fails. Add helpful hint when Binary types are used with string functions.

Before:

Internal error: Expect TypeSignatureClass::Native(...) but received NativeType::Binary, DataType: Binary

After:

Error: Function 'split_part' requires String, but received Binary (DataType: Binary).
Hint: Binary types are not automatically coerced to String. Use CAST(column AS VARCHAR) to convert Binary data to String.

[lemorage]

The fix is prepared, I am not sure if this would be a reasonable change tbh tho. Maybe just setting config is fine? set datafusion.execution.parquet.binary_as_string = true. Need to be discussed before merging.

[Jefffrey]

Personally I'm not sure about this approach as it might suggest all other string functions with similar signature should now be fixed to also ensure they coerce from binary 🤔

e.g.

datafusion/datafusion/functions/src/string/ascii.rs

Lines 63 to 74 in 0808f3a

	impl AsciiFunc {
	pub fn new() -> Self {
	Self {
	signature: Signature::coercible(
	vec![Coercion::new_exact(TypeSignatureClass::Native(
	logical_string(),
	))],
	Volatility::Immutable,
	),
	}
	}
	}

datafusion/datafusion/functions/src/string/btrim.rs

Lines 80 to 98 in 0808f3a

	impl BTrimFunc {
	pub fn new() -> Self {
	Self {
	signature: Signature::one_of(
	vec![
	TypeSignature::Coercible(vec![
	Coercion::new_exact(TypeSignatureClass::Native(logical_string())),
	Coercion::new_exact(TypeSignatureClass::Native(logical_string())),
	]),
	TypeSignature::Coercible(vec![Coercion::new_exact(
	TypeSignatureClass::Native(logical_string()),
	)]),
	],
	Volatility::Immutable,
	),
	aliases: vec![String::from("trim")],
	}
	}
	}

datafusion/datafusion/functions/src/string/contains.rs

Lines 59 to 71 in 0808f3a

	impl ContainsFunc {
	pub fn new() -> Self {
	Self {
	signature: Signature::coercible(
	vec![
	Coercion::new_exact(TypeSignatureClass::Native(logical_string())),
	Coercion::new_exact(TypeSignatureClass::Native(logical_string())),
	],
	Volatility::Immutable,
	),
	}
	}
	}

[adriangb]

I guess ClickHouse does coerce them? So is this just a case of our semantics != ClickHouse?

[Jefffrey]

I guess ClickHouse does coerce them? So is this just a case of our semantics != ClickHouse?

It's a similar problem to how Spark will coerce strings to ints for their math functions but we don't.

• Related: Support numeric string argument in functions  #13285 (well it mentions PG here but same holds for Spark)

[martin-g]

replace.rs uses binary_to_string_coercion() in its invoke_with_args():
https://github.com/lemorage/datafusion/blob/85eebcd25dfbe8e2d2d75d85b8683de8be4851e8/datafusion/functions/src/string/replace.rs#L117-L118

Same for:

• starts_with - https://github.com/lemorage/datafusion/blob/main/datafusion/functions/src/string/starts_with.rs#L41-L42
• contains - https://github.com/lemorage/datafusion/blob/main/datafusion/functions/src/string/contains.rs#L104
• ends_with - https://github.com/lemorage/datafusion/blob/main/datafusion/functions/src/string/ends_with.rs#L118

and maybe more.

[alamb]

I guess ClickHouse does coerce them? So is this just a case of our semantics != ClickHouse?

The problem is only with the ClickHouse hits_partitioned dataset (which has 100 parquet files) which was written by an ancient version of pyarrow which did not correctly annotate the string columns as STRING. This is the only usecase for this config:

https://datafusion.apache.org/user-guide/configs.html

datafusion.execution.parquet.binary_as_string	false	(reading) If true, parquet reader will read columns of Binary/LargeBinary with Utf8, and BinaryView with Utf8View. Parquet files generated by some legacy writers do not correctly set the UTF8 flag for strings, causing string columns to be loaded as BLOB instead.

So TLDR I don't think we need to make split_part work with binary data, as proposed in this PR.

Instead I propose:

1. Change the internal error to a proper error when running split_part on binary data
2. Update the benchmarks to properly set the binary as string. I made a PR here: Use correct setting for click bench queries in sql_planner benchmark #19835

[alamb]

Thank you for working on this @lemorage

Did you find out what was causing the internal error? I would expect the query would error with a normal error (about not supporting binary columns)

[Jefffrey]

Did you find out what was causing the internal error? I would expect the query would error with a normal error (about not supporting binary columns)

This is related to

• Improve error messages when coercion fails #19004

[lemorage]

Thanks folks! Have updated the PR to improve the error msg better instead.

[alamb]

Thank you @lemorage -- looks great to me

[alamb]

👍 for a planning error rather than internal error

[alamb]

I think these messages are much nicer -- thank you @lemorage

[alamb]

Thanks again @lemorage

[lemorage]

@alamb Thanks! And also all the discussions through! ❤️