[Pinot Connector] Support null predicate push down for Pinot connector #15406

xiangfu0 · 2022-12-14T19:46:08Z

Description

Support null predicate push down for Pinot connector

Release notes

# Pinot Connector
* Support null predicate push down for Pinot connector

ebyhr · 2023-01-17T07:56:48Z

Trino community member looks facing this issue. Can you add a test so that we can start the review process?
https://trinodb.slack.com/archives/CGB0QHWSW/p1673940425810719

etadelta222 · 2023-03-15T17:54:47Z

Hi @xiangfu0 , could you add a test per the comment above from ebyhr?

gp-stackid · 2023-04-21T15:44:07Z

Any update? facing similar issue

xiangfu0 · 2023-12-01T18:50:23Z

Rebased and added a few tests

xiangfu0 · 2023-12-01T18:51:36Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotConnectorSmokeTest.java

-                .matches("VALUES (BIGINT '0')")
-                .isNotFullyPushedDown(FilterNode.class);
+                WHERE string_col IS NULL OR string_col = 'null'"""))
+                .matches("VALUES (BIGINT '1')")


This table has 'null' string in string_col, so pinot will return 1 row after pushing down.

Should the table config be checked for null handling enabled?

etadelta222 · 2024-01-05T19:26:53Z

@ebyhr looks ready to be reviewed :)

mosabua · 2024-01-11T23:13:21Z

@xiangfu0 @elonazoulay and @ebyhr can you continue the collaboration on this?

elonazoulay · 2024-01-11T23:37:45Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotConnectorSmokeTest.java

@@ -1727,6 +1728,18 @@ SELECT COUNT(*)
                WHERE double_col NOT IN (0.0, -16.33, -17.33)"""))
                .matches("VALUES (BIGINT '7')")
                .isNotFullyPushedDown(FilterNode.class);
+
+        assertThat(query("SELECT price, vendor FROM " + JSON_TABLE + " WHERE price IS NOT NULL"))


This table has null handling disabled, should it check for all values? and/or just use the count query below?

elonazoulay · 2024-01-11T23:40:36Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotConnectorSmokeTest.java

-                WHERE string_col IS NULL"""))
-                .matches("VALUES (BIGINT '0')")
-                .isNotFullyPushedDown(FilterNode.class);
+                WHERE string_col IS NULL OR string_col = 'null'"""))


Since null handling is enabled for alltypes should this be AND string_col = 'null' to verify null handling? i.e. this would have the same result as if the table had null handling disabled.

elonazoulay · 2024-01-11T23:45:28Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotConnectorSmokeTest.java

