Skip to content

[SPARK-19355][SQL][Followup] Remove the child.outputOrdering check in global limit #22239

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 1 commit into from
Closed

[SPARK-19355][SQL][Followup] Remove the child.outputOrdering check in global limit #22239

wants to merge 1 commit into from

Conversation

@viirya
Copy link
Member

@viirya viirya commented Aug 27, 2018 •
edited
Loading

What changes were proposed in this pull request?

This is based on the discussion https://github.com/apache/spark/pull/16677/files#r212805327.

As SQL standard doesn't mandate that a nested order by followed by a limit has to respect that ordering clause, this patch removes the child.outputOrdering check.

How was this patch tested?

Unit tests.

@viirya
Copy link
Member Author

viirya commented Aug 27, 2018

cc @hvanhovell

@hvanhovell
Copy link
Contributor

hvanhovell commented Aug 27, 2018

@viirya did you try to run TakeOrderedAndProjectSuite? I am pretty sure that will fail now ;)...

@hvanhovell
Copy link
Contributor

hvanhovell commented Aug 27, 2018

cc @cloud-fan for a sanity check.

@viirya
Copy link
Member Author

viirya commented Aug 27, 2018 •
edited
Loading

@viirya did you try to run TakeOrderedAndProjectSuite? I am pretty sure that will fail now ;)...

Not yet. Let me try. Yea, it definitely fails now.

@viirya
Copy link
Member Author

viirya commented Aug 27, 2018

@hvanhovell I can set spark.sql.limit.flatGlobalLimit to false to match TakeOrderedAndProjectExec semantics at the beginning of TakeOrderedAndProjectSuite. Or you prefer to add an explicit flag in GlobalLimitExec as you mentioned in #16677 (comment)? I think it may not necessarily to add the flag for now.

@hvanhovell
Copy link
Contributor

hvanhovell commented Aug 27, 2018

Setting spark.sql.limit.flatGlobalLimit to false works for me.

@viirya viirya force-pushed the improve-global-limit-parallelism-followup branch from b57634b to 67ed97d Compare August 27, 2018 01:42
@hvanhovell
Copy link
Contributor

hvanhovell commented Aug 27, 2018

LGTM - Let's wait a little bit with merging to allow others to comment.

@viirya
Copy link
Member Author

viirya commented Aug 27, 2018

Sure, thank you @hvanhovell

@maropu
Copy link
Member

maropu commented Aug 27, 2018

Better to add in global limit in the title?
Anyway, LGTM

@viirya viirya changed the title [SPARK-19355][SQL][Followup] Remove the child.outputPartitioning check [SPARK-19355][SQL][Followup] Remove the child.outputPartitioning check in global limit Aug 27, 2018
@viirya
Copy link
Member Author

viirya commented Aug 27, 2018

@maropu Thanks. I just added it.

@hvanhovell
Copy link
Contributor

hvanhovell commented Aug 27, 2018

Shall we rename it to: [SPARK-19355][SQL][Followup] Remove the child.outputOrdering check in global limit?

@SparkQA
Copy link

SparkQA commented Aug 27, 2018

Test build #95272 has finished for PR 22239 at commit b57634b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya viirya changed the title [SPARK-19355][SQL][Followup] Remove the child.outputPartitioning check in global limit [SPARK-19355][SQL][Followup] Remove the child.outputOrdering check in global limit Aug 27, 2018
@viirya
Copy link
Member Author

viirya commented Aug 27, 2018

@hvanhovell ah, updated. also updated the PR description too.

@SparkQA
Copy link

SparkQA commented Aug 27, 2018

Test build #95273 has finished for PR 22239 at commit 67ed97d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@asfgit asfgit closed this in 5c27b0d Aug 27, 2018
@cloud-fan
Copy link
Contributor

cloud-fan commented Aug 27, 2018

thanks, merging to master!

bogdanrdc pushed a commit to bogdanrdc/spark that referenced this pull request Aug 28, 2018
@viirya @bogdanrdc
[SPARK-19355][SQL][FOLLOWUP] Remove the child.outputOrdering check in…
c129176 
… global limit

## What changes were proposed in this pull request?

This is based on the discussion https://github.com/apache/spark/pull/16677/files#r212805327.

As SQL standard doesn't mandate that a nested order by followed by a limit has to respect that ordering clause, this patch removes the `child.outputOrdering` check.

## How was this patch tested?

Unit tests.

Closes apache#22239 from viirya/improve-global-limit-parallelism-followup.

Authored-by: Liang-Chi Hsieh <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
@cloud-fan cloud-fan mentioned this pull request Sep 20, 2018
Closed
@viirya viirya mentioned this pull request Sep 20, 2018
Closed
asfgit pushed a commit that referenced this pull request Sep 20, 2018
@viirya @cloud-fan
Revert [SPARK-19355][SPARK-25352]
b3bdfd7 
## What changes were proposed in this pull request?

This goes to revert sequential PRs based on some discussion and comments at #16677 (comment).

#22344
#22330
#22239
#16677

## How was this patch tested?

Existing tests.

Closes #22481 from viirya/revert-SPARK-19355-1.

Authored-by: Liang-Chi Hsieh <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 89671a2)
Signed-off-by: Wenchen Fan <[email protected]>
asfgit pushed a commit that referenced this pull request Sep 20, 2018
@viirya @cloud-fan
Revert [SPARK-19355][SPARK-25352]
89671a2 
## What changes were proposed in this pull request?

This goes to revert sequential PRs based on some discussion and comments at #16677 (comment).

#22344
#22330
#22239
#16677

## How was this patch tested?

Existing tests.

Closes #22481 from viirya/revert-SPARK-19355-1.

Authored-by: Liang-Chi Hsieh <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
@viirya viirya deleted the improve-global-limit-parallelism-followup branch December 27, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@viirya @hvanhovell @maropu @SparkQA @cloud-fan