MySQL LIKE predicate pushdown for varchar/nvarchar columns. #18441

vlad-lyutenko · 2023-07-27T14:13:57Z

Description

LIKE predicate pushdown for varchar/nvarchar columns.

splited from #18140

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/GenericRewrite.java

...jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeEscapeWithCaseSensitivity.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/expression/TestGenericRewrite.java

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

...jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeEscapeWithCaseSensitivity.java

...c/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeAndEscapeWithCaseSensitivity.java

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

...c/test/java/io/trino/plugin/jdbc/expression/TestRewriteLikeAndEscapeWithCaseSensitivity.java

...c/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeAndEscapeWithCaseSensitivity.java

Praveen2112 · 2023-07-31T13:29:10Z

...c/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeAndEscapeWithCaseSensitivity.java

+            return Optional.of(new ParameterizedExpression(format("%s LIKE %s ESCAPE %s", value.get().expression(), pattern.get().expression(), escape.get().expression()), parameters.build()));
+        }
+
+        return Optional.of(new ParameterizedExpression(format("%s LIKE %s", value.get().expression(), pattern.get().expression()), parameters.build()));


nit: Can we have a String builder which could capture the %s LIKE %s and optionally could add the ESCAPE clause towards the end and return a new ParameterizedExpression.

Praveen2112 · 2023-07-31T13:31:16Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

@@ -283,9 +284,53 @@ public void testAddNotNullColumn()
        }
    }

+    @Test
+    public void testLikePredicatePushdownWithCollation()


Is it possible to test them on top of a view created with collate ?

