Convert variance sample to udaf #10713

yyin-dev · 2024-05-29T12:09:55Z

Which issue does this PR close?

Closes #10667 .

Are these changes tested?

New proto roundtrip tests.
Moved tests into sqllogictests.

yyin-dev · 2024-05-29T13:17:17Z

@jayzhan211 I'm working on a change, but can you help me understand the semantics here:

# csv_query_distinct_variance
query R
SELECT var(distinct c2) FROM aggregate_test_100
----
2.5

statement error DataFusion error: This feature is not implemented: VAR\(DISTINCT\) aggregations are not available
SELECT var(c2), var(distinct c2) FROM aggregate_test_100

Why should the first query succeed but not the second one? Feel free to point me to any SQL / datafusion doc.

jayzhan211 · 2024-05-29T15:18:28Z

@jayzhan211 I'm working on a change, but can you help me understand the semantics here:
# csv_query_distinct_variance
query R
SELECT var(distinct c2) FROM aggregate_test_100
----
2.5

statement error DataFusion error: This feature is not implemented: VAR\(DISTINCT\) aggregations are not available
SELECT var(c2), var(distinct c2) FROM aggregate_test_100
Why should the first query succeed but not the second one? Feel free to point me to any SQL / datafusion doc.

I think it is because of optimize rule SingleDistinctToGroupBy, this rule convert distinct to group by, so the first query is no longer distinct, you can try adding explain to see the optimized logical plan.

yyin-dev · 2024-05-29T17:27:03Z

@jayzhan211 I'm working on a change, but can you help me understand the semantics here:
# csv_query_distinct_variance
query R
SELECT var(distinct c2) FROM aggregate_test_100
----
2.5

statement error DataFusion error: This feature is not implemented: VAR\(DISTINCT\) aggregations are not available
SELECT var(c2), var(distinct c2) FROM aggregate_test_100
Why should the first query succeed but not the second one? Feel free to point me to any SQL / datafusion doc.
I think it is because of optimize rule SingleDistinctToGroupBy, this rule convert distinct to group by, so the first query is no longer distinct, you can try adding explain to see the optimized logical plan.

Ah that makes sense. Thanks!

yyin-dev · 2024-06-01T21:23:54Z

@jayzhan211 I'm working on a change, but can you help me understand the semantics here:
# csv_query_distinct_variance
query R
SELECT var(distinct c2) FROM aggregate_test_100
----
2.5

statement error DataFusion error: This feature is not implemented: VAR\(DISTINCT\) aggregations are not available
SELECT var(c2), var(distinct c2) FROM aggregate_test_100
Why should the first query succeed but not the second one? Feel free to point me to any SQL / datafusion doc.
I think it is because of optimize rule SingleDistinctToGroupBy, this rule convert distinct to group by, so the first query is no longer distinct, you can try adding explain to see the optimized logical plan.

I'm thinking about the right way to implement error-raising. Before migration, the logic was implemented in physical-exp/src/aggregate/build_in.rs:create_aggregate_expr as a match statement.

After migration, the error should probably be raised in phyical-expr-common/src/aggregate/mod.rs:create_aggregate_expr. There are two options:

Get the udaf's name and implement similar logic. This is simpler but less principled?
Adds a fun support_distinct(&self) -> bool to the AggregateUDFImpl trait. This feels like a better solution.

What do you think?

jayzhan211 · 2024-06-02T00:19:10Z

@jayzhan211 I'm working on a change, but can you help me understand the semantics here:
# csv_query_distinct_variance
query R
SELECT var(distinct c2) FROM aggregate_test_100
----
2.5

statement error DataFusion error: This feature is not implemented: VAR\(DISTINCT\) aggregations are not available
SELECT var(c2), var(distinct c2) FROM aggregate_test_100
Why should the first query succeed but not the second one? Feel free to point me to any SQL / datafusion doc.
I think it is because of optimize rule SingleDistinctToGroupBy, this rule convert distinct to group by, so the first query is no longer distinct, you can try adding explain to see the optimized logical plan.
I'm thinking about the right way to implement error-raising. Before migration, the logic was implemented in physical-exp/src/aggregate/build_in.rs:create_aggregate_expr as a match statement.

After migration, the error should probably be raised in phyical-expr-common/src/aggregate/mod.rs:create_aggregate_expr. There are two options:

Get the udaf's name and implement similar logic. This is simpler but less principled?

Adds a fun support_distinct(&self) -> bool to the AggregateUDFImpl trait. This feels like a better solution.

What do you think?

We have is_distinct in AccumulatorArgs, so you can return errors if you found is_distinct is true

jayzhan211 · 2024-06-04T07:21:35Z

@yyin-dev There are some error left to fix

You can try ./dev/rust_lint.sh, cargo test --test sqllogictests and cargo test --lib --tests --bins --features avro,json before running the CI

yyin-dev · 2024-06-04T12:11:40Z

@yyin-dev There are some error left to fix

You can try ./dev/rust_lint.sh, cargo test --test sqllogictests and cargo test --lib --tests --bins --features avro,json before running the CI

Thanks! I just ran the 3 commands and fixed all problems. Can you trigger the CI again? I don't think I can trigger it myself.

alamb

Thank you @yyin-dev and @jayzhan211 -- I think this looks really nice. I had one very minor comment about use but otherwise I think this is ready to go

alamb · 2024-06-04T16:26:31Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

