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

refactor: split group_column.rs into multiple submodules #13378

Closed

Conversation

Chen-Yuan-Lai
Copy link

Which issue does this PR close?

Closes #13262.

Rationale for this change

What changes are included in this PR?

  1. separate /group_column.rs into /multi_column/primitive.rs, /multi_column/byte.rs, and /multi_column/byte_view.rs
  2. create /multi_column/mod.rs and place shared trait and function in this file

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Physical Expressions label Nov 12, 2024
Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍

@jayzhan211
Copy link
Contributor

jayzhan211 commented Nov 13, 2024

@Chen-Yuan-Lai You can run ./dev/rust_lint.sh before pushing the code

ByteGroupValueBuilder, ByteViewGroupValueBuilder, GroupColumn,
PrimitiveGroupValueBuilder,
};
use crate::aggregates::group_values::multi_column::byte::ByteGroupValueBuilder;
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to merge them together

@alamb alamb changed the title refactor: split group_column.rs into multiple submoules refactor: split group_column.rs into multiple submodules Nov 13, 2024
@Chen-Yuan-Lai
Copy link
Author

@jayzhan211 thanks for your review! but the other PR (#13352) with the same issue has already been merged. @Rachelint you may need to reorganize codes again based on his change?

@Rachelint
Copy link
Contributor

@jayzhan211 thanks for your review! but the other PR (#13352) with the same issue has already been merged. @Rachelint you may need to reorganize codes again based on his change?

Yes.

@alamb
Copy link
Contributor

alamb commented Nov 13, 2024

#13392 -- I think has the follow on improvements

@Chen-Yuan-Lai Chen-Yuan-Lai deleted the refactor_group_column branch November 13, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reorganize the GroupColumn implementations into more coherent modules
4 participants