kosiew commented on code in PR #25990:
URL: https://github.com/apache/datafusion/pull/25990#discussion_r4335947549
##########
datafusion/optimizer/src/single_distinct_to_groupby.rs:
##########
@@ -104,6 +104,32 @@ struct CountRollup {
sum: Arc<AggregateUDF>,
}
+fn unalias_top(mut expr: &Expr) -> &Expr {
+ while let Expr::Alias(alias) = expr
+ && alias
+ .metadata
+ .as_ref()
+ .is_none_or(|metadata| metadata.is_empty())
+ {
+ expr = &alias.expr;
+ }
+ expr
+}
+
+fn into_unaliased_top(expr: Expr) -> Expr {
Review Comment:
Could you add a Rust test with an aliased aggregate carrying non-empty field
metadata? The rule should leave that plan unchanged and preserve its metadata,
confirming the new alias handling respects this boundary.
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -470,6 +483,20 @@ impl AggregateUDFImpl for Count {
}
}
+fn can_rewrite_count_args(args: &[Expr], info: &SimplifyContext) -> bool {
+ let mut has_non_nullable_column = false;
+ for arg in args {
+ match arg {
+ Expr::Column(_) if matches!(info.nullable(arg), Ok(false)) => {
Review Comment:
A non-nullable dictionary column can still contain logical NULLs. For
example, keys [0, 1] referencing values ["a", NULL] make COUNT(d) return 1, but
this rewrite produces COUNT(1) = 2. Please exclude dictionary columns unless
logical non-nullness is guaranteed, and add register_batch regression tests for
global and grouped COUNT that exercise the rewrite without a statistics
shortcut.
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -1007,6 +1036,47 @@ mod tests {
Ok(())
}
+ #[test]
Review Comment:
Consider adding end-to-end tests pairing EXPLAIN with results for filtered
COUNT on a non-nullable column and COUNT on the nullable side of an unmatched
LEFT JOIN. These would verify filter preservation and the expected zero count
after outer-join null extension.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