@@ -651,6 +653,7 @@ async fn roundtrip_expr_api() -> Result<()> {
        covar_pop(lit(1.5), lit(2.2)),
        sum(lit(1)),
        median(lit(2)),
+        var_sample(lit(2.2)),


alamb · 2024-06-04T16:27:22Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

+use datafusion::functions_aggregate::covariance::{covar_pop, covar_samp};
+use datafusion::functions_aggregate::expr_fn::first_value;


Can you please change this to use the exports from datafusion::functions_aggregate::expr_fn (which I think is the intended external facing API)?

Updated to use exports from expr_fn.

alamb · 2024-06-04T16:27:31Z

datafusion/proto/src/logical_plan/from_proto.rs

@@ -149,7 +149,6 @@ impl From<protobuf::AggregateFunction> for AggregateFunction {
            protobuf::AggregateFunction::Count => Self::Count,
            protobuf::AggregateFunction::ApproxDistinct => Self::ApproxDistinct,
            protobuf::AggregateFunction::ArrayAgg => Self::ArrayAgg,
-            protobuf::AggregateFunction::Variance => Self::Variance,


jayzhan211

👍

jayzhan211 · 2024-06-05T05:18:40Z

Thanks @yyin-dev and @alamb

* Without migrating tests * Should fail VAR(DISTINCT) but doesn't * Pass all other tests. * Return error for var(distinct) * Migrate tests * Fix tests * Lint * Fix tests * Fix use

yyin-dev added 2 commits May 28, 2024 22:44

Without migrating tests

2b6ccd3

Should fail VAR(DISTINCT) but doesn't

e4e9e57

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels May 29, 2024

yyin-dev marked this pull request as draft May 29, 2024 12:10

yyin-dev and others added 2 commits May 29, 2024 08:12

Merge branch 'main' into convert-variance-sample-to-udaf

d5841bb

Pass all other tests.

758ff77

yyin-dev added 2 commits June 3, 2024 22:52

Return error for var(distinct)

09fecca

Migrate tests

d9a1562

yyin-dev marked this pull request as ready for review June 4, 2024 03:24

yyin-dev marked this pull request as draft June 4, 2024 03:28

yyin-dev force-pushed the convert-variance-sample-to-udaf branch from 4fbdb47 to d9a1562 Compare June 4, 2024 03:32

Merge branch 'main' into convert-variance-sample-to-udaf

a26170a

yyin-dev force-pushed the convert-variance-sample-to-udaf branch from 4e043ac to a26170a Compare June 4, 2024 03:45

Fix tests

f3aea7c

yyin-dev marked this pull request as ready for review June 4, 2024 03:51

yyin-dev added 2 commits June 4, 2024 07:48

Lint

d5d43ef

Fix tests

65da63c

Merge branch 'main' into convert-variance-sample-to-udaf

338550b

alamb approved these changes Jun 4, 2024

View reviewed changes

Fix use

590c043

jayzhan211 approved these changes Jun 5, 2024

View reviewed changes

jayzhan211 merged commit 70744d5 into apache:main Jun 5, 2024
25 checks passed

This was referenced Jun 7, 2024

Convert stddev to udaf #10827

Closed

Convert approx_distinct to UDAF #10837

Closed

Convert approx_median to UDAF #10838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert variance sample to udaf #10713

Convert variance sample to udaf #10713

yyin-dev commented May 29, 2024 •

edited

Loading

yyin-dev commented May 29, 2024 •

edited

Loading

jayzhan211 commented May 29, 2024

yyin-dev commented May 29, 2024

yyin-dev commented Jun 1, 2024 •

edited

Loading

jayzhan211 commented Jun 2, 2024

jayzhan211 commented Jun 4, 2024 •

edited

Loading

yyin-dev commented Jun 4, 2024

alamb left a comment •

edited

Loading

alamb Jun 4, 2024

alamb Jun 4, 2024

yyin-dev Jun 5, 2024

alamb Jun 4, 2024

jayzhan211 left a comment

jayzhan211 commented Jun 5, 2024

		use datafusion::functions_aggregate::covariance::{covar_pop, covar_samp};
		use datafusion::functions_aggregate::expr_fn::first_value;

Convert variance sample to udaf #10713

Convert variance sample to udaf #10713

Conversation

yyin-dev commented May 29, 2024 • edited Loading

Which issue does this PR close?

Are these changes tested?

yyin-dev commented May 29, 2024 • edited Loading

jayzhan211 commented May 29, 2024

yyin-dev commented May 29, 2024

yyin-dev commented Jun 1, 2024 • edited Loading

jayzhan211 commented Jun 2, 2024

jayzhan211 commented Jun 4, 2024 • edited Loading

yyin-dev commented Jun 4, 2024

alamb left a comment • edited Loading

Choose a reason for hiding this comment

alamb Jun 4, 2024

Choose a reason for hiding this comment

alamb Jun 4, 2024

Choose a reason for hiding this comment

yyin-dev Jun 5, 2024

Choose a reason for hiding this comment

alamb Jun 4, 2024

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

jayzhan211 commented Jun 5, 2024

yyin-dev commented May 29, 2024 •

edited

Loading

yyin-dev commented May 29, 2024 •

edited

Loading

yyin-dev commented Jun 1, 2024 •

edited

Loading

jayzhan211 commented Jun 4, 2024 •

edited

Loading

alamb left a comment •

edited

Loading