in this PR (#18140) we have such test, which is executed on top of view, I want to add there such test case, so need to merge it (#18140) first

...c/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeAndEscapeWithCaseSensitivity.java

hashhar · 2023-08-01T06:44:27Z

...c/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeAndEscapeWithCaseSensitivity.java

+            .with(argumentCount().matching(count -> count == 2 || count == 3))
+            .with(argument(0).matching(expression().capturedAs(LIKE_VALUE).with(type().matching(VarcharType.class::isInstance))))
+            .with(argument(1).matching(expression().capturedAs(LIKE_PATTERN).with(type().matching(VarcharType.class::isInstance))));
+    // TODO add capture on ESCAPE argument and type


should this be resolved before merging?
Right now we rely on what context.defaultRewrite(escapeExpression) returns.

We capture first 2 arguments, and third optional we took from arguments.

I cannot add third capture, because in this case - pattern matching will not be able to capture call with 2 arguments.

I know that it's optional -

public static Property<Call, ?, ConnectorExpression> argument(int argument) { checkArgument(0 <= argument, "Invalid argument index: %s", argument); return Property.optionalProperty(format("argument(%s)", argument), call -> { if (argument < call.getArguments().size()) { return Optional.of(call.getArguments().get(argument)); } return Optional.empty(); }); }

But it didn't work, maybe some bug in parsing.
I am investigating this and will create issue

Ok I think I need to remove todo, because it's not a bug, it's just the way how things works:
Inside WithPattern

@Override public <C> Stream<Match> accept(Object object, Captures captures, C context) { //TODO remove cast BiFunction<? super T, C, Optional<?>> property = (BiFunction<? super T, C, Optional<?>>) propertyPattern.getProperty().getFunction(); Optional<?> propertyValue = property.apply((T) object, context); return propertyValue.map(value -> propertyPattern.getPattern().match(value, captures, context)) .orElseGet(Stream::of); }

So if propertyValue is empty or propertyPattern.getPattern().match(value, captures, context) didn't match - it's the same, we didn't capture anything and returning empty stream and stop the flow.

So there is no such thing like "optional" capture argument - if you add it it will be captured or not.

This PR could be done as follow up - #18481

The simpler solution would be to just split into two rules - one for LIKE and one for LIKE with ESCAPE. Like how we have in the existing DSL based rules.

Changing Pattern so that it can accept partial matches without explicitly asking for partial matches looks risky.

But it would result in some redundant code right ?

For this PR however consider this comment resolved since the result is that the TODO is applicable.

@hashhar thx! refactored to separate rules
cc @Praveen2112

...c/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeAndEscapeWithCaseSensitivity.java

kokosing · 2023-08-01T12:01:52Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

@@ -30,6 +30,7 @@
 import java.sql.PreparedStatement;


Please add tests where column is case insensitive and push down does not happen

we have it in BaseMysqlConnectorTest:

@Test public void testPredicatePushdown() { // varchar like assertThat(query("SELECT regionkey, nationkey, name FROM nation WHERE name LIKE '%ROM%'")) .matches("VALUES (BIGINT '3', BIGINT '19', CAST('ROMANIA' AS varchar(255)))") .isNotFullyPushedDown(FilterNode.class);

...c/test/java/io/trino/plugin/jdbc/expression/TestRewriteLikeAndEscapeWithCaseSensitivity.java

Praveen2112

% changes

...bc/src/test/java/io/trino/plugin/jdbc/expression/BaseTestRewriteLikeWithCaseSensitivity.java

...e-jdbc/src/test/java/io/trino/plugin/jdbc/expression/TestRewriteLikeWithCaseSensitivity.java

.../src/test/java/io/trino/plugin/jdbc/expression/TestRewriteLikeEscapeWithCaseSensitivity.java

Praveen2112 · 2023-08-01T13:15:57Z

...jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeEscapeWithCaseSensitivity.java

+import static io.trino.spi.type.BooleanType.BOOLEAN;
+import static java.lang.String.format;
+
+public class RewriteLikeEscapeWithCaseSensitivity


Do we need to mention CaseSensitivity ? If we are going to use this rule for other JDBC related connectors ?

Not sure, I think it's important for developer to understand looking at name that we rely on case sensitivity in this rule.
He could miss it from code.

Praveen2112 · 2023-08-02T02:45:48Z

Thanks for working on this

findepi · 2023-12-19T14:48:52Z

...jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeEscapeWithCaseSensitivity.java

+        ConnectorExpression capturedValue = captures.get(LIKE_VALUE);
+        if (capturedValue instanceof Variable variable) {
+            JdbcColumnHandle columnHandle = (JdbcColumnHandle) context.getAssignment(variable.getName());
+            Optional<CaseSensitivity> caseSensitivity = columnHandle.getJdbcTypeHandle().getCaseSensitivity();
+            if (caseSensitivity.orElse(CASE_INSENSITIVE) == CASE_INSENSITIVE) {
+                return Optional.empty();
+            }


when ! capturedValue instanceof Variable, we don't know whether it's safe to pushdown and we should not do it

findepi · 2023-12-19T14:49:45Z

...jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeEscapeWithCaseSensitivity.java

+            .with(functionName().equalTo(LIKE_FUNCTION_NAME))
+            .with(type().equalTo(BOOLEAN))
+            .with(argumentCount().equalTo(3))
+            .with(argument(0).matching(expression().capturedAs(LIKE_VALUE).with(type().matching(VarcharType.class::isInstance))))


per https://github.com/trinodb/trino/pull/18441/files#r1431506745, the pattern should match only variables.

cla-bot bot added the cla-signed label Jul 27, 2023

vlad-lyutenko mentioned this pull request Jul 27, 2023

MySql case sensitive collation varchar push down support #18140

Merged

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch from ee965a1 to e723911 Compare July 27, 2023 14:23

vlad-lyutenko changed the title ~~LIKE predicate pushdown for varchar/nvarchar columns.~~ MySQL LIKE predicate pushdown for varchar/nvarchar columns. Jul 27, 2023

vlad-lyutenko mentioned this pull request Jul 25, 2023

Silent correctness issue for PSQL and SQLServer for LIKE predicate pushdown #18413

Open

vlad-lyutenko requested review from findepi, hashhar and wendigo July 27, 2023 15:00

ssheikin reviewed Jul 27, 2023

View reviewed changes

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java Outdated Show resolved Hide resolved

Praveen2112 reviewed Jul 28, 2023

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/GenericRewrite.java Outdated Show resolved Hide resolved

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch 2 times, most recently from 3143dc5 to 7321d87 Compare July 28, 2023 11:52

Praveen2112 reviewed Jul 28, 2023

View reviewed changes

...jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeEscapeWithCaseSensitivity.java Show resolved Hide resolved

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch 5 times, most recently from 865b0ea to 2f46f73 Compare July 28, 2023 14:17

Praveen2112 reviewed Jul 31, 2023

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch from 2f46f73 to 0e730a8 Compare July 31, 2023 09:23

vlad-lyutenko requested a review from Praveen2112 July 31, 2023 09:24

Praveen2112 reviewed Jul 31, 2023

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch from 0e730a8 to 5cf783e Compare July 31, 2023 12:39

Praveen2112 approved these changes Jul 31, 2023

View reviewed changes

hashhar reviewed Aug 1, 2023

View reviewed changes

...c/src/main/java/io/trino/plugin/jdbc/expression/RewriteLikeAndEscapeWithCaseSensitivity.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 1, 2023

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch 3 times, most recently from 1c1522e to dcda949 Compare August 1, 2023 11:24

kokosing reviewed Aug 1, 2023

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch from dcda949 to 7aaed5e Compare August 1, 2023 12:47

Praveen2112 approved these changes Aug 1, 2023

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch from 7aaed5e to c0d111e Compare August 1, 2023 13:28

hashhar approved these changes Aug 1, 2023

View reviewed changes

Add MySQL case sensitive collation varchar LIKE push down support

7a2300b

vlad-lyutenko force-pushed the vlad-lyutenko/mysql-like-pushdown branch from c0d111e to 7a2300b Compare August 1, 2023 15:43

Praveen2112 merged commit a470785 into trinodb:master Aug 2, 2023

github-actions bot added this to the 423 milestone Aug 2, 2023

colebow mentioned this pull request Aug 2, 2023

Add Trino 423 release notes #18496

Merged

findepi reviewed Dec 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MySQL LIKE predicate pushdown for varchar/nvarchar columns. #18441

MySQL LIKE predicate pushdown for varchar/nvarchar columns. #18441

vlad-lyutenko commented Jul 27, 2023

Praveen2112 Jul 31, 2023

Praveen2112 Jul 31, 2023

vlad-lyutenko Jul 31, 2023 •

edited

Loading

hashhar Aug 1, 2023

vlad-lyutenko Aug 1, 2023

vlad-lyutenko Aug 1, 2023 •

edited

Loading

vlad-lyutenko Aug 1, 2023

vlad-lyutenko Aug 1, 2023

hashhar Aug 1, 2023

hashhar Aug 1, 2023

Praveen2112 Aug 1, 2023

hashhar Aug 1, 2023

vlad-lyutenko Aug 1, 2023

kokosing Aug 1, 2023

vlad-lyutenko Aug 1, 2023

Praveen2112 left a comment

Praveen2112 Aug 1, 2023

vlad-lyutenko Aug 1, 2023

Praveen2112 commented Aug 2, 2023

findepi Dec 19, 2023

findepi Dec 19, 2023

MySQL LIKE predicate pushdown for varchar/nvarchar columns. #18441

MySQL LIKE predicate pushdown for varchar/nvarchar columns. #18441

Conversation

vlad-lyutenko commented Jul 27, 2023

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-lyutenko Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-lyutenko Aug 1, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-lyutenko Jul 31, 2023 •

edited

Loading

vlad-lyutenko Aug 1, 2023 •

edited

Loading