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

findepi · 2024-11-05T12:51:11Z

cover expression known not to be null
cover NULL pattern
cover repeated '%%' in pattern
currently also includes changes from Simplify EXPR LIKE 'constant' to expr = 'constant' #13061 to resolve conflicts

findepi · 2024-11-05T13:39:35Z

currently depends on #13259

findepi · 2024-11-05T15:04:19Z

CI is green, so ready to review
but this will conflict with @adriangb's #13061, because this changes code structure a little bit
conflict resolution will be trivial though.
@adriangb if you want, i can do this, adding your changes to this PR

findepi · 2024-11-05T15:12:34Z

seemed easy enough, done.

adriangb · 2024-11-05T15:15:40Z

I’m happy with my changes being included in this PR :)

findepi · 2024-11-05T15:22:34Z

draft - to be rebased after #13259 lands

still ready to review
cc @crepererum @goldmedal

goldmedal

Just roughly review now. Overall looks to me. I will check the test cases tomorrow.

datafusion/optimizer/Cargo.toml

goldmedal · 2024-11-06T02:51:07Z

draft - to be rebased after #13259 lands

still ready to review cc @crepererum @goldmedal

#13259 has been merged. 👍

- cover expression known not to be null - cover NULL pattern - cover repeated '%%' in pattern

findepi · 2024-11-06T07:31:35Z

@goldmedal rebased, thanks!

alamb

Thank you @findepi and @goldmedal

This looks like a great change to me except for the handling of %% which I am not sure about. Otherwise 👍

alamb · 2024-11-06T11:38:51Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

@@ -2987,41 +3051,41 @@ mod tests {
        })
    }

-    fn like(expr: Expr, pattern: &str) -> Expr {
+    fn like(expr: Expr, pattern: impl Into<Expr>) -> Expr {


I think we could use Expr::like https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html#method.like and similar here instead of these functions

This is likely left over from when the Expr API was less expressive

alamb · 2024-11-06T11:43:40Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        let expr = not_ilike(col("c1"), lit("%"));
+        assert_eq!(simplify(expr), if_not_null(col("c1"), false));
+
+        // expr [NOT] [I]LIKE '%%'


I don't understand this simplification

I thought expr LIKE '%%' means the same as expr = '%' (match the actual literal character % )

Specifically that %% is how to escape the wildcard to not be a wildcard

This test looks like it has applied the rewrite needed for expr LIKE '%' which matches any non null input

🤔

Also, since the code seems to handle more than two %, could you add a test for patterns like

col LIKE '17%%' // 17%

col LIKE '%%five%% // %five%

col LIKE '%%%%five%%%% // %%five%%

I thought expr LIKE '%%' means the same as expr = '%' (match the actual literal character % )

Specifically that %% is how to escape the wildcard to not be a wildcard

I think it depends on how we define it. I assume we prefer to follow the Postgres behavior. 🤔
In Postgres, %% doesn't mean matching the actual literal '%'. The escape character is \.

test=# select '1' like '%%'; ?column? ---------- t (1 row) test=# select '%' like '%%'; ?column? ---------- t (1 row) test=# select '%' like '\%'; ?column? ---------- t (1 row) test=# select '1' like '\%'; ?column? ---------- f (1 row)

I think %% is just a redundant wildcard. So, we can simplify it to be only one %.

By the way, in DuckDB, there is a similar behavior for double '%%'.

D select '1' like '%%'; ┌───────────────┐ │ ('1' ~~ '%%') │ │ boolean │ ├───────────────┤ │ true │ └───────────────┘ D select '%' like '%%'; ┌───────────────┐ │ ('%' ~~ '%%') │ │ boolean │ ├───────────────┤ │ true │ └───────────────┘

However, the escape character should be set by ESCAPE syntax

D select '%' like '\%'; ┌───────────────┐ │ ('%' ~~ '\%') │ │ boolean │ ├───────────────┤ │ false │ └───────────────┘ D select '%' like '\%' escape '\'; ┌──────────────────────────────────┐ │ main.like_escape('%', '\%', '\') │ │ boolean │ ├──────────────────────────────────┤ │ true │ └──────────────────────────────────┘

I was definitely confused -- thank you

alamb · 2024-11-06T11:45:25Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                                    })
+                                })
+                            }
+                            Some(pattern_str)


I am not sure about this rule (see questions below) -- maybe you could add some comments explanation on what it is supposed to do.

Suggested change

Some(pattern_str)

Some(pattern_str)

// Repeated occurrences of wildcard are redundant so remove them

// exp LIKE '%%' --> exp LIKE '%'

alamb · 2024-11-06T11:48:36Z

datafusion/sqllogictest/test_files/string/string_view.slt

@@ -396,8 +396,9 @@ EXPLAIN SELECT
 FROM test;
 ----
 logical_plan
-01)Projection: test.column1_utf8view LIKE Utf8View("foo") AS like, test.column1_utf8view ILIKE Utf8View("foo") AS ilike
-02)--TableScan: test projection=[column1_utf8view]
+01)Projection: __common_expr_1 AS like, __common_expr_1 AS ilike


Nice!

Though I think this test's meaning is now changed it is supposed to be verifying cast exprs for like

Perhaps you can change the patterns to like '%foo%'

