You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "timsaucer (via GitHub)" <gi...@apache.org> on 2025/05/01 13:53:50 UTC
[PR] feat: metadata handling for aggregates and window functions [datafusion]
timsaucer opened a new pull request, #15911:
URL: https://github.com/apache/datafusion/pull/15911
## Which issue does this PR close?
Closes https://github.com/apache/datafusion/issues/15902
## Rationale for this change
This change is a follow on to https://github.com/apache/datafusion/pull/15646. With this change we can now handle metadata for both window and aggregate functions. It enables the use of extension types via this metadata handling.
## What changes are included in this PR?
Instead of passing `DataType` to aggregates and windows we now pass `Field` in their arguments. `return_type` has been replaced with `return_field` so we can get metadata out of these functions as well.
## Are these changes tested?
All existing unit tests pass. New unit tests are added.
## Are there any user-facing changes?
Yes, the migration guide contains information on the updates that the user will need to make for their user defined functions.
## TODO before moving to ready to review
- [ ] Add unit test that is a window of an aggregate function
- [ ] Verify migration guide covers both aggregate and window functions
- [ ] Find remaining places where the function name doesn't match the arguments (ie: `fn return_type()` that returns a `Field`)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2898398814
Yes, that was me volunteering to attempt some more complicated queries that depend on metadata (although I know Tim has done quite a bit). I can do this next week.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2898311133
🎉
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2898506678
Yes, absolutely. I also want to start writing a blog with good examples in it. First I want to get the FieldRef changes in. I’m on PTO for the next few days but might do a little over the weekend.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2876838451
🤖 `./gh_compare_branch_bench.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/gh_compare_branch_bench.sh) Running
Linux aal-dev 6.11.0-1013-gcp #13~24.04.1-Ubuntu SMP Wed Apr 2 16:34:16 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing feat/metadata-handling-aggregates (4512cd8b888dd09e2ac93e56188dc3ec3f03ddee) to 7a3d020af2a3736db11ba7477efc1a5dae96c2d3 [diff](https://github.com/apache/datafusion/compare/7a3d020af2a3736db11ba7477efc1a5dae96c2d3..4512cd8b888dd09e2ac93e56188dc3ec3f03ddee)
BENCH_NAME=sql_planner
BENCH_COMMAND=cargo bench --bench sql_planner
BENCH_FILTER=
BENCH_BRANCH_NAME=feat_metadata-handling-aggregates
Results will be posted here when complete
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#discussion_r2476288257
##########
datafusion/expr/src/udaf.rs:
##########
@@ -674,6 +681,31 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
/// the arguments
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;
+ /// What type will be returned by this function, given the arguments?
+ ///
+ /// By default, this function calls [`Self::return_type`] with the
+ /// types of each argument.
+ ///
+ /// # Notes
+ ///
+ /// Most UDFs should implement [`Self::return_type`] and not this
+ /// function as the output type for most functions only depends on the types
+ /// of their inputs (e.g. `sum(f64)` is always `f64`).
+ ///
+ /// This function can be used for more advanced cases such as:
+ ///
+ /// 1. specifying nullability
+ /// 2. return types based on the **values** of the arguments (rather than
+ /// their **types**.
Review Comment:
Thanks for checking, I'll clean this up in a PR later 👍
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2848636584
FYI @paleolimbot @crystalxyz since this impacts both of your work
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#discussion_r2473721449
##########
datafusion/expr/src/udaf.rs:
##########
@@ -674,6 +681,31 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
/// the arguments
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;
+ /// What type will be returned by this function, given the arguments?
+ ///
+ /// By default, this function calls [`Self::return_type`] with the
+ /// types of each argument.
+ ///
+ /// # Notes
+ ///
+ /// Most UDFs should implement [`Self::return_type`] and not this
+ /// function as the output type for most functions only depends on the types
+ /// of their inputs (e.g. `sum(f64)` is always `f64`).
+ ///
+ /// This function can be used for more advanced cases such as:
+ ///
+ /// 1. specifying nullability
+ /// 2. return types based on the **values** of the arguments (rather than
+ /// their **types**.
Review Comment:
Hi @timsaucer, was reading through some documentation and wanted to understand this point further. Could you elaborate on this? How can this function return types based on argument values if it only gives access to `Field`s? (now `FieldRef`s)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2891735853
> > Do you think it is feasible to update the scalar, aggregate, and window function APIs to use `FieldRef` instead of Field? That way we can avoid most string copies.
>
> Do you think it's reasonable to merge this as is and do switch to `FieldRef` in a separate PR?
Absolutely -- it makes sense to me
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2877038182
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group feat_metadata-handling-aggregates main
----- --------------------------------- ----
logical_aggregate_with_join 1.00 743.2±4.83µs ? ?/sec 1.00 740.7±3.23µs ? ?/sec
logical_select_all_from_1000 1.00 131.4±0.23ms ? ?/sec 1.04 136.2±0.28ms ? ?/sec
logical_select_one_from_700 1.00 419.6±2.14µs ? ?/sec 1.00 418.2±1.36µs ? ?/sec
logical_trivial_join_high_numbered_columns 1.00 382.7±2.66µs ? ?/sec 1.00 383.1±2.60µs ? ?/sec
logical_trivial_join_low_numbered_columns 1.00 367.4±1.33µs ? ?/sec 1.01 369.5±2.12µs ? ?/sec
physical_intersection 1.00 856.6±12.34µs ? ?/sec 1.00 856.0±5.06µs ? ?/sec
physical_join_consider_sort 1.00 1374.4±6.84µs ? ?/sec 1.01 1385.8±4.67µs ? ?/sec
physical_join_distinct 1.00 359.6±2.05µs ? ?/sec 1.00 359.1±1.71µs ? ?/sec
physical_many_self_joins 1.00 10.3±0.06ms ? ?/sec 1.01 10.4±0.06ms ? ?/sec
physical_plan_clickbench_all 1.01 144.6±1.55ms ? ?/sec 1.00 143.4±1.55ms ? ?/sec
physical_plan_clickbench_q1 1.00 1685.4±16.98µs ? ?/sec 1.00 1682.3±16.79µs ? ?/sec
physical_plan_clickbench_q10 1.01 2.5±0.02ms ? ?/sec 1.00 2.5±0.02ms ? ?/sec
physical_plan_clickbench_q11 1.01 2.6±0.04ms ? ?/sec 1.00 2.6±0.03ms ? ?/sec
physical_plan_clickbench_q12 1.00 2.7±0.04ms ? ?/sec 1.00 2.7±0.03ms ? ?/sec
physical_plan_clickbench_q13 1.02 2.4±0.03ms ? ?/sec 1.00 2.4±0.02ms ? ?/sec
physical_plan_clickbench_q14 1.01 2.6±0.03ms ? ?/sec 1.00 2.6±0.03ms ? ?/sec
physical_plan_clickbench_q15 1.01 2.5±0.03ms ? ?/sec 1.00 2.5±0.03ms ? ?/sec
physical_plan_clickbench_q16 1.01 2.3±0.03ms ? ?/sec 1.00 2.3±0.03ms ? ?/sec
physical_plan_clickbench_q17 1.01 2.4±0.02ms ? ?/sec 1.00 2.4±0.02ms ? ?/sec
physical_plan_clickbench_q18 1.00 1998.0±17.95µs ? ?/sec 1.00 1990.9±23.32µs ? ?/sec
physical_plan_clickbench_q19 1.01 2.9±0.03ms ? ?/sec 1.00 2.9±0.03ms ? ?/sec
physical_plan_clickbench_q2 1.00 1945.9±25.90µs ? ?/sec 1.00 1941.2±18.75µs ? ?/sec
physical_plan_clickbench_q20 1.00 1693.9±22.97µs ? ?/sec 1.00 1689.5±20.38µs ? ?/sec
physical_plan_clickbench_q21 1.00 1985.3±18.43µs ? ?/sec 1.00 1992.6±24.08µs ? ?/sec
physical_plan_clickbench_q22 1.00 2.6±0.03ms ? ?/sec 1.00 2.6±0.03ms ? ?/sec
physical_plan_clickbench_q23 1.01 3.0±0.02ms ? ?/sec 1.00 2.9±0.04ms ? ?/sec
physical_plan_clickbench_q24 1.00 4.7±0.04ms ? ?/sec 1.00 4.7±0.05ms ? ?/sec
physical_plan_clickbench_q25 1.01 2.0±0.03ms ? ?/sec 1.00 2.0±0.02ms ? ?/sec
physical_plan_clickbench_q26 1.00 1834.8±20.30µs ? ?/sec 1.00 1829.4±18.25µs ? ?/sec
physical_plan_clickbench_q27 1.01 2.1±0.03ms ? ?/sec 1.00 2.0±0.02ms ? ?/sec
physical_plan_clickbench_q28 1.02 2.9±0.04ms ? ?/sec 1.00 2.8±0.03ms ? ?/sec
physical_plan_clickbench_q29 1.02 3.6±0.04ms ? ?/sec 1.00 3.5±0.03ms ? ?/sec
physical_plan_clickbench_q3 1.01 1920.0±21.90µs ? ?/sec 1.00 1906.0±20.35µs ? ?/sec
physical_plan_clickbench_q30 1.07 15.3±0.13ms ? ?/sec 1.00 14.4±0.13ms ? ?/sec
physical_plan_clickbench_q31 1.01 2.9±0.04ms ? ?/sec 1.00 2.8±0.03ms ? ?/sec
physical_plan_clickbench_q32 1.01 2.9±0.02ms ? ?/sec 1.00 2.9±0.01ms ? ?/sec
physical_plan_clickbench_q33 1.01 2.5±0.03ms ? ?/sec 1.00 2.5±0.03ms ? ?/sec
physical_plan_clickbench_q34 1.00 2.1±0.02ms ? ?/sec 1.00 2.1±0.03ms ? ?/sec
physical_plan_clickbench_q35 1.02 2.2±0.04ms ? ?/sec 1.00 2.2±0.02ms ? ?/sec
physical_plan_clickbench_q36 1.00 3.0±0.03ms ? ?/sec 1.00 3.0±0.03ms ? ?/sec
physical_plan_clickbench_q37 1.00 3.1±0.03ms ? ?/sec 1.00 3.1±0.03ms ? ?/sec
physical_plan_clickbench_q38 1.01 3.1±0.04ms ? ?/sec 1.00 3.1±0.04ms ? ?/sec
physical_plan_clickbench_q39 1.00 2.8±0.02ms ? ?/sec 1.00 2.8±0.03ms ? ?/sec
physical_plan_clickbench_q4 1.01 1665.3±16.45µs ? ?/sec 1.00 1644.6±18.23µs ? ?/sec
physical_plan_clickbench_q40 1.00 3.5±0.03ms ? ?/sec 1.00 3.4±0.03ms ? ?/sec
physical_plan_clickbench_q41 1.00 3.0±0.03ms ? ?/sec 1.00 3.0±0.03ms ? ?/sec
physical_plan_clickbench_q42 1.01 3.0±0.04ms ? ?/sec 1.00 2.9±0.03ms ? ?/sec
physical_plan_clickbench_q43 1.00 3.1±0.03ms ? ?/sec 1.01 3.1±0.04ms ? ?/sec
physical_plan_clickbench_q44 1.00 1808.0±13.06µs ? ?/sec 1.01 1818.9±32.12µs ? ?/sec
physical_plan_clickbench_q45 1.00 1833.0±17.45µs ? ?/sec 1.00 1838.5±26.67µs ? ?/sec
physical_plan_clickbench_q46 1.00 2.3±0.02ms ? ?/sec 1.01 2.3±0.03ms ? ?/sec
physical_plan_clickbench_q47 1.00 2.9±0.04ms ? ?/sec 1.02 2.9±0.07ms ? ?/sec
physical_plan_clickbench_q48 1.00 3.4±0.02ms ? ?/sec 1.03 3.5±0.07ms ? ?/sec
physical_plan_clickbench_q49 1.00 3.9±0.04ms ? ?/sec 1.00 3.9±0.05ms ? ?/sec
physical_plan_clickbench_q5 1.00 1842.3±16.47µs ? ?/sec 1.00 1849.1±24.32µs ? ?/sec
physical_plan_clickbench_q50 1.00 3.6±0.04ms ? ?/sec 1.01 3.6±0.05ms ? ?/sec
physical_plan_clickbench_q51 1.00 2.3±0.02ms ? ?/sec 1.03 2.4±0.04ms ? ?/sec
physical_plan_clickbench_q6 1.01 1857.3±17.44µs ? ?/sec 1.00 1846.3±20.39µs ? ?/sec
physical_plan_clickbench_q7 1.02 1733.2±21.81µs ? ?/sec 1.00 1693.1±17.34µs ? ?/sec
physical_plan_clickbench_q8 1.00 2.4±0.02ms ? ?/sec 1.00 2.4±0.02ms ? ?/sec
physical_plan_clickbench_q9 1.00 2.3±0.03ms ? ?/sec 1.00 2.3±0.02ms ? ?/sec
physical_plan_tpcds_all 1.00 1099.2±4.48ms ? ?/sec 1.00 1094.2±3.64ms ? ?/sec
physical_plan_tpch_all 1.01 65.1±0.54ms ? ?/sec 1.00 64.7±0.37ms ? ?/sec
physical_plan_tpch_q1 1.04 2.2±0.02ms ? ?/sec 1.00 2.2±0.01ms ? ?/sec
physical_plan_tpch_q10 1.01 3.3±0.03ms ? ?/sec 1.00 3.3±0.01ms ? ?/sec
physical_plan_tpch_q11 1.01 3.4±0.03ms ? ?/sec 1.00 3.4±0.02ms ? ?/sec
physical_plan_tpch_q12 1.02 1964.5±11.28µs ? ?/sec 1.00 1929.3±9.28µs ? ?/sec
physical_plan_tpch_q13 1.01 1493.4±8.83µs ? ?/sec 1.00 1482.9±6.28µs ? ?/sec
physical_plan_tpch_q14 1.03 1998.5±4.07µs ? ?/sec 1.00 1946.8±8.12µs ? ?/sec
physical_plan_tpch_q16 1.01 2.6±0.02ms ? ?/sec 1.00 2.6±0.01ms ? ?/sec
physical_plan_tpch_q17 1.00 2.6±0.01ms ? ?/sec 1.01 2.6±0.01ms ? ?/sec
physical_plan_tpch_q18 1.00 2.7±0.02ms ? ?/sec 1.00 2.7±0.01ms ? ?/sec
physical_plan_tpch_q19 1.02 4.0±0.02ms ? ?/sec 1.00 3.9±0.03ms ? ?/sec
physical_plan_tpch_q2 1.00 5.7±0.02ms ? ?/sec 1.01 5.7±0.04ms ? ?/sec
physical_plan_tpch_q20 1.00 3.4±0.02ms ? ?/sec 1.00 3.4±0.05ms ? ?/sec
physical_plan_tpch_q21 1.00 4.4±0.03ms ? ?/sec 1.00 4.4±0.02ms ? ?/sec
physical_plan_tpch_q22 1.01 3.0±0.02ms ? ?/sec 1.00 3.0±0.02ms ? ?/sec
physical_plan_tpch_q3 1.02 2.3±0.01ms ? ?/sec 1.00 2.2±0.01ms ? ?/sec
physical_plan_tpch_q4 1.00 1673.3±6.04µs ? ?/sec 1.00 1670.4±11.15µs ? ?/sec
physical_plan_tpch_q5 1.01 3.2±0.02ms ? ?/sec 1.00 3.2±0.02ms ? ?/sec
physical_plan_tpch_q6 1.02 941.5±10.54µs ? ?/sec 1.00 927.1±7.27µs ? ?/sec
physical_plan_tpch_q7 1.00 4.4±0.04ms ? ?/sec 1.00 4.4±0.04ms ? ?/sec
physical_plan_tpch_q8 1.00 5.3±0.02ms ? ?/sec 1.00 5.3±0.03ms ? ?/sec
physical_plan_tpch_q9 1.00 4.2±0.02ms ? ?/sec 1.00 4.2±0.03ms ? ?/sec
physical_select_aggregates_from_200 1.00 26.9±0.13ms ? ?/sec 1.00 26.8±0.16ms ? ?/sec
physical_select_all_from_1000 1.00 146.0±0.41ms ? ?/sec 1.03 150.0±0.31ms ? ?/sec
physical_select_one_from_700 1.00 1064.6±3.99µs ? ?/sec 1.01 1072.5±7.41µs ? ?/sec
physical_sorted_union_orderby 1.00 64.5±0.55ms ? ?/sec 1.00 64.4±0.43ms ? ?/sec
physical_theta_join_consider_sort 1.00 1759.6±12.43µs ? ?/sec 1.00 1764.7±15.36µs ? ?/sec
physical_unnest_to_join 1.00 1340.5±6.88µs ? ?/sec 1.01 1356.1±11.70µs ? ?/sec
with_param_values_many_columns 1.01 168.4±0.89µs ? ?/sec 1.00 167.0±1.19µs ? ?/sec
```
</p>
</details>
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2891757264
> > > Do you think it is feasible to update the scalar, aggregate, and window function APIs to use `FieldRef` instead of Field? That way we can avoid most string copies.
> >
> >
> > Do you think it's reasonable to merge this as is and do switch to `FieldRef` in a separate PR?
>
> Absolutely -- it makes sense to me -- it would also be great to see if we can update the scalar function API while we are at it
Thanks. I'll re-try CI after pulling in main. This passes every time on my local machine. I'm not sure why it's failing here.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2898362386
@paleolimbot noted today on the call that it would be great to start testing some of the user defined type changes before we released 48
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2888464798
> Do you think it is feasible to update the scalar, aggregate, and window function APIs to use `FieldRef` instead of Field? That way we can avoid most string copies.
Do you think it's reasonable to merge this as is and do switch to `FieldRef` in a separate PR?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#issuecomment-2891771248
I just merged a fix for CI on main, and remerged this PR. Hopefully it will now be good to go
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer merged PR #15911:
URL: https://github.com/apache/datafusion/pull/15911
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: metadata handling for aggregates and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #15911:
URL: https://github.com/apache/datafusion/pull/15911#discussion_r2474355513
##########
datafusion/expr/src/udaf.rs:
##########
@@ -674,6 +681,31 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
/// the arguments
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;
+ /// What type will be returned by this function, given the arguments?
+ ///
+ /// By default, this function calls [`Self::return_type`] with the
+ /// types of each argument.
+ ///
+ /// # Notes
+ ///
+ /// Most UDFs should implement [`Self::return_type`] and not this
+ /// function as the output type for most functions only depends on the types
+ /// of their inputs (e.g. `sum(f64)` is always `f64`).
+ ///
+ /// This function can be used for more advanced cases such as:
+ ///
+ /// 1. specifying nullability
+ /// 2. return types based on the **values** of the arguments (rather than
+ /// their **types**.
Review Comment:
On reading it back months later this docstring looks incorrect. The idea is there in the Scalar UDF but you are correct that it is not handled in the same way in aggregates here. In Scalar UDFs when we call the similar function we pass along `ReturnFieldArgs` not simply `&[FieldRef]`. There we know when we have scalar values that can modify the return.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org