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/07/06 20:30:02 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #6869: Docs: try and clarify what `PartitionEvaluator` functions are called
alamb opened a new pull request, #6869:
URL: https://github.com/apache/arrow-datafusion/pull/6869
# Which issue does this PR close?
N/A
# Rationale for this change
@mhilton noticed that the docs were backwards when he started looking at [the uses_window_frame docs](https://docs.rs/datafusion-expr/latest/datafusion_expr/trait.PartitionEvaluator.html#method.uses_window_frame)
I spent some time researching / verifying what actually happens, and it turns out that `evaluate_all` will be used when the function doesnโt use the window frame and I wanted to encode this learning into documentation
Here is the actual code that dispatches to the different evaluation functions
https://github.com/apache/arrow-datafusion/blob/a1281aa79ea104efdf3d07dd1948f2ae0b374634/datafusion/physical-expr/src/window/built_in.rs#L96-L128
# What changes are included in this PR?
Update the documentation for `PartitionEvaluator` to make what is called when (hopefully) clearer.
I still don't understand enough of what makes a `PartitionEvaluator` "stateful" to document what functions need to be implemented under what circumstances. Maybe @mustafarepo could help clarify this
# Are these changes tested?
N/A
<!--
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
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6869: Docs: try and clarify what `PartitionEvaluator` functions are called
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6869:
URL: https://github.com/apache/arrow-datafusion/pull/6869#discussion_r1255330814
##########
datafusion/expr/src/partition_evaluator.rs:
##########
@@ -87,16 +87,23 @@ use crate::window_state::WindowAggState;
/// previous batch. The previous row number is saved and restored as
/// the state.
///
-/// When implementing a new `PartitionEvaluator`,
-/// `uses_window_frame` and `supports_bounded_execution` flags determine which evaluation method will be called
-/// during runtime. Implement corresponding evaluator according to table below.
+/// When implementing a new `PartitionEvaluator`, implement
+/// corresponding evaluator according to table below.
///
-/// |uses_window_frame|supports_bounded_execution|function_to_implement|
-/// |---|---|----|
-/// |false|false|`evaluate_all` (if we were to implement `PERCENT_RANK` it would end up in this quadrant, we cannot produce any result without seeing whole data)|
-/// |false|true|`evaluate` (optionally can also implement `evaluate_all` for more optimized implementation. However, there will be default implementation that is suboptimal) . If we were to implement `ROW_NUMBER` it will end up in this quadrant. Example `OddRowNumber` showcases this use case|
-/// |true|false|`evaluate` (I think as long as `uses_window_frame` is `true`. There is no way for `supports_bounded_execution` to be false). I couldn't come up with any example for this quadrant |
-/// |true|true|`evaluate`. If we were to implement `FIRST_VALUE`, it would end up in this quadrant|.
+///
+/// |[`uses_window_frame`]|[`supports_bounded_execution`]|[`include_rank`]|function_to_implement|
+/// |---|---|----|----|
+/// |false (default) |false (default) |false (default) | [`evaluate_all`] |
+/// |false |true |false | [`evaluate`] |
+/// |false |true/false |true | [`evaluate_all_with_rank`] |
+/// |true |true/false |true/false | [`evaluate`] |
Review Comment:
:heart:
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #6869: Docs: try and clarify what `PartitionEvaluator` functions are called
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6869:
URL: https://github.com/apache/arrow-datafusion/pull/6869#issuecomment-1636299900
For anyone following along, the simplifying PR is ๐จโ๐ณ ๐ https://github.com/apache/arrow-datafusion/pull/6966
--
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
[GitHub] [arrow-datafusion] alamb merged pull request #6869: Docs: try and clarify what `PartitionEvaluator` functions are called
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6869:
URL: https://github.com/apache/arrow-datafusion/pull/6869
--
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
[GitHub] [arrow-datafusion] mustafasrepo commented on pull request #6869: Docs: try and clarify what `PartitionEvaluator` functions are called
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on PR #6869:
URL: https://github.com/apache/arrow-datafusion/pull/6869#issuecomment-1624970043
Regarding
> I still don't understand enough of what makes a PartitionEvaluator "stateful" to document what functions need to be implemented under what circumstances. Maybe @mustafasrepo could help clarify this
If we ignore `uses_window_frame` flag. Implementation table is as follows
|`supports_bounded_execution`|`include_rank`| function_to_implement |
|---|---|---|
|false|false| evaluate_all |
|false|true| evaluate_all_with_rank |
|true|false| evaluate |
|true|true| evaluate |
In this table, if `supports_bounded_execution` flag is `true`, `evaluate` method should be implemented. For some of the
window functions such as `ROW_NUMBER`, evaluate can be implemented trivially(It can keep track of internal count, each time it is call it can increment counter for each row.). However, if we were to implement `evaluate` method for `RANK` function. Information in the `evaluate` argument (values received and ) is not enough to calculate result. We need to know separation boundaries of order by columns to be able to produce correct results. For this reason, we have `update_state` that enables us to encode useful information to the state. By implementing `update_state` of the `RANK` evaluator, we can store necessary information that enables us to produce correct result during `evaluate` method call.
However, it is a bit confusing.I will try to simplify 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