-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Extend TestSchedulingUtils with top level union #27080
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.
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
base: master
Are you sure you want to change the base?
Extend TestSchedulingUtils with top level union #27080
Conversation
Reviewer's GuideAdd missing test coverage for top-level UNION in SchedulingUtils by implementing a new test method that verifies canStream behavior for each branch of a UNION stage. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In testCanStreamUnion you forgot to include cSubPlan in the createSubPlan children list, so you should pass ImmutableList.of(aSubPlan, bSubPlan, cSubPlan) instead of just a and b.
- Consider adding a negative test where one union child cannot stream so you assert that canStream(parent, child) returns false when any branch is non‐streamable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In testCanStreamUnion you forgot to include cSubPlan in the createSubPlan children list, so you should pass ImmutableList.of(aSubPlan, bSubPlan, cSubPlan) instead of just a and b.
- Consider adding a negative test where one union child cannot stream so you assert that canStream(parent, child) returns false when any branch is non‐streamable.
## Individual Comments
### Comment 1
<location> `core/trino-main/src/test/java/io/trino/execution/scheduler/TestSchedulingUtils.java:93-96` </location>
<code_context>
+ SubPlan cSubPlan = valuesSubPlan("c");
+ RemoteSourceNode remoteSourceC = remoteSource("c");
+
+ SubPlan parentSubPlan = createSubPlan(
+ "parent",
+ union("union", ImmutableList.of(remoteSourceA, remoteSourceB, remoteSourceC)),
+ ImmutableList.of(aSubPlan, bSubPlan));
+
+ assertThat(SchedulingUtils.canStream(parentSubPlan, aSubPlan)).isTrue();
</code_context>
<issue_to_address>
**question (testing):** Check if all union children are included in parentSubPlan's children.
Please verify that cSubPlan is either intentionally excluded or properly accounted for; otherwise, the test may not represent the union's full structure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| SubPlan parentSubPlan = createSubPlan( | ||
| "parent", | ||
| union("union", ImmutableList.of(remoteSourceA, remoteSourceB, remoteSourceC)), | ||
| ImmutableList.of(aSubPlan, bSubPlan)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (testing): Check if all union children are included in parentSubPlan's children.
Please verify that cSubPlan is either intentionally excluded or properly accounted for; otherwise, the test may not represent the union's full structure.
| SubPlan parentSubPlan = createSubPlan( | ||
| "parent", | ||
| union("union", ImmutableList.of(remoteSourceA, remoteSourceB, remoteSourceC)), | ||
| ImmutableList.of(aSubPlan, bSubPlan)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cSubPlan isn't a child of "parent"?
Add some missing test coverage
Summary by Sourcery
Tests: