Skip to content

Conversation

@c21
Copy link
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 :(

@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
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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