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 some decimal-related code and tests #7062

Merged
merged 6 commits into from
Feb 8, 2025

Conversation

CurtHagenlocher
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher commented Feb 1, 2025

Which issue does this PR close?

Part of addressing #6661 but does not close it.

Rationale for this change

This change refactors some decimal-related code and tests in preparation for adding Decimal32 and Decimal64 support. It was factored out of #7061 to reduce the code review burden.

What changes are included in this PR?

Refactoring of decimal-related code and tests.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Feb 1, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 10 changed files in this pull request and generated 3 comments.

Files not reviewed (4)
  • arrow-cast/src/cast/decimal.rs: Evaluated as low risk
  • arrow/benches/builder.rs: Evaluated as low risk
  • arrow/tests/array_cast.rs: Evaluated as low risk
  • arrow-data/src/data.rs: Evaluated as low risk
Comments suppressed due to low confidence (1)

arrow-ord/src/sort.rs:1936

  • The parameters 'precision' and 'scale' should be consistent with the data type used in 'create_decimal_array'.
precision: u8, scale: i8

arrow-ord/src/sort.rs Show resolved Hide resolved
arrow-ord/src/sort.rs Show resolved Hide resolved
arrow-ord/src/sort.rs Show resolved Hide resolved
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Some initial thoughts

arrow-schema/src/ffi.rs Outdated Show resolved Hide resolved
arrow-cast/src/cast/dictionary.rs Outdated Show resolved Hide resolved
arrow-cast/src/cast/dictionary.rs Outdated Show resolved Hide resolved
arrow-cast/src/cast/decimal.rs Outdated Show resolved Hide resolved
arrow-cast/src/cast/decimal.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Seems good to me 👍

arrow-ord/src/sort.rs Outdated Show resolved Hide resolved
arrow-ord/src/sort.rs Show resolved Hide resolved
arrow-cast/src/cast/mod.rs Outdated Show resolved Hide resolved
@CurtHagenlocher CurtHagenlocher merged commit 1081d01 into apache:main Feb 8, 2025
26 checks passed
@CurtHagenlocher CurtHagenlocher deleted the DecimalRefactor branch February 8, 2025 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants