Upgrade Pinot Connector Libraries to 0.8.0 by elonazoulay · Pull Request #9098 · trinodb/trino

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

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

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

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 findepi, hashhar, martint 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

Conversation

elonazoulay commented Sep 2, 2021

Uh oh!

elonazoulay commented Sep 2, 2021

Uh oh!

hashhar commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elonazoulay commented Sep 8, 2021

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elonazoulay commented Sep 9, 2021

Uh oh!

elonazoulay commented Sep 9, 2021

Uh oh!

elonazoulay commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddcprg commented Oct 26, 2021

Uh oh!

elonazoulay commented Oct 26, 2021

Uh oh!

elonazoulay commented Oct 26, 2021

Uh oh!

elonazoulay commented Oct 27, 2021

Uh oh!

ebyhr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hashhar commented Sep 2, 2021 •

edited

Loading

ebyhr commented Oct 26, 2021 •

edited

Loading

ebyhr left a comment •

edited

Loading