Move remaining decimal adapters into the decimal submodule - #11078
Open
anandghegde wants to merge 1 commit into
Open
anandghegde wants to merge 1 commit into
anandghegde wants to merge 1 commit into
Conversation
Follow-up to apache#5125, completing the extractions in apache#5537 and friends. Move the integer-to-decimal and generic decimal routing helpers out of `arrow-cast/src/cast/mod.rs` into the existing `decimal` submodule rather than a new one, leaving `cast_with_options` as the top-level dispatcher. The bodies are moved verbatim. `cast_from_decimal` and `cast_to_decimal` are called from the dispatcher and become `pub(crate)`; `integer_to_decimal_native` and `cast_integer_to_decimal` have no callers outside the moved code and stay private. `single_decimal_to_float_lossy` is public API. `decimal` is a private module imported with a plain `use`, so moving it there would have made it unreachable; it is re-exported explicitly from `mod.rs` to keep `arrow_cast::cast::single_decimal_to_float_lossy` resolving as before. No behavioural change and no public API change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #11032 — the remaining decimal adapters group only.
Sibling PRs against the same issue: #11075 (temporal), #11076 (numeric and
boolean), #11077 (binary and byte-view). With this one, the four groups I took
cover everything in the issue's list except the struct helpers, which
@amitvijapur mentioned taking. All four are independent and move disjoint
functions, but each touches the imports at the top of
mod.rs, so whicheverlands later will want a trivial rebase.
Rationale for this change
arrow-cast/src/cast/mod.rsis ~14k lines. #5125 moved list, decimal,dictionary, string, map, run-array and union support into private submodules,
and #11032 lists the groups still inline. The integer-to-decimal and generic
decimal routing helpers are the last of them.
What changes are included in this PR?
arrow-cast/src/cast/decimal.rsgains:single_decimal_to_float_lossyinteger_to_decimal_nativecast_integer_to_decimalcast_from_decimalcast_to_decimalcast_with_optionsstays the top-level dispatcher inmod.rs.Worth flagging for review:
decimalmodule, not a new one. The issuecalls them "remaining decimal adapters" and
cast/decimal.rsalready exists,so a second decimal module seemed like the wrong shape.
single_decimal_to_float_lossyneeded an explicit re-export, and this isthe one thing here that is not a pure move. It is
puband reachable todayas
arrow_cast::cast::single_decimal_to_float_lossy.decimalis a privatemodule that
mod.rspulls in with a plainuse, so moving the function theresilently made it private — a breaking change that compiles and passes the
whole test suite.
mod.rsnow re-exports that one name explicitly, whichrestores the path without exposing anything else from the module.
cast_from_decimalandcast_to_decimalare calledby the dispatcher and became
pub(crate).integer_to_decimal_nativeandcast_integer_to_decimalhave no callers outside the moved code and stayprivate.
decimal.rs's own#[cfg(test)] mod tests, sinceclippy rejects items after a test module.
Are these changes tested?
Covered by the existing
arrow-casttests — this is a code move, so no newtests are warranted and no existing test was modified.
Three mechanical checks rather than a reading, since a code move is only worth
anything if it is faithful:
HEADonce the added
pub(crate)is normalised away.decimal.rs's pre-existing content, and its test module, are unchanged —checked by comparing the regions either side of the insertion point. The diff
is a symmetric +259/-259 apart from the re-export.
arrow_cast::cast::single_decimal_to_float_lossythrough its public path and passes. This is what caught the privacy
regression described above; it is not part of this diff.
Run on macOS 26.6.2, aarch64-apple-darwin, toolchain 1.98.1 from
rust-toolchain.toml.Are there any user-facing changes?
No — but only because of the explicit re-export. Without it this would have been
a silent breaking change for anyone calling
arrow_cast::cast::single_decimal_to_float_lossy.AI usage disclosure
Written with AI assistance (Claude). The AI performed the mechanical extraction
and ran the verification above; I reviewed the result and own the change.
The moved bodies are unedited, and the byte-identity check is the evidence for
that. The judgement calls are listed above: reusing the existing
decimalmodule, the explicit re-export, the visibility split, and the insertion point.
The re-export in particular is worth a reviewer's attention, because it is the
only line here that is not a move, and nothing in the test suite would have
failed without it.