Skip to content
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

[native] TopNRank optimization #24138

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[native] TopNRank optimization #24138

wants to merge 1 commit into from

Conversation

aditi-pandit
Copy link
Contributor

@aditi-pandit aditi-pandit commented Nov 25, 2024 •
edited
Loading

Description

https://docs.google.com/document/d/1WQfNigR9bVrbM-PqY7F0mswcetN_tdNahzD9ENye-Q0/edit?tab=t.0

Motivation and Context

TPC-DS Q67

== RELEASE NOTES ==

General Changes
* Enhance Window filter pushdown in native engine for rank and dense_rank functions :pr:`24138`

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Nov 25, 2024
@prestodb-ci prestodb-ci requested review from a team, BryanCutler and imjalpreet and removed request for a team November 25, 2024 10:54
@aditi-pandit aditi-pandit mentioned this pull request Nov 25, 2024
Open
@aditi-pandit aditi-pandit force-pushed the topn branch 2 times, most recently from b45c503 to ff8a3b5 Compare November 27, 2024 06:29
BryanCutler
BryanCutler reviewed Dec 11, 2024
View reviewed changes
Copy link
Contributor

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

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

LGTM just a couple questions

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/TopNRowNumberNode.java
@JsonProperty("rowNumberVariable") VariableReferenceExpression rowNumberVariable,
@JsonProperty("maxRowCountPerPartition") int maxRowCountPerPartition,
@JsonProperty("maxRowCountPerPartition") int maxRankPerPartition,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the json property be changed too or is that not possible without breaking compat?

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
@@ -495,6 +495,7 @@ public static Optional<BiFunction<Integer, URI, Process>> getExternalWorkerLaunc
// Write config file
String configProperties = format("discovery.uri=%s%n" +
"presto.version=testversion%n" +
"native-execution-enabled=true" +
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need a %n? I'm guessing that's to give it a newline but not sure

@steveburnett
Copy link
Contributor

Thank you for the release note! Some nits of phrasing and formatting suggestions.

== RELEASE NOTES ==

Prestissimo (Native Execution) Changes
* Improve Window filter pushdown in Prestissimo engine for ``rank`` and ``dense_rank`` functions. :pr:`24138`

@aaneja aaneja self-requested a review January 14, 2025 04:09
@aditi-pandit aditi-pandit mentioned this pull request Feb 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BryanCutler BryanCutler BryanCutler left review comments

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

@feilong-liu feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

@ClarenceThreepwood ClarenceThreepwood Awaiting requested review from ClarenceThreepwood ClarenceThreepwood is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

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

@aaneja aaneja Awaiting requested review from aaneja

At least 1 approving review is required to merge this pull request.

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
@aditi-pandit @steveburnett @BryanCutler @prestodb-ci