Support cast projection pushdown in oracle #22728

krvikash · 2024-07-18T17:08:22Z

Description

This PR adds support for cast projection pushdown for the Oracle connector. This uses the framework introduced in #22203 for cast projection pushdown.
Currently, this PR only adds support for the CHAR and VARCHAR cast pushdown, which has the following limitations:

Pushdown is not supported with Char having length > 500
Pushdown is not supported with Varchar having length > 1000
Pushdown is not supported with unbounded varchar
Pushdown is not supported with Char cast to Varchar

Release notes

(X) Release notes are required, with the following suggested text:

# Oracle
* Add support for cast project pushdown with Char and Varchar data type. ({issue}`22728`

ssheikin

lgtm

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteCast.java

ssheikin · 2024-07-18T17:23:03Z

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

+                new CastTestCase("char(10)", "varchar(1001)"),
+                new CastTestCase("char(10)", "varchar"),
+                new CastTestCase("varchar(10)", "varchar(1001)"),
+                new CastTestCase("varchar(10)", "varchar"),


It's pity. This is very common usecase.

can't we do something for varchars?

I think we can use TO_NCLOB to cast in varchar or varchar(... >1000 CHAR). https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/TO_NCLOB.html

Trying it out.

Apart from CAST pushdown, comparison with clob type was not supported for pushdown. So I have made the change to support clob type comparison pushdown.

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

ssheikin · 2024-07-18T17:29:10Z

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

+        assertJoinFullyPushedDown(session, leftTable, rightTable, "LEFT JOIN", "l.varchar_50 = r.varchar_100", Arrays.asList(1, 2));
+        assertJoinFullyPushedDown(session, leftTable, rightTable, "RIGHT JOIN", "l.varchar_50 = r.varchar_100", Arrays.asList(1, null));
+        assertJoinFullyPushedDown(session, leftTable, rightTable, "INNER JOIN", "l.varchar_50 = r.varchar_100", List.of(1));
+        assertJoinFullyPushedDown(session, leftTable, rightTable, "INNER JOIN", "r.varchar_100 = l.varchar_50", List.of(1));


why join criteria in this assertion differs from others?

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

ssheikin · 2024-07-18T17:34:43Z

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteCast.java

+                targetTypeHandle.get()));
+    }
+
+    private static Optional<JdbcTypeHandle> toJdbcTypeHandle(Type type)


This method is the meat of this PR.
And this is a main opportunity to improve for function pushdown.

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

wendigo · 2024-07-18T18:35:17Z

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

+
+        assertJoinFullyPushedDown(session, leftTable, rightTable, "LEFT JOIN", "l.varchar_50 = r.varchar_100", Arrays.asList(1, 2));
+        assertJoinFullyPushedDown(session, leftTable, rightTable, "RIGHT JOIN", "l.varchar_50 = r.varchar_100", Arrays.asList(1, null));
+        assertJoinFullyPushedDown(session, leftTable, rightTable, "INNER JOIN", "l.varchar_50 = r.varchar_100", List.of(1));


ssheikin · 2024-07-19T09:06:31Z

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

+                    new CastTestCase("integer", "decimal(15)"),
+                    new CastTestCase("integer", "decimal(10, 2)"),
+                    new CastTestCase("integer", "decimal(30, 2)"),
+                    new CastTestCase("smallint", "integer"),


Despite the complexity of the implementation
f4204f3#r1683242993
I guess these return types can be handled too. Could you please try?

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteStringComparison.java

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

ssheikin · 2024-07-19T09:59:43Z

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteStringComparison.java

+
+    private static String toNClob(String expression)
+    {
+        return "TO_NCLOB(%s)".formatted(expression);


What if during trino reads the remote DB the unsupported by trino type (some remote-specific type) was force-mapped to text,
later here it was capturedAs CharType or VarcharType ?
Will TO_NCLOB work correctly?
Maybe it's the case here too https://github.com/trinodb/trino/pull/22203/files#diff-510ce28e8ade87630227de284298df1cb44186f73bc9f2219e1f6ad6505d410aR66

removed this change. nclob cast pushdown is out of scope of this PR.

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteCast.java

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

Praveen2112 · 2024-07-19T10:28:34Z

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteStringComparison.java

+            if (!clobInfo[1]) {
+                secondArgument = toNClob(secondExpression);
+            }
+            return "DBMS_LOB.compare(%s, %s) %s 0".formatted(firstArgument, secondArgument, operator);


Is the DBMS_LOB is something which is provided by default on Oracle or is it some sort of a package which needs to be installed ? How would it work with collation ? What if the underlying bytes in CLOB has some invalid string pattern - in this case won't Trino and Oracle differ ?

removed this change. nclob comparison is out of scope of this PR.

Praveen2112 · 2024-07-19T10:29:55Z

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

+            }
+        }
+
+        private static List<CastTestCase> supportedCastTypePushdown()


