Skip to content

Conversation

@arhimondr
Copy link
Member

== NO RELEASE NOTE ==

@arhimondr arhimondr requested a review from a team as a code owner January 16, 2025 20:03
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Jan 16, 2025
spershin
spershin previously approved these changes Jan 16, 2025
Copy link
Contributor

@spershin spershin left a comment

Choose a reason for hiding this comment

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

Thanks for quickly fixing the issue!

@arhimondr
Copy link
Member Author

@spershin Thank you for a prompt review. Updated.

@amitkdutta
Copy link
Contributor

@arhimondr Thanks for the quick fix. Wondering why we didn't see the problem in OSS change. Do we need to add any e2e tests that we have in Meta internal system.

@arhimondr
Copy link
Member Author

arhimondr commented Jan 16, 2025

@amitkdutta From what I can see this can only happen when query:

  1. uses grouped execution (for non grouped execution drivers are created eagerly: https://github.com/facebookincubator/velox/blob/main/velox/exec/Task.cpp#L683, for grouped execution lazily when splits are received: https://github.com/facebookincubator/velox/blob/main/velox/exec/Task.cpp#L1284, https://github.com/facebookincubator/velox/blob/main/velox/exec/Task.cpp#L1390)
  2. Initial TaskUpdateRequest has no splits

I guess 1 is why I didn't catch it when shadowing thousands of queries. I assume I just didn't get to to run a lot of grouped execution queries.

And the 2 is why tests are not deterministically failing.

@amitkdutta amitkdutta merged commit ae68ae4 into prestodb:master Jan 16, 2025
58 of 59 checks passed
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