Upgrade Pinot Connector Libraries to 0.8.0 #9098

elonazoulay · 2021-09-02T16:39:37Z

No description provided.

elonazoulay · 2021-09-02T16:41:04Z

Based on #7630 .

hashhar · 2021-09-02T17:52:10Z

Does the upgrade require the changes from #7630?
If not, let's extract it so that we can merge this quickly (and then rebase the other PR instead).

hashhar

First pass. Didn't review PinotSqlFormatter yet.

Would appreciate someone familiar with Pinot to also review the changes in DynamicTableBuilder (and PinotSqlFormatter).

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java

plugin/trino-pinot/pom.xml

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

hashhar · 2021-09-06T16:58:39Z

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

-        }
-        else {
-            groupByColumns = resolvePinotColumns(schemaTableName, request.getGroupBy().getExpressions(), columnHandles);
+        Optional<String> filter = Optional.empty();


The order of processing changed - filters before grouping. Was this intentional (I do think the operation doesn't matter but makes the change seem more involved than it actually was).

I reverted the order of processing so the functionality is the same as it was with the following caveat:
A passthrough query with grouping columns but no aggregations is parsed by pinot 0.8.0 differently than other queries: it wraps the grouping columns in a distinct(col1, col2, ...) select list, returns a null list of grouping columns and a "distinct" aggregation function which does not support the Pinot AggregatationFunction::getFinalResultColumnType method.

I addressed this but it seems better to include it in a separate pr, as the previous functionality is preserved with this pr and rewriting would change how distinct aggregations are handled. wdyt?

Makes sense, it's easier to review changes related to version bump separately from behaviour changes.
We can change the behaviour in follow-up PRs.

hashhar · 2021-09-06T17:04:12Z

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

            for (AggregationFunction aggregationFunction : queryContext.getAggregationFunctions()) {
                aggregateColumnsBuilder.add(new PinotColumnHandle(
                        aggregationFunction.getResultColumnName(),
                        toTrinoType(aggregationFunction.getFinalResultColumnType())));
            }
+            if (queryContext.getGroupByExpressions() != null) {


Why is this nested inside the aggregations block?
I think it's possible for queries to have GROUP BY without aggregations.

SELECT 1 FROM t GROUP BY c?

Let's add a test with this query:

Ensure that the plan contains a TableScan (so that we are sure that the connector participated in the query and the engine didn't do it all itself).

The results are correct.

I created #9167 to test this in BaseConnectorTest as well.

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

elonazoulay · 2021-09-08T20:05:10Z

Fixes #9133

hashhar

Let's extract until 5fc2a82 into a separate PR since those commits are all good and can be merged right now.

Let's also extract the below three into another PR since those look good and can be merged:
Add support for varbinary filters 93a80ca
Fix decoding real -INFINITY and +INFINITY e88b097
Fix decoding double -INFINITY and +INFINITY 82f2914

Let's extract "Fix in clause handling in filter pushdown e6ff226" into a separate PR (for the sake of release notes and people who'd want to look at the fix).

We can then focus on just the 0.8.0 upgrade commits here (which I still need to review some parts of).

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/decoders/DoubleDecoder.java

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

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

elonazoulay · 2021-09-09T14:05:58Z

Created #9178 for the first pr

elonazoulay · 2021-09-09T14:10:10Z

Created #9180 for the varbinary filter support + real/double fixes

elonazoulay · 2021-09-09T14:15:15Z

Pr for the in clause varchar handling: #9181

xiangfu0 · 2021-09-06T08:28:27Z

plugin/trino-pinot/pom.xml

+            <groupId>org.apache.pinot</groupId>
+            <artifactId>pinot-yammer</artifactId>
+            <version>${dep.pinot.version}</version>
+            <scope>runtime</scope>


how are we getting this during runtime?

shall we make a noop implementation for this pinot-metrics interface ?

how are we getting this during runtime?

Required by the PinotQueryClient which requires PinotMetricsFactory. Would it be ok to leave it, so that we can have broker metrics? The worker functions like a broker without a reducer service (soon we will put that in also). wdyt?

ebyhr · 2021-10-26T06:53:50Z

@elonazoulay Please separate a PR if subsequent commits Simplify logic for generating Pinot ColumnMetadata ... Cleanup various minor issues in the Pinot Connector aren't mandatory during library upgrade.

ddcprg · 2021-10-26T14:36:17Z

I wonder if anyone can let me know when this is expected to be released? thanks

elonazoulay · 2021-10-26T22:46:02Z

I removed them and pushed. Btw, the commit for handling null on empty group was reworded (and removed from this pr) - it fixes issues in #9137 - let me know if you would like me to restore the branch as it was.

elonazoulay · 2021-10-26T22:48:03Z

@ebyhr, I removed them and pushed. FYI, the commit for handling null on empty group was reworded to "Fix handling of complex aggregate expressions in passthrough queries" and removed from this pr - it fixes issues in #9137 - let me know if you would like me to add it back. If not I will create another pr for it now. Thanks!

elonazoulay · 2021-10-27T03:28:59Z

Looks like an unrelated failure.

ebyhr

Almost good to me. Let's update the minimum requirement from 0.1.0 to 0.8.0 in pinot.rst file.

plugin/trino-pinot/src/test/resources/quotes_in_column_name_realtimeSpec.json

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotColumnHandle.java

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

hashhar

For "Remove PinotColumn":

Maybe reword message to Remove PinotColumn and moved used methods to PinotColumnHandle instead. to clarify it was a mechanical refactor - nothing actually changed.

hashhar

For "Upgrade libraries to pinot-0.8.0"

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

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

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

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

hashhar

Move "Update supported pinot version in documentation" into the commit that updates the library version.

hashhar · 2021-11-01T08:47:40Z

LGTM % comments.

Sorry about the long back and forth for this PR.

This is groundwork for pinot-0.8.0 which uses PinotQuery in the broker request.

Remove PinotColumn and moved used methods to PinotColumnHandle instead

Use the QueryContext from the broker request to parse queries. Use FilterContext to parse filters in pinot-0.8.0. Pinot 0.8.0 supports native boolean types.

The offset is the first parameter in Pinot. Ensure that aggregations are not pushed down if there is an offset.

hashhar

Thanks @elonazoulay

hashhar · 2021-11-05T08:44:42Z

CI hit #8792 and #8911.

cla-bot bot added the cla-signed label Sep 2, 2021

elonazoulay requested review from martint, findepi, hashhar and xiangfu0 September 2, 2021 16:40

elonazoulay force-pushed the pinot-0.8.0 branch 5 times, most recently from 684cef2 to 7ebd38e Compare September 6, 2021 10:21

hashhar reviewed Sep 6, 2021

View reviewed changes

hashhar mentioned this pull request Sep 7, 2021

[Pinot connector] The IN condition does not work correctly for string values when using aggregate pushdown #9133

Closed

elonazoulay force-pushed the pinot-0.8.0 branch 2 times, most recently from aec35ea to 6151d53 Compare September 8, 2021 20:02

elonazoulay requested a review from hashhar September 9, 2021 00:31

hashhar mentioned this pull request Sep 9, 2021

Test aggregation queries that don't select any columns #9167

Open

hashhar reviewed Sep 9, 2021

View reviewed changes

elonazoulay force-pushed the pinot-0.8.0 branch 2 times, most recently from f4e02d2 to e9e0f10 Compare September 13, 2021 09:04

elonazoulay requested a review from hashhar September 13, 2021 09:04

xiangfu0 reviewed Sep 13, 2021

View reviewed changes

elonazoulay requested a review from xiangfu0 September 14, 2021 06:00

elonazoulay force-pushed the pinot-0.8.0 branch from e9e0f10 to d04ee23 Compare September 14, 2021 07:37

hashhar mentioned this pull request Sep 16, 2021

[Pinot Connector] Upgrade Pinot version to 0.8.0/0.9.0 #9258

Closed

elonazoulay mentioned this pull request Oct 22, 2021

[Pinot connector] Some pass-through queries give incorrect result or are failing #9137

Closed

elonazoulay force-pushed the pinot-0.8.0 branch from a594161 to 03f9d25 Compare October 26, 2021 22:44

elonazoulay mentioned this pull request Oct 26, 2021

Fix Handling of Aggregation Expressions in Pinot Passthrough Queries #9781

Merged

ebyhr reviewed Oct 27, 2021

View reviewed changes

elonazoulay force-pushed the pinot-0.8.0 branch 2 times, most recently from d3664a2 to f373703 Compare October 27, 2021 16:31

elonazoulay requested a review from ebyhr October 27, 2021 18:56

hashhar reviewed Nov 1, 2021

View reviewed changes

mosabua added the docs label Nov 3, 2021

elonazoulay added 3 commits November 3, 2021 23:12

Rename PinotQuery to PinotQueryInfo

0eb49f2

This is groundwork for pinot-0.8.0 which uses PinotQuery in the broker request.

Use JsonCodec when submitting broker request in Pinot

2f0275e

Remove PinotColumn

ad0a5d7

Remove PinotColumn and moved used methods to PinotColumnHandle instead

elonazoulay force-pushed the pinot-0.8.0 branch from d556a42 to 791d47f Compare November 4, 2021 06:59

elonazoulay requested a review from hashhar November 4, 2021 06:59

elonazoulay added 2 commits November 5, 2021 00:37

Upgrade libraries to pinot-0.8.0

1fb2ff2

Use the QueryContext from the broker request to parse queries. Use FilterContext to parse filters in pinot-0.8.0. Pinot 0.8.0 supports native boolean types.

Fix offset handling in passthrough queries

f02d3fe

The offset is the first parameter in Pinot. Ensure that aggregations are not pushed down if there is an offset.

elonazoulay force-pushed the pinot-0.8.0 branch from 791d47f to f02d3fe Compare November 5, 2021 07:37

hashhar approved these changes Nov 5, 2021

View reviewed changes

hashhar merged commit 4187472 into trinodb:master Nov 5, 2021

github-actions bot added this to the 365 milestone Nov 5, 2021

ebyhr mentioned this pull request Dec 2, 2021

Aggregation on non-lowercase column returns incorrect result in Pinot connector #10148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Pinot Connector Libraries to 0.8.0 #9098

Upgrade Pinot Connector Libraries to 0.8.0 #9098

elonazoulay commented Sep 2, 2021

elonazoulay commented Sep 2, 2021

hashhar commented Sep 2, 2021 •

edited

Loading

hashhar left a comment

hashhar Sep 6, 2021

elonazoulay Sep 8, 2021

hashhar Sep 16, 2021

hashhar Sep 6, 2021

elonazoulay Sep 8, 2021

hashhar Sep 9, 2021

hashhar Sep 9, 2021

elonazoulay commented Sep 8, 2021

hashhar left a comment

elonazoulay commented Sep 9, 2021

elonazoulay commented Sep 9, 2021

elonazoulay commented Sep 9, 2021

xiangfu0 Sep 6, 2021

xiangfu0 Sep 13, 2021

elonazoulay Sep 14, 2021

ebyhr commented Oct 26, 2021 •

edited

Loading

ddcprg commented Oct 26, 2021

elonazoulay commented Oct 26, 2021

elonazoulay commented Oct 26, 2021

elonazoulay commented Oct 27, 2021

ebyhr left a comment •

edited

Loading

hashhar left a comment

hashhar left a comment

hashhar left a comment

hashhar commented Nov 1, 2021

hashhar left a comment

hashhar commented Nov 5, 2021

Upgrade Pinot Connector Libraries to 0.8.0 #9098

Upgrade Pinot Connector Libraries to 0.8.0 #9098

Conversation

elonazoulay commented Sep 2, 2021

elonazoulay commented Sep 2, 2021

hashhar commented Sep 2, 2021 • edited Loading

hashhar 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

elonazoulay commented Sep 8, 2021

hashhar left a comment

Choose a reason for hiding this comment

elonazoulay commented Sep 9, 2021

elonazoulay commented Sep 9, 2021

elonazoulay commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Oct 26, 2021 • edited Loading

ddcprg commented Oct 26, 2021

elonazoulay commented Oct 26, 2021

elonazoulay commented Oct 26, 2021

elonazoulay commented Oct 27, 2021

ebyhr left a comment • edited Loading

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented Nov 1, 2021

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented Nov 5, 2021

hashhar commented Sep 2, 2021 •

edited

Loading

ebyhr commented Oct 26, 2021 •

edited

Loading

ebyhr left a comment •

edited

Loading