Simplify `EXPR LIKE 'constant'` to `expr = 'constant'` by adriangb · Pull Request #13061 · apache/datafusion

adriangb · 2024-10-22T14:28:15Z

adriangb · 2024-10-22T14:28:37Z

datafusion/optimizer/Cargo.toml

 chrono = { workspace = true }
 datafusion-common = { workspace = true, default-features = true }
 datafusion-expr = { workspace = true }
+datafusion-functions = { workspace = true }


Not sure if this is okay, or if there is a way to avoid it?

I think we need to make some interface for rewriting the function through 🤔

Perhaps adding a method to SimplifyInfo like

/// return a new Expr with a function call for exact string prefix max fn make_starts_with(base: Expr, prefix: Expr) -> Option<Expr>

Hmm not sure what you mean. Is this a pattern already in use somewhere I can learn from?

I was suggest adding a method to https://docs.rs/datafusion/latest/datafusion/logical_expr/simplify/trait.SimplifyInfo.html

However i can't quite figure out how to plumb it in without getting messy

THe trick is that this rewrite is based on the semantcs of starts_with -- so I think logically it would need to live along side the implementation of starts_with because it depends on those semantics.

And similarly, to do the rewrite in the pruning predicate we need something similar (somehow to provide a per function rewrite) 🤔

Hmm okay I'm happy to work on this more but I fear I am not familiar enough with the relationship between all of these things to figure out how to wire everything up correctly. Maybe we pause this work until someone with more familiarity of these bits of code can suggest a way to structure this?

I will try and help push this forward this week. I am sorry for the delay but I have been super busy the last few days

No problem 😄

adriangb · 2024-10-22T15:56:43Z

Wow, this is now rewriting col ~ '^foo' -> col like 'foo%' -> starts_with(col, 'foo') 🤯
If we implement stats pushdown for starts_with(col, 'foo') we'll automatically get col ~ '^foo' pushed down 💪🏻

alamb · 2024-10-30T18:49:33Z

I thought more about this PR the other day -- given the dependecy you have identified, I wonder if we might actually want to do the opposite

Specifically rewrite

starts_with(x, 'foo')

into

x LIKE 'foo%'

With the rationale that like already has quite a bit of special case / optimization.

I'll try and find a ticket to track the idea

adriangb · 2024-10-30T19:56:30Z

Hmm interesting. startswith does seem simpler to me in principle, but I'd understand if it's better to do the other way around.

The dependency is a bit strange to me, I'm not sure why functions would depend on the optimizer, I would think the optimizer could depend on functions.

alamb

@adriangb what would you think about updating this PR to handle the no wild card case.

The more I think about the prefix case the less I am convinced that translating to some other function would be valuable. Instead I am thinking maybe we can ensure the LIKE evaluation is fast (perhaps it can detect this case and invoke a better kernel) 🤔

adriangb · 2024-10-31T00:22:52Z

To handle ONLY the no wildcard case? Happy to do that if that's all the progress we can make for now.