-                .matches("VALUES (BIGINT '11')")
-                .isNotFullyPushedDown(FilterNode.class);
+                WHERE string_col IS NOT NULL AND string_col != 'null'"""))
+                .matches("VALUES (BIGINT '10')")


would the behavior be different for a table with null handling disabled?

elonazoulay · 2024-01-11T23:47:16Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotConnectorSmokeTest.java

-                WHERE long_col IS NULL"""))
-                .matches("VALUES (BIGINT '0')")
-                .isNotFullyPushedDown(FilterNode.class);
+                WHERE long_col IS NULL OR long_col = 0"""))


Same here, should this be tested: if null handling is disabled, shouldn't IS NULL be always false, and if enabled it would be min value?

elonazoulay · 2024-01-11T23:48:19Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotConnectorSmokeTest.java

-                WHERE long_col IS NOT NULL"""))
-                .matches("VALUES (BIGINT '11')")
-                .isNotFullyPushedDown(FilterNode.class);
+                WHERE long_col IS NOT NULL AND long_col != 0"""))


Same here, maybe have an explicit test for null handling enabled vs disabled in a table w the same data, and check for MIN_VALUE and/or the default null value. lmk, can talk offline about it when you have some time.

elonazoulay · 2024-01-11T23:56:16Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotConnectorSmokeTest.java

+
+        assertThat(query("SELECT COUNT(*) FROM " + JSON_TABLE + " WHERE price IS NULL"))
+                .matches("VALUES (BIGINT '0')")
+                .isFullyPushedDown();


Maybe do this for a copy of the table w null handling enabled? Or you can choose a simpler table (hybrid is pretty simple).
Also: what is the behavior of a hybrid table where null handling is enabled for realtime but disabled for offline or vice versa?

elonazoulay · 2024-01-11T23:59:32Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

            return Optional.of(format("(%s != %s)", predicateArgument, predicateArgument));
        }
-        if (valueSet.isAll()) {


iirc this is related to apache/pinot#10600 and 601, since the connector supports 11 and 12.1 should this be put back until we upgrade to 1.0.0 (and remove support for all previous versions)?

elonazoulay · 2024-01-12T01:12:08Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

@@ -131,16 +130,11 @@ private static Optional<String> toPredicate(PinotColumnHandle pinotColumnHandle,
        String predicateArgument = pinotColumnHandle.isAggregate() ? pinotColumnHandle.getExpression() : quoteIdentifier(pinotColumnHandle.getColumnName());
        ValueSet valueSet = domain.getValues();
        if (valueSet.isNone()) {
-            verify(!domain.isNullAllowed(), "IS NULL is not supported due to different null handling semantics. See https://docs.pinot.apache.org/developers/advanced/null-value-support");
+            if (domain.isNullAllowed()) {


Should this be done only if the table has null handling enabled in the config? How would it know the difference between 'null', Long.MIN_VALUE, etc. vs a null value? Is that handled in v1.0.0?

github-actions · 2024-09-04T17:06:21Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-09-04T21:27:39Z

👋 @xiangfu0 and @elonazoulay could you continue work on this PR or should we close it?

github-actions · 2024-09-26T17:12:16Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

ebyhr · 2024-10-11T11:50:25Z

Closing as there was no response. Please feel free to reopen if you continue the work.

cla-bot bot added the cla-signed label Dec 14, 2022

xiangfu0 changed the title ~~Support pushdown pinot null predicate~~ Support null predicate push down for Pinot connector Dec 14, 2022

xiangfu0 marked this pull request as draft December 14, 2022 23:45

martint force-pushed the master branch from 40dbb4f to 0d73d10 Compare December 19, 2022 20:15

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

xiangfu0 force-pushed the support-null-check-predicate branch 6 times, most recently from 9b67866 to 86d406c Compare November 28, 2023 02:31

xiangfu0 marked this pull request as ready for review November 28, 2023 05:41

xiangfu0 requested a review from elonazoulay November 28, 2023 05:41

xiangfu0 changed the title ~~Support null predicate push down for Pinot connector~~ [Pinot Connector] Support null predicate push down for Pinot connector Nov 28, 2023

xiangfu0 requested a review from ebyhr December 1, 2023 18:50

xiangfu0 commented Dec 1, 2023

View reviewed changes

xiangfu0 force-pushed the support-null-check-predicate branch from 86d406c to abe5078 Compare December 1, 2023 22:31

Support pushdown pinot null predicate

e61f56a

xiangfu0 force-pushed the support-null-check-predicate branch from abe5078 to e61f56a Compare December 2, 2023 00:58

elonazoulay reviewed Jan 11, 2024

View reviewed changes

elonazoulay reviewed Jan 12, 2024

View reviewed changes

ebyhr assigned xiangfu0 Mar 4, 2024

ebyhr removed their request for review May 20, 2024 03:22

github-actions bot added the stale label Sep 4, 2024

github-actions bot removed the stale label Sep 5, 2024

github-actions bot added the stale label Sep 26, 2024

ebyhr closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pinot Connector] Support null predicate push down for Pinot connector #15406

[Pinot Connector] Support null predicate push down for Pinot connector #15406

xiangfu0 commented Dec 14, 2022 •

edited

Loading

ebyhr commented Jan 17, 2023

etadelta222 commented Mar 15, 2023

gp-stackid commented Apr 21, 2023

xiangfu0 commented Dec 1, 2023

xiangfu0 Dec 1, 2023

elonazoulay Jan 11, 2024 •

edited

Loading

etadelta222 commented Jan 5, 2024

mosabua commented Jan 11, 2024

elonazoulay Jan 11, 2024

elonazoulay Jan 11, 2024

elonazoulay Jan 11, 2024

elonazoulay Jan 11, 2024

elonazoulay Jan 11, 2024

elonazoulay Jan 11, 2024

elonazoulay Jan 11, 2024

elonazoulay Jan 12, 2024

github-actions bot commented Sep 4, 2024

mosabua commented Sep 4, 2024

github-actions bot commented Sep 26, 2024

ebyhr commented Oct 11, 2024

[Pinot Connector] Support null predicate push down for Pinot connector #15406

[Pinot Connector] Support null predicate push down for Pinot connector #15406

Conversation

xiangfu0 commented Dec 14, 2022 • edited Loading

Description

Release notes

ebyhr commented Jan 17, 2023

etadelta222 commented Mar 15, 2023

gp-stackid commented Apr 21, 2023

xiangfu0 commented Dec 1, 2023

Choose a reason for hiding this comment

elonazoulay Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

etadelta222 commented Jan 5, 2024

mosabua commented Jan 11, 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

github-actions bot commented Sep 4, 2024

mosabua commented Sep 4, 2024

github-actions bot commented Sep 26, 2024

ebyhr commented Oct 11, 2024

xiangfu0 commented Dec 14, 2022 •

edited

Loading

elonazoulay Jan 11, 2024 •

edited

Loading