You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mustafasrepo (via GitHub)" <gi...@apache.org> on 2023/11/15 14:45:55 UTC

[PR] [MINOR]: Fix column indices in the planning tests [arrow-datafusion]

mustafasrepo opened a new pull request, #8191:
URL: https://github.com/apache/arrow-datafusion/pull/8191

   ## Which issue does this PR close?
   
   <!--
   We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
   -->
   
   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.
   
   <!--
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.  
   -->
   
   ## 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.
   
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   ## Are these changes tested?
   Existing tests should work.
   <!--
   We typically require tests for all PRs in order to:
   1. Prevent the code from being accidentally broken by subsequent changes
   2. Serve as another way to document the expected behavior of the code
   
   If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
   -->
   
   ## Are there any user-facing changes?
   
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!--
   If there are any breaking changes to public APIs, please add the `api change` label.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR]: Fix column indices in the planning tests [arrow-datafusion]

Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8191:
URL: https://github.com/apache/arrow-datafusion/pull/8191#discussion_r1394312088


##########
datafusion/core/src/physical_optimizer/enforce_distribution.rs:
##########
@@ -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]",

Review Comment:
   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).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR]: Fix column indices in the planning tests [arrow-datafusion]

Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on PR #8191:
URL: https://github.com/apache/arrow-datafusion/pull/8191#issuecomment-1813889726

   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR]: Fix column indices in the planning tests [arrow-datafusion]

Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo merged PR #8191:
URL: https://github.com/apache/arrow-datafusion/pull/8191


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org