Skip to content

Handle Pinot Project Expressions - SWITCH#17800

Merged
highker merged 1 commit intoprestodb:masterfrom
cliandy:pinot-project-expressions
Jun 16, 2022
Merged

Handle Pinot Project Expressions - SWITCH#17800
highker merged 1 commit intoprestodb:masterfrom
cliandy:pinot-project-expressions

Conversation

@cliandy
Copy link
Copy Markdown
Contributor

@cliandy cliandy commented May 25, 2022 •
edited by highker
Loading

Support Presto switch statement pushdown to Pinot. The motivation for this is to allow for less data transfer when working with multicolumn projections like CASE statements, other functionality, and leading up to supporting COALESCE once NULL can be properly handled.

Enable this feature by setting pinot.pushdown_project_expressions=true

Test plan

  • Modified Unit Tests
  • Explain Queries (i.e.)
GeneratedPinotQuery{query=SELECT CASE true WHEN ("numberOfGames" > "numberOfGamesAsBatter") THEN "playerName" ELSE "teamID" END FROM baseballStats__TABLE_NAME_SUFFIX_TEMPLATE____TIME_BOUNDARY_FILTER_TEMPLATE__ LIMIT 2147483647

== RELEASE NOTES ==

Pinot Changes
* Add support for `CASE` statement pushdown to Pinot
* Add support for logical and arithmetic functions for `CASE` statements.

@cliandy cliandy requested a review from a team as a code owner May 25, 2022 15:43
@cliandy cliandy requested a review from NikhilCollooru May 25, 2022 15:43
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented May 25, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: cliandy / name: Andy Li (63372ca61a48e39160d1cb13bde19a4cb7f976ff)

@cliandy
Copy link
Copy Markdown
Contributor Author

cliandy commented May 25, 2022

@xiangfu0 let me know your thoughts when you get a chance

xiangfu0
xiangfu0 reviewed May 25, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@xiangfu0 xiangfu0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM!
Some generated query details should be tested for pinot syntax, mostly negation and not equal.

...t-toolkit/src/main/java/com/facebook/presto/pinot/query/PinotProjectExpressionConverter.java Outdated
Copy link
Copy Markdown
Contributor

@xiangfu0 xiangfu0 May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinot uses != for not equal, you may need to convert it when generating a pinot query.
Or you can use the map to make it generic just like this PRESTO_TO_PINOT_OPERATORS.

Copy link
Copy Markdown
Contributor Author

@cliandy cliandy Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding here is that based on the calcite spec, <> and != are both okay with != permissible based on the level of conformance, which is currently at BABEL. Tested <> and this runs well, so curious where this is defined internally?

...t-toolkit/src/main/java/com/facebook/presto/pinot/query/PinotProjectExpressionConverter.java Outdated
Copy link
Copy Markdown
Contributor

@xiangfu0 xiangfu0 May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give some examples/test cases for these negation cases?
Will the negation sign just work or the NOT statement is required?

Copy link
Copy Markdown
Contributor Author

@cliandy cliandy Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these in particular are for arithmetic expressions, so we wouldn't go down this path for an expression with a NOT statement.

@cliandy
Copy link
Copy Markdown
Contributor Author

cliandy commented Jun 6, 2022

@NikhilCollooru can you take a look at this when you get a chance? Thanks!

@NikhilCollooru NikhilCollooru requested a review from highker June 9, 2022 22:44
highker
highker approved these changes Jun 10, 2022
View reviewed changes
Copy link
Copy Markdown

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you squash two commits into one?

@highker highker self-assigned this Jun 10, 2022
@cliandy cliandy force-pushed the pinot-project-expressions branch 2 times, most recently from 98aa061 to a5d1995 Compare June 10, 2022 23:27
@cliandy
Copy link
Copy Markdown
Contributor Author

cliandy commented Jun 10, 2022

@highker let me know if there are any other concerns, thanks!

@highker
Copy link
Copy Markdown

highker commented Jun 13, 2022

Test failure is related

@highker
Copy link
Copy Markdown

highker commented Jun 14, 2022

Could squash all commits into one?

@cliandy
Handle Pinot Project Expressions
8780d49 
- handle SWITCH statement pushdown
- add additional pushdown operators
- properly handle negation
@cliandy cliandy force-pushed the pinot-project-expressions branch from 17563bb to 8780d49 Compare June 16, 2022 00:32
@highker highker merged commit e429cb8 into prestodb:master Jun 16, 2022
@highker highker mentioned this pull request Jul 6, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@highker highker highker approved these changes

@NikhilCollooru NikhilCollooru Awaiting requested review from NikhilCollooru NikhilCollooru is a code owner automatically assigned from prestodb/committers

+1 more reviewer

@xiangfu0 xiangfu0 xiangfu0 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@highker highker

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cliandy @highker @xiangfu0