Convert BitAnd, BitOr, BitXor to UDAF #10930

dharanad · 2024-06-15T18:23:24Z

Which issue does this PR close?

Closes #10907 and is part of #8708

Rationale for this change

What changes are included in this PR?

moved BitAnd, BitOr, BitXor to UDAF

Are these changes tested?

Running existing test cases

cargo build
cargo test
cargo test --test sqllogictests

Are there any user-facing changes?

…o-udaf # Conflicts: # datafusion/functions-aggregate/src/lib.rs # datafusion/physical-expr/src/aggregate/build_in.rs # datafusion/proto/src/physical_plan/to_proto.rs

dharanad · 2024-06-16T18:08:21Z

@jayzhan211 / @alamb can you please help me with a review here.

jayzhan211

Also, remember to add the test in roundtrip_expr_api in datafusion/proto/tests/cases/roundtrip_logical_plan.rs

datafusion/functions-aggregate/src/bit_and_or_xor.rs

dharanad · 2024-06-17T06:18:56Z

cc: @jayzhan211 I have made the changes and also updated the UT.

jayzhan211 · 2024-06-17T08:55:27Z

datafusion/physical-expr/src/aggregate/bit_and_or_xor.rs

-        true
-    }
-
-    fn create_groups_accumulator(&self) -> Result<Box<dyn GroupsAccumulator>> {


I think create_groups_accumulator is not supported in new UDAF, but we should

Agreed. Can you help me, how to do i test groups accumulator ?

I don't understand this comment @jayzhan211

It seems like the groups accumulator code was moved from the existing function here

https://github.com/apache/datafusion/pull/10930/files#diff-9b0874795d7dc847d4137426e205de0a2e3b0b56c03ad80d55b000aa020ca132L438-L462

Are you saying we shouldn't support the groups accumulator? Or that we should and it is missing from some places?

Update -- NM I see now now that @dharanad added the groups accumulator code

alamb

Thank you @dharanad -- this is so cool to see

alamb · 2024-06-17T11:38:58Z

datafusion/physical-expr/src/aggregate/bit_and_or_xor.rs

-        true
-    }
-
-    fn create_groups_accumulator(&self) -> Result<Box<dyn GroupsAccumulator>> {


I don't understand this comment @jayzhan211

It seems like the groups accumulator code was moved from the existing function here

https://github.com/apache/datafusion/pull/10930/files#diff-9b0874795d7dc847d4137426e205de0a2e3b0b56c03ad80d55b000aa020ca132L438-L462

Are you saying we shouldn't support the groups accumulator? Or that we should and it is missing from some places?

alamb

Thank you @dharanad -- this looks good to me.

Let's wait for @jayzhan211 to give it another review as well

jayzhan211 · 2024-06-17T13:06:12Z

👍 Thanks @dharanad and @alamb

dharanad · 2024-06-17T13:19:21Z

Thank you very much @jayzhan211 and @alamb for your help and time.

alamb · 2024-06-17T13:26:38Z

Thank you for the great initial contribution @dharanad

* remove bit and or xor from expr * remove bit and or xor from physical expr and proto * add proto regen changes * impl BitAnd, BitOr, BitXor UADF * add support for float * removing support for float * refactor helper macros * clippy'fy * simplify Bitwise operation * add documentation * formatting * fix lint issue * remove XorDistinct * update roundtrip_expr_api test * linting * support groups accumulator

The functions now take a single expression instead of a Vec<_>. Ref: apache/datafusion#10930

* chore: update datafusion deps * feat: impl ExecutionPlan::static_name() for DatasetExec This required trait method was added upstream [0] and recommends to simply forward to `static_name`. [0]: apache/datafusion#10266 * feat: update first_value and last_value wrappers. Upstream signatures were changed for the new new `AggregateBuilder` api [0]. This simply gets the code to work. We should better incorporate that API into `datafusion-python`. [0] apache/datafusion#10560 * migrate count to UDAF Builtin Count was removed upstream. TBD whether we want to re-implement `count_star` with new API. Ref: apache/datafusion#10893 * migrate approx_percentile_cont, approx_distinct, and approx_median to UDAF Ref: approx_distinct apache/datafusion#10851 Ref: approx_median apache/datafusion#10840 Ref: approx_percentile_cont and _with_weight apache/datafusion#10917 * migrate avg to UDAF Ref: apache/datafusion#10964 * migrage corr to UDAF Ref: apache/datafusion#10884 * migrate grouping to UDAF Ref: apache/datafusion#10906 * add alias `mean` for UDAF `avg` * migrate stddev to UDAF Ref: apache/datafusion#10827 * remove rust alias for stddev The python wrapper now provides stddev_samp alias. * migrage var_pop to UDAF Ref: apache/datafusion#10836 * migrate regr_* functions to UDAF Ref: apache/datafusion#10898 * migrate bitwise functions to UDAF The functions now take a single expression instead of a Vec<_>. Ref: apache/datafusion#10930 * add missing variants for ScalarValue with todo * fix typo in approx_percentile_cont * add distinct arg to count * comment out failing test `approx_percentile_cont` is now returning a DoubleArray instead of an IntArray. This may be a bug upstream; it requires further investigation. * update tests to expect lowercase `sum` in query plans This was changed upstream. Ref: apache/datafusion#10831 * update ScalarType data_type map * add docs dependency pickleshare * re-implement count_star * lint: ruff python lint * lint: rust cargo fmt * include name of window function in error for find_window_fn * refactor `find_window_fn` for debug clarity * search default aggregate functions by both name and aliases The alias list no longer includes the name of the function. Ref: apache/datafusion#10658 * fix markdown in find_window_fn docs * parameterize test_window_functions `first_value` and `last_value` are currently failing and marked as xfail. * add test ids to test_simple_select tests marked xfail * update find_window_fn to search built-ins first The behavior of `first_value` and `last_value` UDAFs currently does not match the built-in behavior. This allowed me to remove `marks=pytest.xfail` from the window tests. * improve first_call and last_call use of the builder API * remove trailing todos * fix examples/substrait.py * chore: remove explicit aliases from functions.rs Ref: #779 * remove `array_fn!` aliases * remove alias rules for `expr_fn_vec!` * remove alias rules from `expr_fn!` macro * remove unnecessary pyo3 var-arg signatures in functions.rs * remove pyo3 signatures that provided defaults for first_value and last_value * parametrize test_string_functions * test regr_ function wrappers Closes #778

remove bit and or xor from expr

757bba6

github-actions bot added the logical-expr Logical plan and expressions label Jun 15, 2024

dharanad changed the title ~~remove bit and or xor from expr~~ Convert BitAnd, BitOr, BitXor to UDAF Jun 15, 2024

remove bit and or xor from physical expr and proto

eb0c870

github-actions bot added the physical-expr Physical Expressions label Jun 15, 2024

dharanad and others added 9 commits June 16, 2024 00:34

add proto regen changes

f5cc74d

impl BitAnd, BitOr, BitXor UADF

418f37b

add support for float

9c3e9b7

removing support for float

d481ccc

refactor helper macros

0d25cc0

clippy'fy

5d339cb

simplify Bitwise operation

74fc16c

Merge branch 'apache:main' into dharanad/convert-bit-and-or-xor-to-udaf

bf99ed9

add documentation

ae6879e

dharanad marked this pull request as ready for review June 16, 2024 16:31

dharanad added 3 commits June 16, 2024 22:51

Merge branch 'refs/heads/main' into dharanad/convert-bit-and-or-xor-t…

e965207

…o-udaf # Conflicts: # datafusion/functions-aggregate/src/lib.rs # datafusion/physical-expr/src/aggregate/build_in.rs # datafusion/proto/src/physical_plan/to_proto.rs

formatting

41b64c9

fix lint issue

66c5b8a

jayzhan211 reviewed Jun 17, 2024

View reviewed changes

datafusion/functions-aggregate/src/bit_and_or_xor.rs Outdated Show resolved Hide resolved

datafusion/functions-aggregate/src/bit_and_or_xor.rs Outdated Show resolved Hide resolved

dharanad added 3 commits June 17, 2024 11:05

remove XorDistinct

e453f78

update roundtrip_expr_api test

a8f3c80

linting

356c557

dharanad requested a review from jayzhan211 June 17, 2024 08:42

jayzhan211 reviewed Jun 17, 2024

View reviewed changes

support groups accumulator

7829982

alamb reviewed Jun 17, 2024

View reviewed changes

alamb approved these changes Jun 17, 2024

View reviewed changes

jayzhan211 merged commit 2daadb7 into apache:main Jun 17, 2024
23 checks passed

dharanad deleted the dharanad/convert-bit-and-or-xor-to-udaf branch June 17, 2024 15:46

alamb mentioned this pull request Jun 17, 2024

DataFusion weekly project plan (Andrew Lamb) - June 17, 2024 #10955

Closed

5 tasks

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 24, 2024

migrate bitwise functions to UDAF

e533bee

The functions now take a single expression instead of a Vec<_>. Ref: apache/datafusion#10930

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 24, 2024

migrate bitwise functions to UDAF

bbe02ff

The functions now take a single expression instead of a Vec<_>. Ref: apache/datafusion#10930

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 25, 2024

migrate bitwise functions to UDAF

8881e2a

The functions now take a single expression instead of a Vec<_>. Ref: apache/datafusion#10930

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert BitAnd, BitOr, BitXor to UDAF #10930

Convert BitAnd, BitOr, BitXor to UDAF #10930

dharanad commented Jun 15, 2024 •

edited

Loading

dharanad commented Jun 16, 2024

jayzhan211 left a comment

dharanad commented Jun 17, 2024

jayzhan211 Jun 17, 2024

dharanad Jun 17, 2024 •

edited

Loading

alamb Jun 17, 2024

alamb Jun 17, 2024

alamb left a comment

alamb Jun 17, 2024

alamb left a comment

jayzhan211 commented Jun 17, 2024

dharanad commented Jun 17, 2024

alamb commented Jun 17, 2024

Convert BitAnd, BitOr, BitXor to UDAF #10930

Convert BitAnd, BitOr, BitXor to UDAF #10930

Conversation

dharanad commented Jun 15, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

dharanad commented Jun 16, 2024

jayzhan211 left a comment

Choose a reason for hiding this comment

dharanad commented Jun 17, 2024

jayzhan211 Jun 17, 2024

Choose a reason for hiding this comment

dharanad Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Jun 17, 2024

Choose a reason for hiding this comment

alamb Jun 17, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jun 17, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

jayzhan211 commented Jun 17, 2024

dharanad commented Jun 17, 2024

alamb commented Jun 17, 2024

dharanad commented Jun 15, 2024 •

edited

Loading

dharanad Jun 17, 2024 •

edited

Loading