simplify regex expressions #4646

crepererum · 2022-12-15T11:57:49Z

Which issue does this PR close?

Closes #4370.

Rationale for this change

Regex expressions are expensive to evaluate. If we can, we shall use our blazing fast arrow like kernels instead.

What changes are included in this PR?

New expression optimizer rule to lower plan-time-known regexes to LIKE expressions.

Are these changes tested?

test_simplify_regex

Are there any user-facing changes?

Plans run faster now.

Closes apache#4370.

alamb · 2022-12-17T12:04:09Z

datafusion/optimizer/Cargo.toml

@@ -45,6 +45,7 @@ datafusion-expr = { path = "../expr", version = "15.0.0" }
 datafusion-physical-expr = { path = "../physical-expr", version = "15.0.0" }
 hashbrown = { version = "0.13", features = ["raw"] }
 log = "^0.4"
+regex-syntax = "0.6.28"


This is technically not a new dependency (as datafusion-physical-expr already depends on regex which depends on regex-syntax). However, regex is an optional dependency whereas this is not optional.

I think it would be better to move this dependency into datafusion-physical-expr so regex can still be optional. I will try to do so over the next few days

alamb · 2022-12-17T12:30:28Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+
+        // OR-chain
+        assert_change(
+            regex_match(col("c1"), lit("foo|bar|baz")),


I think it might be better to convert this to an IN <list> which has a bunch of optimizations https://github.com/apache/arrow-datafusion/blob/master/datafusion/physical-expr/src/expressions/in_list.rs

I remember a discussion about IN <list> vs OR chains -- @tustvold do you have any opinion on which is better in this case?

If you can express it as an IN that will be fastest, but there isn't a IN LIKE AFAIK (although we could implement such a thing)

We can extend the rewrite logic at one point to spot fully anchored regex expressions (i.e. the ones that span the whole string, not any substring). In this case we could emit EQ/NEQ/IN.

alamb · 2022-12-17T12:31:26Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        );
+    }
+
+    #[track_caller]


TIL https://rustc-dev-guide.rust-lang.org/backend/implicit-caller-location.html 👍

alamb · 2022-12-17T12:32:14Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        Expr::Like(Like {
+            negated: false,
+            expr: Box::new(expr),
+            pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),


Suggested change

pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),

pattern: Box::new(lit(pattern)),

alamb · 2022-12-17T12:32:20Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        Expr::Like(Like {
+            negated: true,
+            expr: Box::new(expr),
+            pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),


Suggested change

pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),

pattern: Box::new(lit(pattern)),

alamb · 2022-12-17T12:32:26Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        Expr::ILike(Like {
+            negated: false,
+            expr: Box::new(expr),
+            pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),


Suggested change

pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),

pattern: Box::new(lit(pattern)),

in 197e1cf

alamb · 2022-12-17T12:33:06Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        })
+    }
+
+    fn like(expr: Expr, pattern: &str) -> Expr {


I think there are already functions like this : https://docs.rs/datafusion/15.0.0/datafusion/prelude/enum.Expr.html#method.like

🤔 but it turns out that those make Expr::Like rather than BinaryOperator::Like 🤔 -- I am not sure why there is duplicate

Filed #4765 which I think is fairly serious

alamb · 2022-12-17T12:36:29Z

I still need to review the rewrite logic carefully

tustvold · 2022-12-19T12:39:14Z

I wonder if we could eventually push some of this logic into the arrow-rs kernel itself 🤔

alamb

I went through the lowering logic and it looks very nice to me. Thank you @crepererum

alamb · 2022-12-26T13:55:23Z

datafusion/optimizer/src/simplify_expressions/regex.rs

+
+    if let Expr::Literal(ScalarValue::Utf8(Some(pattern))) = right.as_ref() {
+        match regex_syntax::Parser::new().parse(pattern) {
+            Ok(hir) => {


Relevant docs in case anyone else is looking: https://docs.rs/regex-syntax/0.6.28/regex_syntax/struct.Parser.html#method.parse

alamb · 2022-12-26T14:02:19Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                .and(not_like(col("c1"), "%baz%")),
+        );
+    }
+


alamb

There was a conflict with datafusion-cli/Cargo.lock which I resolved.

Since I had to push changes anyways, I also cleaned up the tests and added coverage.

alamb · 2022-12-28T21:27:19Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        })
+    }
+
+    fn like(expr: Expr, pattern: &str) -> Expr {


🤔 but it turns out that those make Expr::Like rather than BinaryOperator::Like 🤔 -- I am not sure why there is duplicate

alamb · 2022-12-28T21:32:19Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        Expr::ILike(Like {
+            negated: false,
+            expr: Box::new(expr),
+            pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),


in 197e1cf

ursabot · 2022-12-29T12:42:09Z

Benchmark runs are scheduled for baseline = ad0459e and contender = d4934e8. d4934e8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Dec 15, 2022

crepererum force-pushed the crepererum/issue4370c branch from cbf81d2 to c719afa Compare December 15, 2022 12:04

feat: simplify regex expressions

d29576b

Closes apache#4370.

crepererum force-pushed the crepererum/issue4370c branch from c719afa to d29576b Compare December 16, 2022 11:41

crepererum marked this pull request as ready for review December 16, 2022 11:42

github-actions bot removed logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) physical-expr Physical Expressions core Core DataFusion crate labels Dec 16, 2022

alamb reviewed Dec 17, 2022

View reviewed changes

alamb approved these changes Dec 26, 2022

View reviewed changes

alamb added 4 commits December 28, 2022 15:16

Merge remote-tracking branch 'apache/master' into crepererum/issue4370c

9663363

Fix typo in constant name, add coverage

3aed833

cleanups

197e1cf

fmt

0809d05

alamb reviewed Dec 28, 2022

View reviewed changes

alamb mentioned this pull request Dec 28, 2022

Multiple ways to express like / ilike / not like / not ilike #4765

Closed

alamb merged commit d4934e8 into apache:master Dec 29, 2022

tustvold mentioned this pull request Jan 19, 2023

Add rlike string comparison apache/arrow-rs#3568

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify regex expressions #4646

simplify regex expressions #4646

crepererum commented Dec 15, 2022 •

edited

Loading

alamb Dec 17, 2022

alamb Dec 17, 2022

tustvold Dec 19, 2022

crepererum Jan 2, 2023

alamb Dec 17, 2022

alamb Dec 17, 2022

alamb Dec 17, 2022

alamb Dec 17, 2022

alamb Dec 28, 2022

alamb Dec 17, 2022

alamb Dec 28, 2022

alamb Dec 28, 2022

alamb commented Dec 17, 2022

tustvold commented Dec 19, 2022

alamb left a comment

alamb Dec 26, 2022

alamb Dec 26, 2022

alamb Dec 28, 2022

alamb left a comment

alamb Dec 28, 2022

alamb Dec 28, 2022

ursabot commented Dec 29, 2022

	pattern: Box::new(Expr::Literal(ScalarValue::Utf8(Some(pattern.to_owned())))),
	pattern: Box::new(lit(pattern)),

simplify regex expressions #4646

simplify regex expressions #4646

Conversation

crepererum commented Dec 15, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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

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

alamb commented Dec 17, 2022

tustvold commented Dec 19, 2022

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

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

ursabot commented Dec 29, 2022

crepererum commented Dec 15, 2022 •

edited

Loading