Agreed. I did some tests. I think they still use the native StringView implementation for pattern matching.

query TT EXPLAIN SELECT column1_utf8view like '%foo%' as "like", column1_utf8view ilike '%foo%' as "ilike" FROM test; ---- logical_plan 01)Projection: test.column1_utf8view LIKE Utf8View("%foo%") AS like, test.column1_utf8view ILIKE Utf8View("%foo%") AS ilike 02)--TableScan: test projection=[column1_utf8view]

goldmedal · 2024-11-06T13:55:32Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        let null = lit(ScalarValue::Utf8(None));
+
+        // expr [NOT] [I]LIKE NULL
+        let expr = like(col("c1"), null.clone());


Suggested change

let expr = like(col("c1"), null.clone());

let expr = col("c1").like(null.clone());

As @alamb's comment (#13260 (comment)), we can do some refactor like this.

goldmedal · 2024-11-06T14:03:15Z

datafusion/sqllogictest/test_files/string/string_view.slt

@@ -396,8 +396,9 @@ EXPLAIN SELECT
 FROM test;
 ----
 logical_plan
-01)Projection: test.column1_utf8view LIKE Utf8View("foo") AS like, test.column1_utf8view ILIKE Utf8View("foo") AS ilike
-02)--TableScan: test projection=[column1_utf8view]
+01)Projection: __common_expr_1 AS like, __common_expr_1 AS ilike


Agreed. I did some tests. I think they still use the native StringView implementation for pattern matching.

query TT EXPLAIN SELECT column1_utf8view like '%foo%' as "like", column1_utf8view ilike '%foo%' as "ilike" FROM test; ---- logical_plan 01)Projection: test.column1_utf8view LIKE Utf8View("%foo%") AS like, test.column1_utf8view ILIKE Utf8View("%foo%") AS ilike 02)--TableScan: test projection=[column1_utf8view]

crepererum · 2024-11-06T15:43:55Z

since there are already two reviewers on it, I'll gonna skip this PR. However if you need my input, ping me.

alamb

Thank you @findepi and @goldmedal

I had some small testing suggestions but I think we can add that coverage as a follow on PR as well

github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Nov 5, 2024

findepi force-pushed the findepi/expand-like-simplification-e96eca branch from c6285bf to be26107 Compare November 5, 2024 13:39

findepi mentioned this pull request Nov 5, 2024

Incorrect LIKE and ILIKE result for NULL input, `%', and '%%' pattern #12637

Open

findepi force-pushed the findepi/expand-like-simplification-e96eca branch from 88d6df1 to 6c57af7 Compare November 5, 2024 15:14

findepi marked this pull request as draft November 5, 2024 15:21

goldmedal reviewed Nov 5, 2024

View reviewed changes

datafusion/optimizer/Cargo.toml Outdated Show resolved Hide resolved

adriangb mentioned this pull request Nov 5, 2024

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

Closed

goldmedal mentioned this pull request Nov 6, 2024

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

Merged

findepi and others added 2 commits November 6, 2024 08:30

Expand LIKE simplification

068866e

- cover expression known not to be null - cover NULL pattern - cover repeated '%%' in pattern

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

520ad2b

findepi force-pushed the findepi/expand-like-simplification-e96eca branch from e4eae46 to 520ad2b Compare November 6, 2024 07:31

findepi marked this pull request as ready for review November 6, 2024 07:31

alamb reviewed Nov 6, 2024

View reviewed changes

alamb changed the title ~~Expand LIKE simplification~~ Expand LIKE simplification: cover NULL pattern/expression and constant Nov 6, 2024

goldmedal reviewed Nov 6, 2024

View reviewed changes

alamb approved these changes Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

findepi commented Nov 5, 2024 •

edited

Loading

findepi commented Nov 5, 2024

findepi commented Nov 5, 2024

findepi commented Nov 5, 2024

adriangb commented Nov 5, 2024

findepi commented Nov 5, 2024

goldmedal left a comment

goldmedal commented Nov 6, 2024

findepi commented Nov 6, 2024

alamb left a comment

alamb Nov 6, 2024

alamb Nov 6, 2024

alamb Nov 6, 2024

goldmedal Nov 6, 2024 •

edited

Loading

alamb Nov 6, 2024

alamb Nov 6, 2024

alamb Nov 6, 2024

alamb Nov 6, 2024

goldmedal Nov 6, 2024

goldmedal Nov 6, 2024

goldmedal Nov 6, 2024

crepererum commented Nov 6, 2024

alamb left a comment

	let expr = like(col("c1"), null.clone());
	let expr = col("c1").like(null.clone());

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

Are you sure you want to change the base?

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

Conversation

findepi commented Nov 5, 2024 • edited Loading

findepi commented Nov 5, 2024

findepi commented Nov 5, 2024

findepi commented Nov 5, 2024

adriangb commented Nov 5, 2024

findepi commented Nov 5, 2024

goldmedal left a comment

Choose a reason for hiding this comment

goldmedal commented Nov 6, 2024

findepi commented Nov 6, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldmedal Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crepererum commented Nov 6, 2024

alamb left a comment

Choose a reason for hiding this comment

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

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

findepi commented Nov 5, 2024 •

edited

Loading

goldmedal Nov 6, 2024 •

edited

Loading