Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: Move group accumulator for aggregate function to physical-expr-common, and add ahash physical-expr-common #10574

Merged
merged 8 commits into from
May 22, 2024

Conversation

jayzhan211
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

  1. add ahash for common, used for distinct count accumulator Move Count to functions-aggregate, update MSRV to rust 1.75 #10484
  2. move other group accumulator module that is used in aggregate function.
  3. adapter is used in physical plan is not used in aggregate function, so no need to move it.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate labels May 19, 2024
Signed-off-by: jayzhan211 <[email protected]>
@jayzhan211 jayzhan211 marked this pull request as ready for review May 19, 2024 08:42
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- thanks @jayzhan211

@@ -59,6 +59,9 @@ version = "38.0.0"
# for the inherited dependency but cannot do the reverse (override from true to false).
#
# See for more detaiils: https://github.com/rust-lang/cargo/issues/11329
ahash = { version = "0.8", default-features = false, features = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// specific language governing permissions and limitations
// under the License.

pub mod accumulate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could add a note that this module is for implementing GroupsAccumulator

Suggested change
pub mod accumulate;
//! Utilities for implementing [`GroupsAccumulator`]
pub mod accumulate;

mod adapter;
pub use accumulate::NullState;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for backwards compatibility

Maybe we can leave a comment explaining it is for backwards compatibility

Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
@jayzhan211
Copy link
Contributor Author

Thanks @alamb

@jayzhan211 jayzhan211 merged commit 2eb38bd into apache:main May 22, 2024
25 checks passed
@alamb
Copy link
Contributor

alamb commented May 22, 2024

🚀

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
…-common, and add ahash physical-expr-common (apache#10574)

* ahash workspace

Signed-off-by: jayzhan211 <[email protected]>

* move other utils

Signed-off-by: jayzhan211 <[email protected]>

* move NullState

Signed-off-by: jayzhan211 <[email protected]>

* move PrimitiveGroupsAccumulator

Signed-off-by: jayzhan211 <[email protected]>

* move boolop

Signed-off-by: jayzhan211 <[email protected]>

* move deciamlavg

Signed-off-by: jayzhan211 <[email protected]>

* add comment

Signed-off-by: jayzhan211 <[email protected]>

* fix doc

Signed-off-by: jayzhan211 <[email protected]>

---------

Signed-off-by: jayzhan211 <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants