Refactor EnforceDistribution test cases to demonstrate dependencies across optimizer runs. #61
+769
−390
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wiedld
changed the title
wiedld
force-pushed
the
15003/remove-macro
branch
from
bf5a9a1 to
498983f
Compare
wiedld
commented
Mar 5, 2025
Comment on lines
+449
to
+458
| /// Perform a series of runs using the current [`TestConfig`], | ||
| /// assert the expected plan result, | ||
| /// and return the result plan (for potentional subsequent runs). | ||
| fn run( | ||
| &self, | ||
| expected_lines: &[&str], | ||
| plan: Arc<dyn ExecutionPlan>, | ||
| optimizers_to_run: Vec<Run>, | ||
| ) -> Result<Arc<dyn ExecutionPlan>> { | ||
| let expected_lines: Vec<&str> = expected_lines.to_vec(); |
There was a problem hiding this comment.
The reason this returns the plan, is so that we can easily write test cases for idempotency.
let plan_after_first_run = test_config.run(
expected,
plan,
vec![Run::Distribution],
)?;
let plan_after_second_run = test_config.run(
expected,
plan_after_first_run.clone(),
vec![Run::Distribution], // exact same run again
)?;
assert_eq!(
get_plan_string(&plan_after_first_run),
get_plan_string(&plan_after_second_run),
);
wiedld
commented
Mar 5, 2025
Comment on lines
+404
to
+405
| const DISTRIB_DISTRIB_SORT: [Run; 3] = | ||
| [Run::Distribution, Run::Distribution, Run::Sorting]; |
There was a problem hiding this comment.
wiedld
commented
Mar 5, 2025
Comment on lines
+406
to
+407
| const SORT_DISTRIB_DISTRIB: [Run; 3] = | ||
| [Run::Sorting, Run::Distribution, Run::Distribution]; |
There was a problem hiding this comment.
wiedld
commented
Mar 5, 2025
Comment on lines
+1565
to
+1570
| // TODO(wiedld): show different test result if enforce distribution first. | ||
| test_config.run( | ||
| &expected_first_sort_enforcement, | ||
| top_join, | ||
| &TestConfig::new(DoFirst::Sorting).with_prefer_existing_sort() | ||
| ); | ||
| SORT_DISTRIB_DISTRIB.into(), | ||
| )?; |
There was a problem hiding this comment.
I left a few TODO(wiedld) in the test multi_smj_joins.
In some cases the test case is showing the outcome for the sort occurring first BEFORE the distribution run, which does not match the ordering of the optimizer in the default planner -- and I thought that should be highlighted in the test cases.
wiedld
commented
Mar 5, 2025
|
|
||
| // Test: result IS DIFFERENT, if EnforceSorting is run first: |
There was a problem hiding this comment.
Highlighting the dependency of run ordering in the existing test cases.
This was referenced Mar 5, 2025
Merged
…and highlight when the same testing setup, but different ordering of optimizer runs, effect the outcome.
wiedld
force-pushed
the
15003/remove-macro
branch
from
498983f to
9411528
Compare
github-actions
bot
added
documentation
|
Closing this draft, since we opened a new one on the actual apache project. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of apache#15003
Is the next PR, after this previous PR merges: apache#15010
Rationale for this change
Enable us to write test cases using:
TestConfigThe benefits of this approach are:
What changes are included in this PR?
The above, as we as a bit of an increase in test coverage to demonstrate exactly when our test cases (the plan results we see) are ordering dependent.
Are these changes tested?
Yes
Are there any user-facing changes?
No