Fix Handling of Aggregation Expressions in Pinot Passthrough Queries#9781
Merged
hashhar merged 5 commits intotrinodb:masterfrom Mar 19, 2022
Merged
Conversation
This was referenced Oct 26, 2021
Contributor
|
is there any update to this PR? my org would find this pretty useful |
Member
Author
|
Yes, will be updating shortly. |
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
from
9d77b77 to
8436f89
Compare
Member
Author
|
Fixes #10148 |
|
Any chance this can get into the 368 release? |
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
3 times, most recently
from
fa2dac2 to
aea36fd
Compare
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
2 times, most recently
from
8178c73 to
56d842e
Compare
findepi
reviewed
Mar 14, 2022
Member
There was a problem hiding this comment.
context.getIdentifierQuote() is identity, right? is it OK?
Member
Author
There was a problem hiding this comment.
It's ok: the DynamicTablePqlExtractor quotes identifiers.
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
from
56d842e to
45a8c8b
Compare
hashhar
reviewed
Mar 15, 2022
Member
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
LGTM "Simplify logic for generating Pinot ColumnMetadata"
hashhar
reviewed
Mar 15, 2022
Member
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
"Fix handling of complex aggregate expressions in Pinot passthrough queries".
(just over length limit commit message, reword)
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/DynamicTableBuilder.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/DynamicTableBuilder.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/DynamicTableBuilder.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
hashhar
previously approved these changes
Mar 15, 2022
Member
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
"Cleanup various minor issues in the Pinot Connector" - reword to indicate these are not "issues" - just refactors.
hashhar
reviewed
Mar 15, 2022
Member
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
"Fix Pinot aggregate functions with mixed case columns" -> mixed case column names
Some comments, mostly stylistic + comment from Piotr already about the identifier quote - even though pre-existing. IIUC the quote here may not matter since Pinot library performs quoting of literal values for us - but not sure about column names.
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
from
9f62e75 to
dac3cd5
Compare
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
from
dac3cd5 to
2e2e979
Compare
hashhar
reviewed
Mar 16, 2022
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
hashhar
reviewed
Mar 16, 2022
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/DynamicTableBuilder.java
Outdated
Show resolved
Hide resolved
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
from
2e2e979 to
cfb4ad7
Compare
Directly create ColumnMetadata from Pinot FieldSpecs. Create PinotColumnHandles from ColumnMetadata in getColumnHandles. Previously a PinotColumnHandle was created which would be turned into ColumnMetadata and then converted back to PinotColumnHandle.
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
from
cfb4ad7 to
fd8d1c6
Compare
hashhar
reviewed
Mar 17, 2022
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestDynamicTable.java
Outdated
Show resolved
Hide resolved
Remove unused methods. Remove unused parameters in methods. Fix warnings in TestDynamicTable.
elonazoulay
force-pushed
the
pinot-0.8.0-aggregation_expressions
branch
from
fd8d1c6 to
090ce8a
Compare
hashhar
approved these changes
Mar 19, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also includes some minor code cleanup.
Based off of #9098