This is same as parameterized tests ? Can we have an assertion which assert all types of join and we could pass the types as an argument to it ?

This is same as parameterized tests ?

Yes

Can we have an assertion which assert all types of join and we could pass the types as an argument to it ?

I have a test testAllJoinPushdownWithCast which covers all join but only with one data type. For other all data types I am verifying only with INNER JOIN. Should we test all data types with all join? Or one should be fine?

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteCast.java

krvikash · 2024-07-23T16:12:25Z

Thanks @ssheikin @Praveen2112 @marcinsbd @SemionPar for the review. Addressed comments. Removing pushdown support for nclob data type. This requires some more work.

Here is summary:
This PR only adds support for the VARCHAR and CHAR cast pushdown, which has the following limitations:

Pushdown is not supported with Char having length > 500
Pushdown is not supported with varchar having length > 1000
Pushdown is not supported with unbounded varchar

krvikash · 2024-07-25T07:34:57Z

Added AbstractRewriteCast in base-jdbc pacakge, so that other jdbc connector can support cast pushdown by implementing toJdbcTypeHandle and buildCast. Also I have the base test case in base-jdbc package.

There is test case enhancement required to cover this comment #22728 (comment) . I will create table using remote database instead of trino.

mosabua · 2024-09-04T21:17:30Z

Adding stale-ignore label since this looks like its very active but just stalled a bit.

krvikash · 2024-09-05T09:49:11Z

(Do not cast unnecessary with extra space padding when target char type has more length than source char type)

krvikash · 2024-09-05T09:49:39Z

(rebased with master)

krvikash · 2024-09-05T09:56:45Z

(CI fix)

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

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteCast.java

Praveen2112 · 2024-09-11T11:29:09Z

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleCastPushdownTest.java

+import static java.util.Arrays.asList;
+import static org.assertj.core.api.Assertions.assertThat;
+
+public abstract class BaseOracleCastPushdownTest


Do we need this abstraction ?

Abstraction will be useful in the scenario where we might want to verify cast pushdown for different query runner.

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

Praveen2112 · 2024-09-16T17:00:08Z

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

+            assertThatThrownBy(() -> getQueryRunner().execute("SELECT CAST(%s AS %s) FROM %s".formatted(testCase.sourceColumn(), testCase.castType(), leftTable())))
+                    .hasMessageMatching("(.*)Cannot cast (.*) to (.*)");


Does it thrown by Trino or by Oracle or by both ?

krvikash · 2024-09-17T20:41:53Z

Pushdown was giving incorrect result wrt Unicode character values and char data type column. So, For now in this PR I am removing char cast pushdown.

krvikash · 2024-09-17T20:47:54Z

(rebased with master)

krvikash · 2024-09-18T05:20:31Z

Pushdown was giving incorrect result wrt Unicode character values and char data type column. So, For now in this PR I am removing char cast pushdown.

The issue seems to be happening in this case: When casting char unicode value to varchar type.

SELECT CAST(c_char_unicode AS varchar(50)) FROM table1;

SELECT CAST(c_nchar_unicode AS varchar(50)) FROM table1;

mayankvadariya

Please evaluate a possibility of case-insensitive matching in Oracle when pushed down.

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

SemionPar · 2024-09-23T12:38:23Z

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

