Skip to content

Comments

Fix join validation when spill enabled for native#23595

Merged
amitkdutta merged 1 commit intoprestodb:masterfrom
rschlussel:validate-native-spill
Sep 5, 2024
Merged

Fix join validation when spill enabled for native#23595
amitkdutta merged 1 commit intoprestodb:masterfrom
rschlussel:validate-native-spill

Conversation

@rschlussel
Copy link
Contributor

@rschlussel rschlussel commented Sep 5, 2024

Description

ValidateStreamingJoins tests that a join node has the necessary stream properties. When spill is enabled for Java, we need the probe side to have a fixed parallelism, but native does not have that requirement.. Fix ValidateStreamingJoins to not require an extra local exchange for spilling when using native execution.

Motivation and Context

Fix query failures like "java.lang.IllegalArgumentException: Probe side needs an additional local exchange for join: 17" when running with native execution with spill enabled

Impact

Fixes query failures for some join queries when spill and native execution are both enabled.

Test Plan

new tests

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Fix plan validation failures for some join queries running with spill enabled when using Presto C++

@rschlussel rschlussel force-pushed the validate-native-spill branch from 9bafd7d to 08683f2 Compare September 5, 2024 18:58
ValidateStreamingJoins tests that a join node has the necessary stream
properties.  When spill is enabled for Java, we need the probe side to
have a fixed parallelism, but native does not have that requirement..
Fix ValidateStreamingJoins to not require an extra local exchange for
spilling when using native execution.
@rschlussel rschlussel force-pushed the validate-native-spill branch from 08683f2 to bc5d016 Compare September 5, 2024 19:04
@rschlussel rschlussel marked this pull request as ready for review September 5, 2024 19:08
Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Looks great! Thasnk @rschlussel for quickly fixing this long standing issue.

@amitkdutta amitkdutta merged commit 5b10fba into prestodb:master Sep 5, 2024
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
@tdcmeehan tdcmeehan added the from:Meta PR from Meta label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants