alamb commented on code in PR #10831:
URL: https://github.com/apache/datafusion/pull/10831#discussion_r1632033601
##########
datafusion/sql/Cargo.toml:
##########
@@ -54,6 +54,7 @@ strum = { version = "0.26.1", features = ["derive"] }
[dev-dependencies]
ctor = { workspace = true }
datafusion-functions = { workspace = true, default-features = true }
+datafusion-functions-aggregate = { workspace = true }
Review Comment:
I think this is ok.
We have had bad experiences in the past with making semi-circular
dependencies for tests (as I recall it messes up the release process to
crates.io. THough maybe this one is ok, because datafusion-functions-aggregate
doesn't depend on datafusion-sql either directly or transitively
If we are going to go with this approach, perhaps we could remove the stubs
added in https://github.com/apache/datafusion/pull/10816 🤔
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -1124,19 +1125,7 @@ mod tests {
Expr::Literal(ScalarValue::Date32(Some(-1))),
r#"CAST('1969-12-31' AS DATE)"#,
),
- (
- Expr::AggregateFunction(AggregateFunction {
- func_def: AggregateFunctionDefinition::BuiltIn(
- datafusion_expr::AggregateFunction::Sum,
- ),
- args: vec![col("a")],
- distinct: false,
- filter: None,
- order_by: None,
- null_treatment: None,
- }),
- r#"SUM(a)"#,
- ),
+ (sum(col("a")), r#"sum(a)"#),
Review Comment:
😍
--
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]
