Skip to content
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

[MINOR]: Fix column indices in the planning tests #8191

Merged
merged 1 commit into from
Nov 16, 2023
Merged

[MINOR]: Fix column indices in the planning tests #8191

merged 1 commit into from
Nov 16, 2023

Conversation

mustafasrepo
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

I have recognized that in some of the tests under enforce_distribution.rs and replace_with_order_preserving_variant.rs. Column names and column indices are not consistent with the schema. Since during these test we do not execute these plans, inconsistencies didn't arise. However, if we were to run those plans, we would get error.

What changes are included in this PR?

This PR fixes, column name and indices in those tests so that they refer to valid fields in the schema.

Are these changes tested?

Existing tests should work.

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Nov 15, 2023
@@ -3803,7 +3807,7 @@ pub(crate) mod tests {
"SortPreservingMergeExec: [c@2 ASC]",
// Expect repartition on the input to the sort (as it can benefit from additional parallelism)
"SortExec: expr=[c@2 ASC]",
"ProjectionExec: expr=[a@0 as a]",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously after projection there was no column c. However, sort above refers to c, this example changes projection below so that column c is still present at the input of the Sort (All of the changes in this PR are in similar spirit).

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @mustafasrepo

I wonder if we could change the tests to actually try to run the plan (and verify there were no errors, rather than check the results 🤔 )

@mustafasrepo
Copy link
Contributor Author

Thanks @mustafasrepo

I wonder if we could change the tests to actually try to run the plan (and verify there were no errors, rather than check the results 🤔 )

I think checking results is still necessary, to make sure we have expected plans. However, while doing so additionally we can run the plan to verify there is no error. We can add this support in following PR, where we execute optimized plan inside assert_optimized macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants