Skip to content

Conversation

@n-young-db
Copy link
Contributor

What changes were proposed in this pull request?

  • Add LocalLimitExec to SparkStrategies in Limit + Offset cases
  • Add UT

Why are the changes needed?

Originally, OffsetAndLimit and LimitAndOffset match cases were matching then dropping a LocalLimit node. Adds this LocalLimitExec node to the physical plan to improve efficiency. Note that this was not a correctness bug since not applying LocalLimit only leads to larger intermediate shuffles / nodes.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the SQL label Jan 12, 2024
@cloud-fan
Copy link
Contributor

also cc @beliefer

@n-young-db n-young-db requested a review from yaooqinn January 12, 2024 05:46
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @n-young-db and all.
Merged to master.

@dongjoon-hyun
Copy link
Member

Welcome to the Apache Spark community, @n-young-db .
I added you to the Apache Spark contributor group and assigned SPARK-46693 to you.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 13, 2024

BTW, @n-young-db , according to the commit log, SPARK-46274 is your contribution too, right?

[SPARK-46274][SQL] Fix Range operator computeStats() to check long validity before converting

It seems that it's assigned to someone-else currently. Is there any reason for this contributor mismatch, @n-young-db and @cloud-fan ?
Screenshot 2024-01-13 at 1 21 44 PM

@yaooqinn
Copy link
Member

late lgtm

Copy link
Contributor

@beliefer beliefer left a comment

Choose a reason for hiding this comment

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

Late LGTM.

@cloud-fan
Copy link
Contributor

@dongjoon-hyun thanks for catching it! it's a mistake, I've updated the assignee.

@n-young-db
Copy link
Contributor Author

@dongjoon-hyun I hadn't gotten my JIRA account yet so I had someone else create the ticket for me; thank you for catching and fixing this!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants