Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25482 +/- ##
==========================================
+ Coverage 81.91% 82.73% +0.81%
==========================================
Files 1134 1147 +13
Lines 425703 449552 +23849
Branches 425703 449552 +23849
==========================================
+ Hits 348725 371937 +23212
+ Misses 56300 54942 -1358
- Partials 20678 22673 +1995 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The aggregate support looks useful, but I think the new output-name deduplication changes an existing validation rule, so I'd like that addressed before approval.
| .aggregate(Vec::<Expr>::new(), aggr_with_alias)? | ||
| .build()?; | ||
| expr_list = rewrite_select_aggs(expr_list, &input, &rewrite_map)?; | ||
| expr_list = uniquify_select_expr_names(expr_list)?; |
There was a problem hiding this comment.
This now silently renames duplicate explicit aliases before the existing projection validator can reject them, so select behaves differently from ordinary projections and explicit aggregate. Please remove the public output-name uniquification and let the existing projection validation reject duplicate names, while keeping unique internal aggregate aliases where needed.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_dataframe_api_select_semantics() -> Result<()> { |
There was a problem hiding this comment.
Could you also add a small empty-input case using filter(lit(false)).select(...) with distinct aliases? It would be useful to verify that global aggregation still returns exactly one row, with COUNT = 0, SUM = NULL, and the literal value preserved.
There was a problem hiding this comment.
Agreed. Let me add new tests.
|
@kosiew Thank you for the valuable review! I've removed the public output-name uniquification and added empty-input test case. Could you please take another look when you have a chance? |
kosiew
left a comment
There was a problem hiding this comment.
Thanks for addressing the review comments. The follow-up restores the existing duplicate projection name validation while keeping internal aggregate aliases unique. The new empty-input test also verifies the expected global aggregation behavior.
I've reviewed the changes and have no further concerns. LGTM!
Which issue does this PR close?
select()#17874.Rationale for this change
DataFrame::selectcurrently does not support aggregate expressions directly. Users must explicitly callDataFrame::aggregate. This PR improves ergonomics by allowing aggregate expressions insideselect, while preserving existing behavior and validation rules.What changes are included in this PR?
DataFrame::selectlifts a global Aggregate when every expression is an aggregate or a scalar over aggregates (and literals).What is the testing strategy for this PR?
DataFrameselect/aggregate tests continue to pass without edits.test_dataframe_api_select_semantics(from DataFrame API: allow aggregate functions in select() (#17874) #21021, plus two cases for this design).roundtrip_expr_apiwas not changed and still passes (mixed aggregate + scalar lists stay aProjection).Are there any user-facing changes?
Yes — additive behavior only. The public
select()signature is unchanged. There are no breaking API changes.Users can write a global aggregation with
select():Previously this required:
Both remain valid. Grouped aggregation is still aggregate(group, aggs).