huan233usc commented on code in PR #805:
URL: https://github.com/apache/iceberg-cpp/pull/805#discussion_r3624711483
##########
src/iceberg/expression/literal.cc:
##########
@@ -95,6 +105,85 @@ Literal
LiteralCaster::AboveMaxLiteral(std::shared_ptr<PrimitiveType> type) {
return Literal(Literal::AboveMax{}, std::move(type));
}
+namespace {
+
+// 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);
+ if (remainder * Decimal(2) >= divisor) {
+ quotient += negative ? Decimal(-1) : Decimal(1);
+ }
Review Comment:
Fixed in cf1dd9e4: HALF_UP now compares `remainder >= divisor / 2` so we
don't overflow `int128` when `drop` is near `kMaxScale`.
##########
src/iceberg/expression/literal.cc:
##########
@@ -95,6 +105,85 @@ Literal
LiteralCaster::AboveMaxLiteral(std::shared_ptr<PrimitiveType> type) {
return Literal(Literal::AboveMax{}, std::move(type));
}
+namespace {
+
+// 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);
+ if (remainder * Decimal(2) >= divisor) {
+ 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);
+ // 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));
Review Comment:
Fixed in cf1dd9e4: both integer and real paths reject a target scale outside
`[-kMaxScale, kMaxScale]` via `ValidateDecimalScale` (with regression tests for
`-40`).
##########
src/iceberg/expression/literal.cc:
##########
@@ -95,6 +105,85 @@ Literal
LiteralCaster::AboveMaxLiteral(std::shared_ptr<PrimitiveType> type) {
return Literal(Literal::AboveMax{}, std::move(type));
}
+namespace {
+
+// 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);
+ if (remainder * Decimal(2) >= divisor) {
+ 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);
+ // 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);
+ if (!std::isfinite(value)) {
+ return InvalidArgument("Cannot cast {} as a {} value", value,
+ target_type->ToString());
+ }
+
+ // Parse the shortest round-tripping decimal representation of the value
(matching
+ // Java's BigDecimal.valueOf(double), which goes through Double.toString)
into a
+ // full-precision decimal, then round to the target scale.
+ int32_t parsed_scale = 0;
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto parsed, Decimal::FromString(std::format("{}", value), nullptr,
&parsed_scale));
Review Comment:
Fixed in cf1dd9e4: switched to `std::to_chars` (no format specifier) for the
shortest round-trip decimal string.
On float→decimal we still widen to `double` first, which matches Java
Iceberg: `FloatLiteral.to(DECIMAL)` calls `BigDecimal.valueOf(value())`, and
that overload takes `double` (float is promoted), so it uses `Double.toString`
of the widened value rather than `Float.toString`. Splitting
float/`Float.toString` would diverge from Java.
Added a regression for a value that needs more than 6 significant digits
(`1.23456789 → decimal(20,8)`).
##########
src/iceberg/expression/literal.cc:
##########
@@ -95,6 +105,85 @@ Literal
LiteralCaster::AboveMaxLiteral(std::shared_ptr<PrimitiveType> type) {
return Literal(Literal::AboveMax{}, std::move(type));
}
+namespace {
+
+// 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);
+ if (remainder * Decimal(2) >= divisor) {
+ 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);
+ // 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);
+ if (!std::isfinite(value)) {
+ return InvalidArgument("Cannot cast {} as a {} value", value,
+ target_type->ToString());
+ }
+
+ // Parse the shortest round-tripping decimal representation of the value
(matching
+ // Java's BigDecimal.valueOf(double), which goes through Double.toString)
into a
+ // full-precision decimal, then round to the target scale.
+ int32_t parsed_scale = 0;
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto parsed, Decimal::FromString(std::format("{}", value), nullptr,
&parsed_scale));
+
+ ICEBERG_ASSIGN_OR_RAISE(auto unscaled, RescaleHalfUp(parsed, parsed_scale,
+ decimal_type.scale(),
value < 0));
Review Comment:
Fixed in cf1dd9e4: same `ValidateDecimalScale` guard as the integer path.
--
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]
