Support DictionaryString for Regex matching operators

[blaginin]

Which issue does this PR close?

Closes #12618

Rationale for this change

As explained in the original PR, regex comparison operations don't support dictionaries like ~*.

What changes are included in this PR?

When building a logical query, the query is initially correctly coerced to Utf8 (which is supported in BinaryExpr), but then unwrap_cast_in_comparison removes the column cast.

Interestingly, for the Operator::LikeMatch operator, which is similar to regex operators, unwrap_cast_in_comparison doesn't rewrite the query. This happens because of the function currently named is_comparison_operator.

According to its usages, it is only used in interval propagation, and the number of actual arguments supported is actually smaller than the number mentioned in the method.

Moreover, the name is_comparison_operator seems a bit confusing, as it doesn't match the list of comparison operators in the docs. Therefore, I have renamed the method.

Are these changes tested?

Yes, I uncommented tests in datafusion/sqllogictest/test_files/string.

Are there any user-facing changes?

No

[alamb]

Thank you for this contribution @blaginin -- I agree this PR seems to have fixed the issue, but I am a little worried about it.

1. It is probably slower ( I think this PR effectively leaves the cast to Utf8 there (which would basically unpack the dictionary array))
2. It seems to change interval analysis and propagation to not support LIKE etc.

FYI @ozankabak I am a bit worried that the changes to the propagation logic did not cause any test failures 🤔 Maybe we need to increase test coverage in that area.

[alamb]

What would you think about explicitly implementing physical operator support for DictionaryArrays? That would mean basically applying the operation on the DictionaryArray::values

That would look something like

let input_array: DictionaryArray<Int32, String> = ...;
let values = input_array.values();
// apply regexp match on the values of the array
let regexp_match_result = regexp_match(values, ..);
// form the output boolean array by looking up the result
let mut bool_builder = BooleanBuilder::new()
for key in input_array.keys() {
  bool_builder.push(regexp_match_result.value(key))
}
bool_builder.build(); // create output boolean arary

[alamb]

You would have to find the relevant place to plumb this in to binary.rs too I think

[alamb]

Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look

[blaginin]

Thanks for the review and detailed feedback, @alamb! 😍

To give a bit of context, when submitting the PR, I was choosing between removing Regex operators from is_comparison_operator and manually unwrapping the dict. I chose the first option because expressions like dict_column ~* 'scalar' are the only outlier. dict_column ~~ 'scalar' and even dict_column ~* dict_column are both cast to UTF from dicts, so I thought consistent behavior was desirable.

However, your performance point is really valid!! Unwrapping the dict with bools directly can be much more efficient than unwrapping strings and then constructing booleans... I think I've made a change; could you take a look?

A few things to consider on top of this PR:

• Maybe we should stop casting in other cases too (mentioed above)
• Feels like we might want to return results as a dict, rather than unwrapping it. I tried to make that change in this PR, but it became bigger than the actual fix 😅

[alamb]

Thank you @blaginin -- this looks great.

The only thing I think we should do is to deprecate is_comparison_op to ease people's upgrades

[alamb]

this is technically an API change -- Can you please avoid the change and help people upgrade by adding back is_comparison_operator that calls supports_propagation and mark it deprecated?

datafusion/datafusion/core/src/datasource/file_format/parquet.rs

Lines 610 to 619 in 3353c06

	#[deprecated(
	since = "40.0.0",
	note = "please use `statistics_from_parquet_meta_calc` instead"
	)]
	pub async fn statistics_from_parquet_meta(
	metadata: &ParquetMetaData,
	table_schema: SchemaRef,
	) -> Result<Statistics> {
	statistics_from_parquet_meta_calc(metadata, table_schema)
	}

[alamb]

(We could add the deprecation as a follow on)

[alamb]

👍

[goldmedal]

Thanks @blaginin for working on this. It looks good to me 👍

[alamb]

Thanks @blaginin and @goldmedal for the review

[berkaysynnada]

I just noticed the change at is_comparison_operator(). The new name actually does not reflect what it is used for in interval arithmetic related codes. I am planning to open a PR for this.

@blaginin do you have a preferred new name for supports_propagation() considering its use only in unwrap_cast_in_comparison.rs -- I am asking for that only since I will separate the other uses with a different name specific to constraint propagation, and narrow the scope of supported types.

[blaginin]

hey @berkaysynnada, I found the name is_comparison_operator a bit confusing because it’s not consistent with the documentation. For example:

do you have a preferred new name for supports_propagation() considering its use only in unwrap_cast_in_comparison.rs

Since it’s only for unwrap_cast_in_comparison.rs, maybe we could rename it to something like supports_unwrap_cast?

[berkaysynnada]

I won't revert the name change to is_comparison_operator(). Instead, I will create a similar function with a narrower scope, excluding operators like regex, since they aren't supported in the interval arithmetic part. In the end, there will be two functions: one for the current usage in unwrap_cast_in_comparison.rs, and another for interval arithmetic with a more specific set of operators. I hope this clarifies my intention.

[blaginin]

makes a lot of sense!! 👍