jayzhan211 merged PR #10560:
URL: https://github.com/apache/datafusion/pull/10560
--
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-unsubscr...@dat
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2156333141
It looks pretty nice now! Thanks @alamb
--
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
alamb commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2156133115
> > I have some ideas about additional comments / documentation that I would
be happy to help add
>
> Sure!
Thank you for your patience @jayzhan211 -- I just pushed a bun
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2154855300
> I have some ideas about additional comments / documentation that I would
be happy to help add
Sure!
--
This is an automated message from the Apache Git Service.
To res
alamb commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1631147705
##
datafusion/functions-aggregate/src/first_last.rs:
##
@@ -31,20 +31,28 @@ use datafusion_common::{
use datafusion_expr::function::{AccumulatorArgs, StateFieldsArg
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1631125762
##
datafusion/functions-aggregate/src/first_last.rs:
##
@@ -31,20 +31,28 @@ use datafusion_common::{
use datafusion_expr::function::{AccumulatorArgs, StateFiel
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2154728354
> > I'm not sure if we really need the error check since we don't have any
now
>
> I guess what I was thinking is that the reason there is no check now is
because the rust
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2154683624
And, I think we need `fn agg_builder(self) -> AggregateBuilder;` to get
`AggregateBuilder` from Expr
```rust
fn agg_builder(self) -> AggregateBuilder {
match s
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2154664764
> ```rust
> AggregateExt
> ```
I don't think I get the point
I think we can get the AggregateFunction from the result of `call`
```rust
pub fn call(&se
alamb commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2154554804
> I'm not sure if we really need the error check since we don't have any now
I guess what I was thinking is that the reason there is no check now is
because the rust compiler
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2153499258
> This is really cool @jayzhan211 -- other than some documentation and
examples I think this looks great
>
> I left some other comments inline
>
> One thing that I th
alamb commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1630300241
##
datafusion/functions-aggregate/src/first_last.rs:
##
@@ -31,20 +31,24 @@ use datafusion_common::{
use datafusion_expr::function::{AccumulatorArgs, StateFieldsArg
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1629552024
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1629552024
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1629550398
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1626750687
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1626750687
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1626750687
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1626750687
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1626750687
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform
alamb commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1626296887
##
datafusion-examples/examples/advanced_udaf.rs:
##
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
let df = ctx.table("t").await?;
// perform the a
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611638831
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611638831
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611638831
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611644593
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611644593
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611644593
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611638831
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1611623369
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,31 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2126917265
Maybe we can just extend function for `AggregateFunction` 🤔
`pub fn call(&self, args: Vec) -> AggregateFunction`
``` rust
impl AggregateFunction {
fn build()
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1610984960
##
datafusion/functions-aggregate/src/expr_builder.rs:
##
@@ -0,0 +1,89 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributo
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2126012154
> which means both adding many new functions as well as that they won't work
for user defined aggregate functions
I think they can build the function with macro in function-
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2126011843
> Thank you @jayzhan211 -- this is looking really cool. I have some feeback
on the API design
>
> One major thing I think that might be worth considering is now to get one
alamb commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1610577152
##
datafusion-examples/examples/udaf_expr.rs:
##
@@ -0,0 +1,31 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agre
alamb commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2123673946
THanks @jayzhan211 -- I will plan to review this tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use th
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1605955848
##
docs/source/user-guide/expressions.md:
##
@@ -304,6 +304,16 @@ select log(-1), log(0), sqrt(-1);
| rollup(exprs)
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1605623477
##
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##
@@ -621,9 +623,12 @@ async fn roundtrip_expr_api() -> Result<()> {
lit(1),
)
alamb commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1605497952
##
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##
@@ -31,8 +31,10 @@ use datafusion::datasource::TableProvider;
use datafusion::execution::context::Sess
alamb commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2118310034
This looks super cool @jayzhan211
--
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 s
jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2117670563
I plan to add `distinct` macro in the follow up PR for `count`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub an
jayzhan211 commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1605052852
##
datafusion/functions-aggregate/src/macros.rs:
##
@@ -48,49 +48,98 @@ macro_rules! make_udaf_expr_and_func {
None,
))
}
jayzhan211 opened a new pull request, #10560:
URL: https://github.com/apache/datafusion/pull/10560
## Which issue does this PR close?
Closes #10545.
## Rationale for this change
After this PR, there are two kinds of expression API.
One is the normal one
42 matches
Mail list logo
