Skip to content

[Datasets] Require preserving order if plan has Zip or Sort stage#32860

Merged
ericl merged 1 commit intoray-project:masterfrom
c21:check-plan
Feb 28, 2023
Merged

[Datasets] Require preserving order if plan has Zip or Sort stage#32860
ericl merged 1 commit intoray-project:masterfrom
c21:check-plan

Conversation

@c21
Copy link
Copy Markdown
Contributor

@c21 c21 commented Feb 27, 2023

Why are these changes needed?

This PR is to require preserving order when plan has either Zip or Sort stage. This applies to both bulk and streaming executor. Given streaming executor is not preserving order by default, this PR is more of a bugfix to prevent regression. A followup PR is to add the corresponding check inside optimizer (probably as an optimizer rule). Don't want to couple the change of optimizer here, because we want to enable streaming executor by default in 2.4.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Cheng Su <scnju13@gmail.com>
@c21 c21 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Feb 28, 2023
@ericl ericl merged commit 5472e59 into ray-project:master Feb 28, 2023
Copy link
Copy Markdown
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

LGTM

@c21 c21 deleted the check-plan branch February 28, 2023 01:23
@c21 c21 mentioned this pull request Feb 28, 2023
7 tasks
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request Mar 21, 2023
…y-project#32860)

This PR is to require preserving order when plan has either Zip or Sort stage. This applies to both bulk and streaming executor. Given streaming executor is not preserving order by default, this PR is more of a bugfix to prevent regression. A followup PR is to add the corresponding check inside optimizer (probably as an optimizer rule). Don't want to couple the change of optimizer here, because we want to enable streaming executor by default in 2.4.

Signed-off-by: Jack He <jackhe2345@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
…y-project#32860)

This PR is to require preserving order when plan has either Zip or Sort stage. This applies to both bulk and streaming executor. Given streaming executor is not preserving order by default, this PR is more of a bugfix to prevent regression. A followup PR is to add the corresponding check inside optimizer (probably as an optimizer rule). Don't want to couple the change of optimizer here, because we want to enable streaming executor by default in 2.4.

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
…y-project#32860)

This PR is to require preserving order when plan has either Zip or Sort stage. This applies to both bulk and streaming executor. Given streaming executor is not preserving order by default, this PR is more of a bugfix to prevent regression. A followup PR is to add the corresponding check inside optimizer (probably as an optimizer rule). Don't want to couple the change of optimizer here, because we want to enable streaming executor by default in 2.4.
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
…y-project#32860)

This PR is to require preserving order when plan has either Zip or Sort stage. This applies to both bulk and streaming executor. Given streaming executor is not preserving order by default, this PR is more of a bugfix to prevent regression. A followup PR is to add the corresponding check inside optimizer (probably as an optimizer rule). Don't want to couple the change of optimizer here, because we want to enable streaming executor by default in 2.4.

Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
…y-project#32860)

This PR is to require preserving order when plan has either Zip or Sort stage. This applies to both bulk and streaming executor. Given streaming executor is not preserving order by default, this PR is more of a bugfix to prevent regression. A followup PR is to add the corresponding check inside optimizer (probably as an optimizer rule). Don't want to couple the change of optimizer here, because we want to enable streaming executor by default in 2.4.

Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests-ok The tagger certifies test failures are unrelated and assumes personal liability.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants