Skip to content

Refactor createProjectIfStatement #25215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Refactor createProjectIfStatement #25215

wants to merge 2 commits into from

Conversation

imsayari404
Copy link
Contributor

Description

to create processBatch_XX methods and call them rather than using a single large block of project_ invocations

Motivation and Context

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

aaneja and others added 2 commits May 28, 2025 15:58
@aaneja
Refactor createProjectIfStatement
140f433 
to generate `processBatch_XX` methods and invoke them instead of one giant block of `project_` invocations
@imsayari404
Added unit tests to validate projection batching behavior
03b149f
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label May 28, 2025
@imsayari404 imsayari404 requested a review from aaneja May 28, 2025 11:58
aaneja
aaneja reviewed May 28, 2025
View reviewed changes
presto-main-base/src/test/java/com/facebook/presto/sql/gen/TestCursorProcessorCompiler.java
PageFunctionCompiler functionCompiler = new PageFunctionCompiler(METADATA, 0);
ExpressionCompiler expressionCompiler = new ExpressionCompiler(METADATA, functionCompiler);

int projectionCount = 1500;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having two methods, let's make projectionCount a parameter and use a @DataProvider to inject this. This way we can test with different values like 1,10,1000,1500,5000

@aaneja aaneja marked this pull request as ready for review June 3, 2025 05:58
@aaneja aaneja requested a review from a team as a code owner June 3, 2025 05:58
@aaneja aaneja requested a review from jaystarshot June 3, 2025 05:58
@prestodb-ci prestodb-ci requested review from a team, ScrapCodes and infvg and removed request for a team June 3, 2025 05:58
@imsayari404 imsayari404 closed this Jun 3, 2025
@pratyakshsharma
Copy link
Contributor

Closed in favour of #25212

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaneja aaneja aaneja left review comments

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

@ScrapCodes ScrapCodes Awaiting requested review from ScrapCodes ScrapCodes was automatically assigned from prestodb/ibm-reviewers

@infvg infvg Awaiting requested review from infvg infvg was automatically assigned from prestodb/ibm-reviewers

Assignees
No one assigned
Labels
from:IBM PR from IBM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@imsayari404 @pratyakshsharma @aaneja @prestodb-ci