You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2024/03/18 23:57:35 UTC
[PR] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
comphead opened a new pull request, #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686
## 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
There are scenarios when output datatype for builtin window functions are not the same as hardcoded in DataFusion.
For instance Apache Spark requires some functions like `ROW_NUMBER, RANK`, etc to be `Int` rather than `UInt` which is hardcoded in DF. It would be great to derive builtin window function output type from the incoming schema rather than having the type hardcoded. Having this derivation from the schema increases DF extensibility to be used in other hybrid systems.
<!--
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?
Changed physical planner for window builtin functions to derive the output data type from 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?
Yes
<!--
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?
No
<!--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#issuecomment-2005361615
@mustafasrepo @alamb @viirya appreciate for your thoughts
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1530643670
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
- // figure out the output type
- let data_type = &fun.return_type(&input_types)?;
Ok(match fun {
- BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, data_type)),
- BuiltInWindowFunction::Rank => Arc::new(rank(name, data_type)),
- BuiltInWindowFunction::DenseRank => Arc::new(dense_rank(name, data_type)),
- BuiltInWindowFunction::PercentRank => Arc::new(percent_rank(name, data_type)),
- BuiltInWindowFunction::CumeDist => Arc::new(cume_dist(name, data_type)),
+ BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, out_data_type)),
Review Comment:
You are right @alamb, It is always the same for DataFusion, but when building the hybrid system, like something on top of the DF, it happens expected external output type its not the same as hardcoded in DF.
Imho, derivation from schema improves DF extensibility as it reduces coupling between physical and logical layers and the schema is the contract. Thus external systems on top of DF may safely use physical layer just providing the expected schema
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1529805006
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
Review Comment:
Actually, I think with these changes parameter `input_schema: &Schema,` name is misleading in the `create_built_in_window_expr` function. Similarly, `logical_input_schema: &DFSchema,` parameter for the `create_window_expr` is misleading. Now these parameters are the schema of the window function (not input schema). Hence, I think it is guaranteed to find name of the window function in the schema (As long as window function name calculations are consistent). However, we should change these parameters name I think. I checked, how `LogicalPlan::Window` calculates its schema.
It call `exprlist_to_fields` util to calculate fields of window expressions.
- Which in turn uses `.to_field` method on `input_schema`
- Which in turn uses `Expr::get_type` method on `input_schema`
- which calculates output type using `fun.return_type(&data_types)`.
In this sense, I think this PR behaves same with current version, and removes duplication. However, we should change parameter/variable names to reflect which data it actually uses.
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1530765940
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
- // figure out the output type
- let data_type = &fun.return_type(&input_types)?;
Ok(match fun {
- BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, data_type)),
- BuiltInWindowFunction::Rank => Arc::new(rank(name, data_type)),
- BuiltInWindowFunction::DenseRank => Arc::new(dense_rank(name, data_type)),
- BuiltInWindowFunction::PercentRank => Arc::new(percent_rank(name, data_type)),
- BuiltInWindowFunction::CumeDist => Arc::new(cume_dist(name, data_type)),
+ BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, out_data_type)),
Review Comment:
> I'm thinking if can we move on with this PR, and migrate all window funcs to unified trait in follow up activity?
That seems fine to me (and the migration is already tracked, albiet at a high level) by another ticket
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1530643670
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
- // figure out the output type
- let data_type = &fun.return_type(&input_types)?;
Ok(match fun {
- BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, data_type)),
- BuiltInWindowFunction::Rank => Arc::new(rank(name, data_type)),
- BuiltInWindowFunction::DenseRank => Arc::new(dense_rank(name, data_type)),
- BuiltInWindowFunction::PercentRank => Arc::new(percent_rank(name, data_type)),
- BuiltInWindowFunction::CumeDist => Arc::new(cume_dist(name, data_type)),
+ BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, out_data_type)),
Review Comment:
You are right @alamb, It is always the same for DataFusion, but when building the hybrid system, like something on top of the DF, it happens expected external output type its not the same as hardcoded in DF.
Imho, derivation from schema improves DF extensibility as it reduces coupling between physical and logical layers and the schema is the contract. Thus external systems on top of DF may safely use physical layer just providing the expected schema.
UPD: Thus external systems on top of DF may safely use physical layer just providing the expected query plan(schema is the part of the query plan)
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1529678537
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
Review Comment:
I am not sure how this works (or makes sense), can you help me understand? How can you decide the output type of a function without knowing what the function is (i.e. using `fun.return_type`)? Is it always guaranteed that you will find `name` in `input_schema`? If yes, no problem. But if no, shouldn't you fall back to using `fun.return_type`?
I understand that some people may want to have `ROW_NUMBER` output an `Int` instead of an `UInt` (which doesn't make sense but I can see how such an ask may arise for backwards compatibility reasons).
##########
datafusion/core/tests/fuzz_cases/window_fuzz.rs:
##########
@@ -142,6 +144,7 @@ async fn window_bounded_window_random_comparison() -> Result<()> {
// This tests whether we can generate bounded window results for each input
// batch immediately for causal window frames.
#[tokio::test(flavor = "multi_thread", worker_threads = 16)]
+#[ignore = "reason"]
Review Comment:
Is this a leftover?
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1529808130
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -742,7 +742,7 @@ impl DefaultPhysicalPlanner {
);
}
- let logical_input_schema = input.schema();
+ let logical_input_schema = logical_plan.schema();
Review Comment:
As far as I can tell this schema is no longer `input_schema`.
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1530501984
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
- // figure out the output type
- let data_type = &fun.return_type(&input_types)?;
Ok(match fun {
- BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, data_type)),
- BuiltInWindowFunction::Rank => Arc::new(rank(name, data_type)),
- BuiltInWindowFunction::DenseRank => Arc::new(dense_rank(name, data_type)),
- BuiltInWindowFunction::PercentRank => Arc::new(percent_rank(name, data_type)),
- BuiltInWindowFunction::CumeDist => Arc::new(cume_dist(name, data_type)),
+ BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, out_data_type)),
Review Comment:
I find it strange in general that we have to pass a "output type" to `RowNumber` when it is (effectively) always the same.
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1530678850
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
Review Comment:
Thanks @mustafasrepo for clarification, I would also like to add the part were `fun.report_type()` called for window builtin functions in logical planner https://github.com/apache/arrow-datafusion/blob/b87dd6143c2dc089b07f74780bd525c4369e68a3/datafusion/expr/src/expr_schema.rs#L184
So when physical layer gets called all the metadata(names, datatypes) is already known
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1529480855
##########
datafusion/core/tests/fuzz_cases/window_fuzz.rs:
##########
@@ -46,6 +46,7 @@ use rand::rngs::StdRng;
use rand::{Rng, SeedableRng};
#[tokio::test(flavor = "multi_thread", worker_threads = 16)]
+#[ignore = "reason"]
Review Comment:
I'm investigating why fuzz testing not going through
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1529808130
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -742,7 +742,7 @@ impl DefaultPhysicalPlanner {
);
}
- let logical_input_schema = input.schema();
+ let logical_input_schema = logical_plan.schema();
Review Comment:
Similar to before this schema is no longer `input_schema`.
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1529805006
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
Review Comment:
Actually, I think with these changes parameter `input_schema: &Schema,` name is misleading in the `create_built_in_window_expr` function. Similarly, `logical_input_schema: &DFSchema,` parameter for the `create_window_expr` is misleading. Now these parameters are the schema of the window function (not input schema). Hence, I think it is guaranteed to find name of the window function in the schema (As long as window function name calculations are consistent).
I checked, how `LogicalPlan::Window` calculates its schema.
It calls `exprlist_to_fields` util to calculate fields of window expressions.
- Which in turn uses `.to_field` method on `input_schema`
- Which in turn uses `Expr::get_type` method on `input_schema`
- which calculates output type using `fun.return_type(&data_types)`.
In this sense, I think this PR behaves same with current version, and removes duplication. However, we should change parameter/variable names to reflect which data it actually uses.
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1530657760
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType = input_schema.field_with_name(&name)?.data_type();
- // figure out the output type
- let data_type = &fun.return_type(&input_types)?;
Ok(match fun {
- BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, data_type)),
- BuiltInWindowFunction::Rank => Arc::new(rank(name, data_type)),
- BuiltInWindowFunction::DenseRank => Arc::new(dense_rank(name, data_type)),
- BuiltInWindowFunction::PercentRank => Arc::new(percent_rank(name, data_type)),
- BuiltInWindowFunction::CumeDist => Arc::new(cume_dist(name, data_type)),
+ BuiltInWindowFunction::RowNumber => Arc::new(RowNumber::new(name, out_data_type)),
Review Comment:
Reg to trait unification this is also a great idea to improve extensibility.
I'm thinking if can we move on with this PR, and migrate all window funcs to unified trait in follow up activity?
--
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] Make builtin window function output datatype to be derived from schema [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686
--
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