findepi · 2024-10-31T07:28:15Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                        if index == pattern.len() - 1 && !case_insensitive && escape_char.is_none() {
+                            let prefix = pattern[..index].to_string();
+                            let new_expr = Expr::ScalarFunction(ScalarFunction {
+                                func: datafusion_functions::string::starts_with(),


Today a downstream project can choose which functions to use, this is configurable in the session, right? The DF repertoire of functions serves as a convenient package, but not mandatory.
Here we're inserting implicit function call to a specific function.
Is this following a pre-existing pattern? if so, i am OK with this.
But maybe it would be better to eg consult session state whether this function was supposed to be used. What do you think?

findepi · 2024-10-31T07:30:03Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

            Expr::Like(Like {
-                expr,
-                pattern,
+                expr: ref like_expr,


keep expr as expr
use like_expr when referring to the whole Expr::Like thing

findepi · 2024-10-31T07:43:42Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                    if pattern == "%" && !is_null(like_expr) {
+                        return Ok(Transformed::yes(lit(!negated)));


This is cool idea, but is_null(like_expr) checks makes it incorrect.
This function returns "is this always null". Applied to a column reference, it will return false, so this optimization will kick in, but the column values are still nullable.

The "more correct" way would be to

optimize WHERE col LIKE '%' to WHERE col IS NOT NULL

optimize WHERE col NOT LIKE '%' to WHERE false.

However, this simplification cannot be done here. It can only be done for top-level filter conjuncts, or when expr being simplified is immediately used as a condition (eg IF(expr, ...)). In other cases, this need to be NULL-preserving boolean logic:

trino> SELECT a, a LIKE '%', a NOT LIKE '%' -> FROM (VALUES NULL, '', 'x') t(a); a | _col1 | _col2 ------+-------+------- NULL | NULL | NULL | true | false x | true | false (3 rows)

The easiest solution would be to just drop this special case.
I.e. expr LIKE '%' would get simplified to starts_with(expr, '')

See also #12637 (comment)

findepi · 2024-10-31T07:44:16Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                    let pct_wildcard_index = pattern.find('%');
+                    let underscore_index = pattern.find('_');


Honor escape_char

findepi · 2024-10-31T07:46:00Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+
+                    // Handle single % at end case (prefix search)
+                    if let Some(index) = pct_wildcard_index {
+                        if index == pattern.len() - 1 && !case_insensitive && escape_char.is_none() {


&& underscore_index.is_none()

findepi · 2024-10-31T07:47:59Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

        let expr = not_ilike(col("c1"), "%");
        assert_eq!(simplify(expr), lit(false));

+        let expr = like(col("c1"), "a%");


add test cases

with % pattern

with _ pattern (can be simplified to a length check, but that can be follow up)

with %% and %%% patterns (can be simplified just like single %)

with __ and ___ patterns (can be simplified to a length check, but that can be follow up)

with patterns combining % and _, eg a_b%, a%b_ (these cannot simplified) and _%, %_ (these can be simplified to a length check, but that can be follow up)

findepi · 2024-11-02T07:49:35Z

do we have equivalent of #12978 for starts_with?

adriangb · 2024-11-02T19:28:19Z

do we have equivalent of #12978 for starts_with?

No but I think it will have a lot of code sharing with #12978 and should be relatively easy to implement. After we merge that PR I'd be happy to go back in and implement it, doing whatever refactoring is needed to get good code sharing.

adriangb · 2024-11-03T23:01:59Z

@alamb @findepi I redid this PR now to only handle the equality cases and close the issue.

I do still think it'd be nice to figure out how to rewrite to startswith but it seems like that requires some larger scaler refactoring that I don't have the knowledge or time to carry out.

findepi · 2024-11-04T09:29:33Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                    if !is_null(&like_expr.expr) && pattern_str == "%" {
+                        Transformed::yes(lit(!like_expr.negated))


I don't think this is correct. Did you consider #13061 (comment) ?

This code was already here (see diff above). Did I change the logic in any way?

Ack. Can you please confirm or disconfirm the correctness concern?

I agree this is incorrect but this is the logic that was there before this PR. I'm happy to remove it. I guess no one has complained because the case that breaks is SELECT NULL LIKE '%' which isn't something that happens under normal usage.

I agree this is incorrect but this is the logic that was there before this PR.

thanks for confirming and understood it's pre-existing.

Can it be fixed though?
i personally would prefer that git annotate on the file wouldn't point at me as the author of something that's known to be incorrect.

My latest commit removes that special case. I'm not sure how to fix it, your comment suggests it can't be fixed right?

I guess no one has complained because the case that breaks is SELECT NULL LIKE '%' which isn't something that happens under normal usage.

actually the opposite.
this was correct only for this case
and wrong for all other ... LIKE '%' queries.

see #13259

findepi · 2024-11-04T09:31:46Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                    } else if !pattern_str.contains(['%', '_'].as_ref()) {
+                        // If the pattern does not contain any wildcards, we can simplify the like expression to an equality expression


Add TODO to handle escaped search characters in a follow-up.
Escaped wildcards are not uncommon in certain scenarios - eg correct use of JDBC DatabaseMetaData will produce LIKE foo\_bar when inspecting object foo_bar.

I made it not simplify if there is an escape character for now

findepi · 2024-11-04T09:32:47Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        let expr = like(col("c1"), "a_");
+        assert_eq!(simplify(expr), col("c1").like(lit("a_")));
+        let expr = not_like(col("c1"), "a_");
+        assert_eq!(simplify(expr), col("c1").not_like(lit("a_")));


add a test cases where pattern is constant NULL (it won't be simplified today, that's fine)

Added in 6c847fa

findepi · 2024-11-04T16:31:57Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                        Transformed::yes(lit(!like_expr.negated))
+                    } else if !pattern_str.contains(['%', '_'].as_ref()) {
+                        // If the pattern does not contain any wildcards, we can simplify the like expression to an equality expression
+                        if like_expr.escape_char.is_some() {


nit: can this condition be checked on the same line with !pattern_str.contains(['%', '_']
it would reduce code branching.

findepi · 2024-11-05T13:38:31Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

-                    Expr::Literal(ScalarValue::Utf8(Some(pattern_str))) if pattern_str == "%"
-                ) =>
-            {
-                Transformed::yes(lit(!negated))


remove this is correct, but requires updating some test cases.
i am doing this in #13259

awesome, I'll wait for your work to be merged then continue here

adriangb · 2024-11-05T20:16:03Z

Closing in favor of #13260

github-actions bot added the optimizer Optimizer rules label Oct 22, 2024

adriangb commented Oct 22, 2024

View reviewed changes

alamb mentioned this pull request Oct 30, 2024

Simplify EXPR LIKE 'constant' to expr = 'constant' #13192

Closed

alamb reviewed Oct 30, 2024

View reviewed changes

findepi requested changes Oct 31, 2024

View reviewed changes

findepi mentioned this pull request Oct 31, 2024

Allow testing records with sibling whitespace in SLT tests and add more string tests #13197

Merged

findepi mentioned this pull request Nov 2, 2024

Implement predicate pruning for like expressions (prefix matching) #12978

Merged

adriangb changed the title ~~rewrite prefix and constant cases of like to startswith and equality~~ Simplify EXPR LIKE 'constant' to expr = 'constant' Nov 3, 2024

only handle cases that simplify to equalityu comparisons

d174f5f

adriangb force-pushed the starstwith branch from 3571e5e to d174f5f Compare November 3, 2024 23:00

fmt

354e62c

findepi reviewed Nov 4, 2024

View reviewed changes

add test for null pattern

6c847fa

findepi reviewed Nov 4, 2024

View reviewed changes

adriangb added 2 commits November 4, 2024 11:00

remove faulty logic, merge conditions

99b54be

fmt

1f13d0d

adriangb requested a review from findepi November 4, 2024 17:01

findepi mentioned this pull request Nov 5, 2024

Fix incorrect ... LIKE '%' simplification with NULLs #13259

Merged

findepi reviewed Nov 5, 2024

View reviewed changes

findepi mentioned this pull request Nov 5, 2024

Expand LIKE simplification: cover NULL pattern/expression and constant #13260

Merged

adriangb closed this Nov 5, 2024

		if pattern == "%" && !is_null(like_expr) {
		return Ok(Transformed::yes(lit(!negated)));

		let pct_wildcard_index = pattern.find('%');
		let underscore_index = pattern.find('_');

		if !is_null(&like_expr.expr) && pattern_str == "%" {
		Transformed::yes(lit(!like_expr.negated))

		} else if !pattern_str.contains(['%', '_'].as_ref()) {
		// If the pattern does not contain any wildcards, we can simplify the like expression to an equality expression

Conversation

adriangb commented Oct 22, 2024 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Oct 22, 2024

Uh oh!

alamb commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented Oct 30, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

adriangb commented Oct 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Nov 2, 2024

Uh oh!

adriangb commented Nov 2, 2024

Uh oh!

adriangb commented Nov 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Oct 22, 2024 •

edited by alamb

Loading

alamb commented Oct 30, 2024 •

edited

Loading