You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/11/10 19:17:28 UTC
[PR] Minor: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
alamb opened a new pull request, #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128
## Which issue does this PR close?
Part of https://github.com/apache/arrow-datafusion/issues/8120
## Rationale for this change
I did not properly set `ExecutionPlan::equivalence_properties` for one of our extension nodes, which took me a non trivial time to debug.
## What changes are included in this PR?
Add doc comments to `ExecutionPlan::equivalence_properties`
## Are these changes tested?
By doc test
## 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: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128#discussion_r1390684353
##########
datafusion/physical-plan/src/lib.rs:
##########
@@ -203,7 +203,23 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
.collect()
}
- /// Get the [`EquivalenceProperties`] within the plan
+ /// Get the [`EquivalenceProperties`] within the plan.
+ ///
+ /// Equivalence properties tell DataFsion what columns are known to be
+ /// equal, during various optimization passes. By default, this returns " no
+ /// known equivalances" which is always correct, but may cause DataFusion to
+ /// unecessairly resort data.
Review Comment:
```suggestion
/// known equivalences" which is always correct, but may cause DataFusion to
/// unnecessarily resort data.
```
--
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: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128#discussion_r1390683385
##########
datafusion/physical-plan/src/lib.rs:
##########
@@ -203,7 +203,23 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
.collect()
}
- /// Get the [`EquivalenceProperties`] within the plan
+ /// Get the [`EquivalenceProperties`] within the plan.
+ ///
+ /// Equivalence properties tell DataFsion what columns are known to be
Review Comment:
```suggestion
/// Equivalence properties tell DataFusion what columns are known to be
```
--
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: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128#discussion_r1390652405
##########
datafusion/physical-plan/src/lib.rs:
##########
@@ -203,7 +203,23 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
.collect()
}
- /// Get the [`EquivalenceProperties`] within the plan
+ /// Get the [`EquivalenceProperties`] within the plan.
+ ///
+ /// Equivalence properties tell DataFsion what columns are known to be
+ /// equal, during various optimization passes. By default, this returns " no
+ /// known equivalances" which is always correct, but may cause DataFusion to
+ /// unecessairly resort data.
+ ///
+ /// If this ExecutionPlan makes no changes to the schema of the rows flowing
+ /// through it or how columns withink each row relate to each other, it
+ /// should should return the equivalence properties of its input. For
+ /// example, since `FilterExec` may remove rows from its input, but does not
+ /// otherwise modify them, it preserves its input equivalece properties.
Review Comment:
```suggestion
/// otherwise modify them, it preserves its input equivalence properties.
```
--
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: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128#discussion_r1390684642
##########
datafusion/physical-plan/src/lib.rs:
##########
@@ -203,7 +203,23 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
.collect()
}
- /// Get the [`EquivalenceProperties`] within the plan
+ /// Get the [`EquivalenceProperties`] within the plan.
+ ///
+ /// Equivalence properties tell DataFsion what columns are known to be
+ /// equal, during various optimization passes. By default, this returns " no
+ /// known equivalances" which is always correct, but may cause DataFusion to
+ /// unecessairly resort data.
+ ///
+ /// If this ExecutionPlan makes no changes to the schema of the rows flowing
+ /// through it or how columns withink each row relate to each other, it
+ /// should should return the equivalence properties of its input. For
Review Comment:
```suggestion
/// should return the equivalence properties of its input. For
```
--
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: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128
--
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: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128#discussion_r1390652956
##########
datafusion/physical-plan/src/lib.rs:
##########
@@ -203,7 +203,23 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
.collect()
}
- /// Get the [`EquivalenceProperties`] within the plan
+ /// Get the [`EquivalenceProperties`] within the plan.
+ ///
+ /// Equivalence properties tell DataFsion what columns are known to be
+ /// equal, during various optimization passes. By default, this returns " no
+ /// known equivalances" which is always correct, but may cause DataFusion to
+ /// unecessairly resort data.
+ ///
+ /// If this ExecutionPlan makes no changes to the schema of the rows flowing
+ /// through it or how columns withink each row relate to each other, it
Review Comment:
```suggestion
/// through it or how columns within each row relate to each other, it
```
--
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: Document `ExecutionPlan::equivalence_properties` more thoroughly [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8128:
URL: https://github.com/apache/arrow-datafusion/pull/8128#discussion_r1390683674
##########
datafusion/physical-plan/src/lib.rs:
##########
@@ -203,7 +203,23 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
.collect()
}
- /// Get the [`EquivalenceProperties`] within the plan
+ /// Get the [`EquivalenceProperties`] within the plan.
+ ///
+ /// Equivalence properties tell DataFsion what columns are known to be
+ /// equal, during various optimization passes. By default, this returns " no
Review Comment:
```suggestion
/// equal, during various optimization passes. By default, this returns "no
```
--
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