+    public void testCastFails()
+    {
+        for (CastTestCase testCase : failCast()) {
+            assertThatThrownBy(() -> getQueryRunner().execute("SELECT CAST(%s AS %s) FROM %s".formatted(testCase.sourceColumn(), testCase.castType(), leftTable())))


When this test fails, it doesn't say which condition has failed exactly. Do you think we could improve on that?
Took a quick look - as() or withFailMessage() does not seem to work, maybe custom satifies() would.

Thanks @SemionPar, I will take this as a follow up.

@krvikash Sure thing!

One more thought that crossed my mind: if cast fails at runtime with different message when pushed down, say:

Trino: Cannot cast DECIMAL(19, 2) '99999999999999999.99' to DECIMAL(10, 2) Teradata: [Error 2616] [SQLState 22003] Numeric overflow occurred during computation.

It would be good to be able to use testCastFails to test such cases, don't you think? We might want to add message customization to CastTestCase (or introduce FailCastTestCase) - consider this another improvement idea.

@SemionPar Here is the refactoring PR #23737

ssheikin · 2024-09-26T11:05:26Z

🎉

cla-bot bot added the cla-signed label Jul 18, 2024

krvikash requested review from ssheikin, Praveen2112, SemionPar, vlad-lyutenko and marcinsbd July 18, 2024 17:09

ssheikin reviewed Jul 18, 2024

View reviewed changes

wendigo reviewed Jul 18, 2024

View reviewed changes

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 1acef9d to f4204f3 Compare July 19, 2024 08:33

ssheikin reviewed Jul 19, 2024

View reviewed changes

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java Outdated Show resolved Hide resolved

ssheikin reviewed Jul 19, 2024

View reviewed changes

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteStringComparison.java Outdated Show resolved Hide resolved

ssheikin reviewed Jul 19, 2024

View reviewed changes

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteStringComparison.java Outdated Show resolved Hide resolved

ssheikin reviewed Jul 19, 2024

View reviewed changes

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteStringComparison.java Outdated Show resolved Hide resolved

ssheikin mentioned this pull request Jul 19, 2024

Pushdown CAST in JOIN criteria for PostgreSql #21939

Closed

ssheikin reviewed Jul 19, 2024

View reviewed changes

Praveen2112 reviewed Jul 19, 2024

View reviewed changes

marcinsbd reviewed Jul 22, 2024

View reviewed changes

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java Show resolved Hide resolved

SemionPar reviewed Jul 22, 2024

View reviewed changes

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/RewriteCast.java Outdated Show resolved Hide resolved

krvikash force-pushed the krvikash/oracle-cast-pushdown branch 5 times, most recently from fa267be to 77cf16e Compare July 23, 2024 16:09

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 77cf16e to e341765 Compare July 25, 2024 07:31

krvikash force-pushed the krvikash/oracle-cast-pushdown branch 2 times, most recently from b44eb71 to 424f81d Compare July 25, 2024 21:16

mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Sep 4, 2024

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 9fb7a98 to 26e9272 Compare September 5, 2024 09:32

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 26e9272 to 831320b Compare September 5, 2024 09:49

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 831320b to 4151d4c Compare September 5, 2024 09:56

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 4151d4c to 93b238f Compare September 5, 2024 10:05

Praveen2112 reviewed Sep 11, 2024

View reviewed changes

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 93b238f to 1d4bd4b Compare September 12, 2024 09:46

Praveen2112 approved these changes Sep 16, 2024

View reviewed changes

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 1d4bd4b to cedf6fb Compare September 17, 2024 20:39

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from cedf6fb to 97daf47 Compare September 17, 2024 20:46

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 97daf47 to 9a002a4 Compare September 18, 2024 04:48

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from 9a002a4 to b9cf9f3 Compare September 18, 2024 09:14

mayankvadariya approved these changes Sep 19, 2024

View reviewed changes

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

SemionPar reviewed Sep 23, 2024

View reviewed changes

Support char/varchar cast projection pushdown in oracle

69df024

krvikash force-pushed the krvikash/oracle-cast-pushdown branch from b9cf9f3 to 69df024 Compare September 25, 2024 10:22

Praveen2112 merged commit 1c3b9f8 into trinodb:master Sep 26, 2024
61 checks passed

krvikash deleted the krvikash/oracle-cast-pushdown branch September 26, 2024 09:41

github-actions bot added this to the 460 milestone Sep 26, 2024

mosabua mentioned this pull request Sep 27, 2024

Add Trino 460 release notes #23571

Merged

1 task

krvikash mentioned this pull request Oct 10, 2024

Cast pushdown test case refactoring #23737

Merged

		assertThatThrownBy(() -> getQueryRunner().execute("SELECT CAST(%s AS %s) FROM %s".formatted(testCase.sourceColumn(), testCase.castType(), leftTable())))
		.hasMessageMatching("(.)Cannot cast (.) to (.*)");

Support cast projection pushdown in oracle #22728

Support cast projection pushdown in oracle #22728

Conversation

krvikash commented Jul 18, 2024 • edited Loading

Description

Release notes

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

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

krvikash commented Jul 23, 2024

krvikash commented Jul 25, 2024

mosabua commented Sep 4, 2024

krvikash commented Sep 5, 2024

krvikash commented Sep 5, 2024

krvikash commented Sep 5, 2024

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

krvikash commented Sep 17, 2024

krvikash commented Sep 17, 2024

krvikash commented Sep 18, 2024

mayankvadariya 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

Choose a reason for hiding this comment

ssheikin commented Sep 26, 2024

krvikash commented Jul 18, 2024 •

edited

Loading