Implement more connector expression pushdowns in SQL Server #14570

vlad-lyutenko · 2022-10-11T13:41:57Z

Description

Adding support for SqlServer predicate modulo pushdown,
Also added test in BaseJdbcConnectorTest, which can be used by overriding hasBehavior
method in any connector

Non-technical explanation

Release notes

(x) 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`)

vlad-lyutenko · 2022-10-11T13:43:51Z

Initial work on predicate modulo push down support,
to discuss:

should we enable modulo push down tests for other connectors like Postgres in this PR?
what other connectors most require this changes and should I refactor them in this PR?
e.t.c

ssheikin

lgtm, however my opinion is not authoritative enough here.

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

findepi · 2022-10-12T06:43:59Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+    @Test
+    public void testPredicateModuloPushdown()
+    {
+        if (!hasBehavior(SUPPORTS_PREDICATE_MODULO_PUSHDOWN)) {


Due to high number for connector behaviors, it's imperative that these declarations are self-tested. I.e. both "supports" and "does not support" should be verified. Otherwise, connector implementors will not know that they need to declare that give behavior is supported and it will not be tested.

Concrete example: PostgreSQL supports modulo pushdown, and automation should remind us to declare that in the PostgreSQL's connector test.

added

if (!hasBehavior(SUPPORTS_PREDICATE_ARITHMETIC_EXPRESSION_PUSHDOWN)) { assertThat(query("SELECT shippriority FROM orders WHERE shippriority % 4 = 0")).isNotFullyPushedDown(FilterNode.class); return; }

But didn't get fully what should be the node, I took for now FilterNode,
but can you please clarify is it correct way

The easiest way is to change to isFullyPushedDown, see what the error shows the plan looks like and add the node (or sub-tree) just above TableScan as argument to isNotFullyPushedDown.

e.g. if connector doesn't support LIMIT pushdown but supports aggregation pushdown then a query like SELECT regionkey, sum(nationkey) FROM (SELECT * FROM nation WHERE regionkey < 2 LIMIT 11) GROUP BY regionkey will have plan like:

... LimitNode ... -> TableScanNode

That can be represented as isNotFullyPushedDown(LimitNode.class) or to be more precise isNotFullyPushedDown(node(LimitNode.class, anyTree(node(TableScanNode.class)))) (i.e. a LimitNode followed by 1 or more nodes followed by a TableScanNode at the end).

The easiest way is to change to isFullyPushedDown, see what the error shows the plan looks like and add the node (or sub-tree) just above TableScan as argument to isNotFullyPushedDown

I made this test and see that we have ScanFilter in plan
So looks like isNotFullyPushedDown(FilterNode.class); is correct

I cannot find exact ScanFilter.class node, could I assume that FilterNode is the same here?

Yes, FilterNode is correct here.

ScanFilter = FilterNode + TableScanNode
ScanFilterProject = ProjectNode + FilterNode + TableScanNode.

findepi · 2022-10-12T06:44:24Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+        if (!hasBehavior(SUPPORTS_PREDICATE_MODULO_PUSHDOWN)) {
+            return;
+        }
+        //modulo over bigint


style: we put a space after // comment start

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

wendigo · 2022-10-18T13:33:11Z

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

@@ -46,6 +47,13 @@ public Optional<String> rewrite(Constant constant, Captures captures, RewriteCon
        if (slice == null) {
            return Optional.empty();
        }
+
+        boolean isAscii = CharMatcher.ascii().matchesAllOf(slice.toStringUtf8());


We are constructing String from Slice twice here. Let's do it once instead.

Also we should invert the logic - let's test if there is at least single non ascii character.

wendigo · 2022-10-18T14:01:19Z

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

@@ -48,12 +48,13 @@ public Optional<String> rewrite(Constant constant, Captures captures, RewriteCon
            return Optional.empty();
        }

-        boolean isAscii = CharMatcher.ascii().matchesAllOf(slice.toStringUtf8());
+        String sliceUtf8String = slice.toStringUtf8();
+        boolean isNonAscii = !CharMatcher.ascii().matchesAllOf(sliceUtf8String);



CharMatcher.ascii().negate().precomputed().matchesAnyOf(sliceUtf8String)

Move CharMatcher instance to a const.

And let's rename it to isUnicodeString

I looked at implementation, and looks like it's not working as we expect in more performance way( like as soon it will find non ascii char, it stops processing and return), instead it just use negation of match all :(

There is Slice.isAscii static method. Let's use that

Sorry, can't find it, I see only nonstatic method slice.toStringAscii()

maybe CharUtils.isAscii ? from commons-lang

hashhar · 2022-10-19T07:50:10Z

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

+        if (isUnicodeString) {
+            return Optional.of("N'" + sliceUtf8String.replace("'", "''") + "'");
+        }
+
+        return Optional.of("'" + sliceUtf8String.replace("'", "''") + "'");


this shouldn't be in trino-base-jdbc - it's specific to SQL Server.

Looks like it's part of SQL-92 standard and SQL server is required to use it, but all other support it (but not required), like MySQL, Postgres, DB-2 e.t.c, so should we move only for SQL Server part?

is the N string interpretation same across databases?

This what I found about N prefix:
The "N" prefix stands for National Language in the SQL-92 standard, and is used for representing Unicode characters
While most databases do not need the added N prefix when using Unicode data, in SQL Server you must precede all Unicode strings with a prefix N when dealing with Unicode string constants.

All other supported backends work with or without the N prefix - the N prefix is not required in those environments but does work if used.

Not all databases / query engines support N prefix. For example, does Trino support it?

Supporting it is one thing - but the bigger question is do escapes get handled the same across databases.

From an older offline discussion my conclusion was it might make more sense to make ConnectorExpression pushdown use prepared statements which would make handling this (and other cases) easier.

In case we are able to identify cases where the N behaviour differs across databases then IMO we should remove all non-arithmetic pushdowns from this PR and revisit them when we have better mechanisms to support them.

So maybe we can move for now this N support as you suggested previously only to SQL Server?

@hashhar I tried implement today the path, when we remove all non-arithmetic pushdowns
but the problem is, arithmetics push down will work only if we have such rule:

.add(new RewriteComparison(ImmutableSet.of(RewriteComparison.ComparisonOperator.EQUAL, RewriteComparison.ComparisonOperator.NOT_EQUAL)))

But if add such rule, equal/not equal push down start work even for simple cases with varchars like failing one:
WHERE variable = 'łąka for the win'

So looks like we need this UnicodeConstantRewrite rule, if we want arithmetics pushdown, but we can keep everything only in SQL Server

Let's keep it only in SQL Server for now in that case until we have better ways to implement connector expression to SQL conversion + more test coverage.

The problem seems to be that there is no way to "prevent a connector expression pushdown" - we can only add more rules. i.e. we cannot say allow RewriteComparision but only if operands are integer types (we can but it'd require writing entirely new rule).

The problem seems to be that there is no way to "prevent a connector expression pushdown" - we can only add more rules. i.e. we cannot say allow RewriteComparision but only if operands are integer types (we can but it'd require writing entirely new rule).

exactly, I tried to made it, but with no luck

kokosing · 2022-10-26T09:41:53Z

@hashhar @wendigo @findepi @ssheikin Can you please resume the review? @vlad-lyutenko says that all yours concerns were addressed.

findepi · 2022-10-26T10:23:17Z

@hashhar @wendigo @findepi @ssheikin Can you please resume the review? @vlad-lyutenko says that all yours concerns were addressed.

Happy to.

from my perspective it would be helpful to squash the commits, as I'll read anew.
For example, i still see Support predicate modulo pushdown for SqlServer commit, even though I think we wanted arithmetics as a whole, not singling out the modulo (which is likely least interesting anyway).

kokosing · 2022-10-26T10:37:53Z

@vlad-lyutenko It is also a good practice to make sure that CI is happy with changes you provide. It could be a case that you have a serious bug that may affect in big refactor.
If CI issues are because of flakiness, please report flaky tests as github issues or add a comment to existing issues. If you have some time, consider to fix flaky issues too (as separate PR :)).
:)

vlad-lyutenko · 2022-10-26T10:43:27Z

@hashhar @wendigo @findepi @ssheikin Can you please resume the review? @vlad-lyutenko says that all yours concerns were addressed.

Happy to.

from my perspective it would be helpful to squash the commits, as I'll read anew. For example, i still see Support predicate modulo pushdown for SqlServer commit, even though I think we wanted arithmetics as a whole, not singling out the modulo (which is likely least interesting anyway).

commits squashed

hashhar · 2022-10-26T18:00:40Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+        assertThat(query("SELECT shippriority FROM orders WHERE shippriority % 4 = 0")).isFullyPushedDown();
+
+        assertThat(query("SELECT nationkey, name, regionkey FROM nation WHERE nationkey > 0 AND (nationkey - regionkey) % nationkey = 2"))
+                .isFullyPushedDown()
+                .matches("VALUES (BIGINT '3', CAST('CANADA' AS varchar(25)), BIGINT '1')");
+
+        // some databases calculate remainder instead of modulus when one of the values is negative
+        assertThat(query("SELECT nationkey, name, regionkey FROM nation WHERE nationkey > 0 AND (nationkey - regionkey) % -nationkey = 2"))
+                .isFullyPushedDown()
+                .matches("VALUES (BIGINT '3', CAST('CANADA' AS varchar(25)), BIGINT '1')");
+
+        assertThatThrownBy(() -> query("SELECT nationkey, name, regionkey FROM nation WHERE nationkey > 0 AND (nationkey - regionkey) % 0 = 2"))
+                .hasMessageContaining("by zero");
+
+        // Expression that evaluates to 0 for some rows on RHS of modulus
+        assertThatThrownBy(() -> query("SELECT nationkey, name, regionkey FROM nation WHERE nationkey > 0 AND (nationkey - regionkey) % (regionkey - 1) = 2"))
+                .hasMessageContaining("by zero");


nit: I'd probably extract these to a separate method (not @Test) so that it's easy to group related things togethers and for other connectors to override easily if needed.

Also add TODO to add coverage for other arithmetic pushdowns + create a issue.

Issue created #14808, TODO added,
but I didn't got idea with method extraction

Add a protected static void moduloPushdownTestCases() { ... } and move the modulo related assertions to that method and call that method from this test.

That way subclasses can override just modulo pushdown if needed. But maybe it's premature optimization - so feel free to ignore for now.

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

hashhar · 2022-10-26T18:06:46Z

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

+        if (isUnicodeString) {
+            return Optional.of("N'" + sliceUtf8String.replace("'", "''") + "'");
+        }
+
+        return Optional.of("'" + sliceUtf8String.replace("'", "''") + "'");


Supporting it is one thing - but the bigger question is do escapes get handled the same across databases.

From an older offline discussion my conclusion was it might make more sense to make ConnectorExpression pushdown use prepared statements which would make handling this (and other cases) easier.

In case we are able to identify cases where the N behaviour differs across databases then IMO we should remove all non-arithmetic pushdowns from this PR and revisit them when we have better mechanisms to support them.

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

vlad-lyutenko · 2022-10-28T09:24:12Z

Created new flaky test issue
#14814

kokosing · 2022-10-28T09:47:41Z

...t-tests/src/test/java/io/trino/faulttolerant/sqlserver/BaseSqlServerFailureRecoveryTest.java

+    {
+        // because we have arithmetic push down, now we will get exception not on trino side, but from sql server,
+        // so error message will be different
+        assertThatThrownBy(() -> getQueryRunner().execute("SELECT * FROM nation WHERE regionKey / nationKey - 1 = 0"))


pass a session in a base test to disable the predicate pushdown. Then you don't need to override the test.

kokosing · 2022-10-28T09:48:31Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java

@@ -406,13 +406,6 @@ public void testShowCreateTable()
                        ")");
    }

-    @Override
-    public void testDeleteWithLike()


hashhar

LGTM % comments.

...n/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/RewriteUnicodeVarcharConstant.java

hashhar · 2022-10-28T10:48:47Z

...n/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/RewriteUnicodeVarcharConstant.java

+        Slice slice = (Slice) constant.getValue();
+        if (slice == null) {
+            return Optional.empty();
+        }


A stupid question maybe but won't this be caught by the null check above?

The topmost check was added in 15839ea.

The cast check was added in 2c5ce56.

I don't understand.

to be honest it taken from RewriteVarcharConstant rule, and I was not brave enough to touch this code

Yeah, it's a question for @findepi and @wendigo to answer. No change requested here - it's pre-existing code. 😄

Secondary check seems redundant 😄

...n/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/RewriteUnicodeVarcharConstant.java

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

.../trino-faulttolerant-tests/src/test/java/io/trino/faulttolerant/BaseFailureRecoveryTest.java

hashhar · 2022-10-28T11:29:30Z

Oh, also reminder to squash commits before merging since all of them depend on each other and it's single logical change.

Test added in BaseJdbcConnectorTest and can be used in any connector using hasBehavior override

hashhar · 2022-10-31T11:05:17Z

...n/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/RewriteUnicodeVarcharConstant.java

+        if (isUnicodeString) {
+            return Optional.of("N'" + sliceUtf8String.replace("'", "''") + "'");
+        }
+
+        return Optional.of("'" + sliceUtf8String.replace("'", "''") + "'");


what happens if we unconditionally use N'<literal>'? Seems like it'll still be valid - no need to bother with CharMatcher which can be slow when matching larger values.

hashhar

LGTM

cla-bot bot added the cla-signed label Oct 11, 2022

vlad-lyutenko requested review from hashhar, findepi, wendigo, ssheikin and mx123 October 11, 2022 13:44

ssheikin reviewed Oct 11, 2022

View reviewed changes

findepi reviewed Oct 12, 2022

View reviewed changes

hashhar reviewed Oct 12, 2022

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Show resolved Hide resolved

hashhar mentioned this pull request Oct 12, 2022

Add more tests for modulo pushdown in PostgreSQL #12104

Merged

hashhar reviewed Oct 13, 2022

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Show resolved Hide resolved

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch 3 times, most recently from e68e5dc to 9198aad Compare October 14, 2022 11:20

hashhar changed the title ~~SqlServer predicate modulo pushdown support~~ Implement more connector expression pushdowns in SQL Server Oct 17, 2022

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from 1fba516 to 6b6e75b Compare October 18, 2022 12:07

wendigo reviewed Oct 18, 2022

View reviewed changes

hashhar reviewed Oct 19, 2022

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from 9b86729 to 91527c1 Compare October 26, 2022 10:42

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from 8b7411f to 4924e3c Compare October 26, 2022 15:25

hashhar reviewed Oct 26, 2022

View reviewed changes

vlad-lyutenko commented Oct 27, 2022

View reviewed changes

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java Show resolved Hide resolved

vlad-lyutenko mentioned this pull request Oct 27, 2022

Add coverage for other arithmetic pushdowns in BaseJdbcConnectorTest #14808

Open

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from 73e0445 to 47acd02 Compare October 27, 2022 15:21

kokosing reviewed Oct 28, 2022

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from 47acd02 to a0ab098 Compare October 28, 2022 10:33

hashhar reviewed Oct 28, 2022

View reviewed changes

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from a0ab098 to 1c4a1fe Compare October 28, 2022 11:04

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from 1c4a1fe to ebf34d6 Compare October 28, 2022 13:03

hashhar approved these changes Oct 31, 2022

View reviewed changes

Support arithmetics predicate pushdown for SqlServer

fb0d057

Test added in BaseJdbcConnectorTest and can be used in any connector using hasBehavior override

vlad-lyutenko force-pushed the vlad-lyutenko/sqlserver-modulo-pushdown branch from ebf34d6 to fb0d057 Compare October 31, 2022 10:53

hashhar reviewed Oct 31, 2022

View reviewed changes

vlad-lyutenko mentioned this pull request Oct 31, 2022

Flaky testAggregation in Pinot smoke test #14239

Open

hashhar approved these changes Oct 31, 2022

View reviewed changes

hashhar merged commit 5c65ffd into trinodb:master Oct 31, 2022

github-actions bot added this to the 402 milestone Oct 31, 2022

hashhar mentioned this pull request Oct 31, 2022

Release notes for 402 #14780

Closed

colebow mentioned this pull request Nov 1, 2022

Add Trino 402 release notes #14862

Merged

Implement more connector expression pushdowns in SQL Server #14570

Implement more connector expression pushdowns in SQL Server #14570

Conversation

vlad-lyutenko commented Oct 11, 2022

Description

Non-technical explanation

Release notes

vlad-lyutenko commented Oct 11, 2022 • edited Loading

ssheikin 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

vlad-lyutenko Oct 27, 2022 • 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

vlad-lyutenko Oct 18, 2022 • 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

Choose a reason for hiding this comment

vlad-lyutenko Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Oct 26, 2022

findepi commented Oct 26, 2022

kokosing commented Oct 26, 2022

vlad-lyutenko commented Oct 26, 2022

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

vlad-lyutenko commented Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar Oct 28, 2022 • 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

hashhar commented Oct 28, 2022

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

vlad-lyutenko commented Oct 11, 2022 •

edited

Loading

vlad-lyutenko Oct 27, 2022 •

edited

Loading

vlad-lyutenko Oct 18, 2022 •

edited

Loading

vlad-lyutenko Oct 27, 2022 •

edited

Loading

vlad-lyutenko commented Oct 28, 2022 •

edited

Loading

hashhar Oct 28, 2022 •

edited

Loading