huan233usc commented on code in PR #805:
URL: https://github.com/apache/iceberg-cpp/pull/805#discussion_r3678249020
##########
src/iceberg/expression/literal.cc:
##########
@@ -95,6 +109,105 @@ Literal
LiteralCaster::AboveMaxLiteral(std::shared_ptr<PrimitiveType> type) {
return Literal(Literal::AboveMax{}, std::move(type));
}
+namespace {
+
+Status ValidateDecimalScale(int32_t scale) {
+ if (scale < -Decimal::kMaxScale || scale > Decimal::kMaxScale) {
+ return InvalidArgument("decimal scale must be in range [-{}, {}], was {}",
+ Decimal::kMaxScale, Decimal::kMaxScale, scale);
+ }
+ return {};
+}
+
+// Rescale `unscaled` (interpreted at `from_scale`) to `to_scale` using
HALF_UP rounding
+// (round half away from zero), matching Java's BigDecimal.setScale(scale,
HALF_UP).
+// Unlike Decimal::Rescale, which only truncates and rejects any dropped
remainder, this
+// rounds, and it supports the full negative..positive scale range Iceberg
decimals allow.
+Result<Decimal> RescaleHalfUp(const Decimal& unscaled, int32_t from_scale,
+ int32_t to_scale, bool negative) {
+ const int32_t delta = to_scale - from_scale;
+ if (delta == 0) {
+ return unscaled;
+ }
+ if (delta > 0) {
+ // Growing the scale multiplies by 10^delta and is exact; Rescale rejects
overflow.
+ if (delta > Decimal::kMaxScale) {
+ return InvalidArgument("scale change {} exceeds the maximum {}", delta,
+ Decimal::kMaxScale);
+ }
+ return unscaled.Rescale(from_scale, to_scale);
+ }
+ // Shrinking the scale drops `drop` digits with HALF_UP rounding. A drop
larger than the
+ // digits any decimal can hold rounds everything away, so the result is zero
(e.g.
+ // 1e-100 to decimal(9, 2)); this also keeps the divisor within the
powers-of-ten table.
+ const int32_t drop = -delta;
+ if (drop > Decimal::kMaxScale) {
+ return Decimal(0);
+ }
+ ICEBERG_ASSIGN_OR_RAISE(auto divisor, Decimal(1).Rescale(0, drop));
+ ICEBERG_ASSIGN_OR_RAISE(auto divmod, unscaled.Divide(divisor));
+ Decimal quotient = divmod.first;
+ Decimal remainder = Decimal::Abs(divmod.second);
+ // Compare against divisor/2 rather than remainder*2: for drop near
kMaxScale,
+ // remainder*2 can overflow int128 and flip the HALF_UP decision. Powers of
ten
+ // with drop >= 1 are always even, so the two comparisons are equivalent.
+ if (remainder >= divisor / Decimal(2)) {
+ quotient += negative ? Decimal(-1) : Decimal(1);
+ }
+ return quotient;
+}
+
+} // namespace
+
+Result<Literal> LiteralCaster::CastIntegerToDecimal(
+ int64_t value, const std::shared_ptr<PrimitiveType>& target_type) {
+ const auto& decimal_type = internal::checked_cast<const
DecimalType&>(*target_type);
+ ICEBERG_RETURN_UNEXPECTED(ValidateDecimalScale(decimal_type.scale()));
+ // An integer has scale 0; rescale it to the target scale, rounding HALF_UP
when the
+ // target scale is negative (matching Java's numeric-to-decimal default
handling).
+ ICEBERG_ASSIGN_OR_RAISE(auto unscaled, RescaleHalfUp(Decimal(value),
/*from_scale=*/0,
+ decimal_type.scale(),
value < 0));
+ if (!unscaled.FitsInPrecision(decimal_type.precision())) {
+ return InvalidArgument("Cannot cast {} as a {} value", value,
+ target_type->ToString());
+ }
+ return Literal::Decimal(unscaled.value(), decimal_type.precision(),
+ decimal_type.scale());
+}
+
+Result<Literal> LiteralCaster::CastRealToDecimal(
+ double value, const std::shared_ptr<PrimitiveType>& target_type) {
+ const auto& decimal_type = internal::checked_cast<const
DecimalType&>(*target_type);
+ ICEBERG_RETURN_UNEXPECTED(ValidateDecimalScale(decimal_type.scale()));
+ if (!std::isfinite(value)) {
+ return InvalidArgument("Cannot cast {} as a {} value", value,
+ target_type->ToString());
+ }
+
+ // Convert via the shortest round-tripping decimal string (std::to_chars
without a
+ // format specifier), then round to the target scale. Float callers widen to
double
+ // first, so both float and double sources share this path.
+ std::array<char, 64> buf{};
+ auto [ptr, ec] = std::to_chars(buf.data(), buf.data() + buf.size(), value);
+ if (ec != std::errc{}) {
+ return InvalidArgument("Cannot cast {} as a {} value", value,
+ target_type->ToString());
+ }
+ int32_t parsed_scale = 0;
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto parsed,
+ Decimal::FromString(std::string_view(buf.data(), ptr), nullptr,
&parsed_scale));
Review Comment:
Good catch. Now I parse the coefficient and its scale directly from the
`to_chars` output (via a small `ParseRealCoefficient` helper) instead of going
through `Decimal::FromString`, which was normalizing the negative scale by
multiplying the coefficient by 10^-scale and overflowing int128. The
coefficient itself always fits, and `RescaleHalfUp` combines the exponent with
the target scale and rejects true overflow. Added regressions for both `4e38 ->
decimal(38, 0)` (rejected) and `1e39 -> decimal(2, -38)` (accepted as 10).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
