Handle NaN when pushing filter for JDBC connector #21923

Praveen2112 · 2024-05-10T15:18:19Z

Description

Fixes #21922
Some JDBC based sources like PotgreSql, Oracle, Ignite etc treats NaN values as equal,
and greater than all non-NaN values. This is different from Trino's behaviour where NaN values
are not equal, and they are not greater than or lesser than non-NaN values which results in in-correct
results when they are pushed down to the underlying datasource. So we push down additional condition
to ensure that NaN values are not considered in comparison operations.

Additional context and related issues

Release notes

( ) This is not user-visible or is 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`)

Praveen2112 · 2024-05-12T06:06:33Z

cc: @findepi I attempted a fix for it Please share your feedback for the same.

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClientModule.java

findepi · 2024-05-14T12:04:45Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/NaNSpecificQueryBuilder.java

+    @Override
+    protected String toPredicate(JdbcClient client, ConnectorSession session, JdbcColumnHandle column, JdbcTypeHandle jdbcType, Type type, WriteFunction writeFunction, String operator, Object value, Consumer<QueryParameter> accumulator)
+    {
+        if ((type == REAL || type == DOUBLE) && (operator.equals(">") || operator.equals(">="))) {


what about <, <=, =, !=?

I think we don't need this check of other operators as they work as expected - in case of Postgres NaN is placed about Infinity so we need to add them only for > or >= operator.

this should be a code comment.

when you write it down say "in PostgreSQL ..." in a generic class, you'll realize that you're exploiting a (common) implementation detail.

what does the spec say about double and NaN ordering? (i think the spec omits existence of NaNs, but i may be wrong)

even if the spec said something definitive, implementation vary (for example, Trino and PostgreSQL have different behavior). Which means the behavior should be implemented in connector-specific manner.

findepi · 2024-05-14T12:05:14Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/NaNSpecificQueryBuilder.java

+    {
+        if ((type == REAL || type == DOUBLE) && (operator.equals(">") || operator.equals(">="))) {
+            accumulator.accept(new QueryParameter(jdbcType, type, Optional.of(value)));
+            return format("((%s %s %s) AND (%s <> 'NaN'))", client.quoted(column.getColumnName()), operator, writeFunction.getBindExpression(), client.quoted(column.getColumnName()));


'NaN' as a varchar?
are you sure this is portable SQL?

From the tests 'NaN' is not parsed as a Varchar but considered as a NaN representation.

Since it is used along with double/real columns I think it is coerced automatically and the tests also proves the same.

This may work for PostgreSQL and may also work for some other databases, but I doubt this to be a really portable behavior (for example, Trino does not support comparing double values with 'NaN' literal, does it?) and this class is meant to be portable.

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

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

findepi · 2024-05-14T12:08:45Z

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

+    public void testSpecialValueOnApproximateNumericColumn()
+    {
+        assertThatThrownBy(super::testSpecialValueOnApproximateNumericColumn)
+                .hasMessageContaining("'NaN' is not a valid numeric or approximate numeric value");


looks wrong. why is it OK to skip this test for mysql?

findepi · 2024-05-14T12:09:07Z

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

@@ -246,6 +246,69 @@ public void testVarcharCharComparison()
        }
    }

+    @Test
+    @Override


why override? document

findepi · 2024-05-14T12:10:11Z

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

+                .map("$less_than_or_equal(left: exact_numeric_type, right: exact_numeric_type)").to("left <= right")
+                .map("$greater_than(left: exact_numeric_type, right: exact_numeric_type)").to("left > right")
+                .map("$greater_than_or_equal(left: exact_numeric_type, right: exact_numeric_type)").to("left >= right")
+                .map("$less_than(left: approx_numeric_type, right: approx_numeric_type)").to("((left < right) AND right <> 'NaN')")


braces around left < right are redundant
braces around whole replaced expression are also redundant

(braces are inserted automatically by rewrite engine)

findepi · 2024-05-14T12:10:38Z

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgresAwareQueryBuilder.java

@@ -69,6 +71,11 @@ protected String toPredicate(JdbcClient client, ConnectorSession session, JdbcCo
            return format("%s %s %s COLLATE \"C\"", client.quoted(column.getColumnName()), operator, writeFunction.getBindExpression());
        }

+        if ((type == REAL || type == DOUBLE) && (operator.equals(">") || operator.equals(">="))) {


what about other comparison operators?

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

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/NaNSpecificQueryBuilder.java

For equals and not equals rewrite we capture them as GenericRewrite expression and usage of RewriteComparison rule is redundant here.

Some JDBC based sources like PotgreSql, Oracle, Ignite etc treats NaN values as equal, and greater than all non-NaN values. This is different from Trino's behaviour where NaN values are not equal, and they are not greater than or lesser than non-NaN values which results in in-correct results when they are pushed down to the underlying datasource. So we push down additional condition to ensure that NaN values are not considered in comparison operations.

Praveen2112 · 2024-05-20T16:21:30Z

@findepi Thanks for the review. AC

findepi · 2024-05-21T09:03:23Z

First two commits lgtm. You can merge them.

cla-bot bot added the cla-signed label May 10, 2024

Praveen2112 force-pushed the praveen/nan_for_pg branch 5 times, most recently from 8e50017 to ded8441 Compare May 12, 2024 02:34

Praveen2112 added tests:all Run all tests snowflake Snowflake connector labels May 12, 2024

Praveen2112 force-pushed the praveen/nan_for_pg branch from ded8441 to 4391b08 Compare May 12, 2024 06:00

Praveen2112 marked this pull request as ready for review May 12, 2024 06:01

findepi reviewed May 14, 2024

View reviewed changes

Praveen2112 force-pushed the praveen/nan_for_pg branch 8 times, most recently from 1df9d6e to 0e5acfc Compare May 18, 2024 01:23

Praveen2112 changed the title ~~Handle NaN when pushing filter for Postgres connector~~ Handle NaN when pushing filter for JDBC connector May 18, 2024

Praveen2112 force-pushed the praveen/nan_for_pg branch 2 times, most recently from 22a1cf4 to 86a9211 Compare May 18, 2024 04:05

findepi reviewed May 20, 2024

View reviewed changes

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteClient.java Show resolved Hide resolved

findepi reviewed May 20, 2024

View reviewed changes

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

Praveen2112 force-pushed the praveen/nan_for_pg branch from 86a9211 to d3db6cc Compare May 20, 2024 15:48

Praveen2112 added 3 commits May 20, 2024 21:19

Use QueryBuilder injected by Guice in H2 connector

263994a

Remove redundant usage of RewriteComparison rule

17ab0f5

For equals and not equals rewrite we capture them as GenericRewrite expression and usage of RewriteComparison rule is redundant here.

Praveen2112 force-pushed the praveen/nan_for_pg branch from d3db6cc to 1caffaf Compare May 20, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle NaN when pushing filter for JDBC connector #21923

Handle NaN when pushing filter for JDBC connector #21923

Praveen2112 commented May 10, 2024 •

edited

Praveen2112 commented May 12, 2024

findepi May 14, 2024

Praveen2112 May 17, 2024

findepi May 21, 2024

findepi May 14, 2024

Praveen2112 May 17, 2024

Praveen2112 May 17, 2024

findepi May 21, 2024

findepi May 14, 2024

findepi May 14, 2024

findepi May 14, 2024

findepi May 14, 2024

Praveen2112 commented May 20, 2024

findepi commented May 21, 2024

Handle NaN when pushing filter for JDBC connector #21923

Are you sure you want to change the base?

Handle NaN when pushing filter for JDBC connector #21923

Conversation

Praveen2112 commented May 10, 2024 • edited

Description

Additional context and related issues

Release notes

Praveen2112 commented May 12, 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

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 commented May 20, 2024

findepi commented May 21, 2024

Praveen2112 commented May 10, 2024 •

edited