Copilot commented on code in PR #805:
URL: https://github.com/apache/iceberg-cpp/pull/805#discussion_r3619376468
##########
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:
`remainder * Decimal(2)` can overflow `int128` when `drop` is large (e.g.
divisor = 10^38, remainder close to divisor), because `Decimal` multiplication
truncates to 128 bits. That can make the HALF_UP comparison incorrect. Since
`divisor` is always a power of ten (thus divisible by 2), compare against
`divisor / 2` instead to avoid overflow.
##########
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:
`decimal_type.scale()` is not validated here. With an out-of-range
*negative* scale (e.g. -40), `RescaleHalfUp` currently returns 0 for large
drops and the cast can succeed, producing a `Literal::Decimal` with an invalid
scale that later fails in `Decimal::ToString` (which requires scale in [-38,
38]). Consider rejecting any target scale outside `[-Decimal::kMaxScale,
Decimal::kMaxScale]` up front.
##########
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:
The PR description says this matches Java's `Double.toString` /
`Float.toString` "shortest round-tripping" representation, but
`std::format("{}", value)` uses the default `std::formatter<double>` precision
(typically 6 significant digits). That is not round-tripping for many values
(and for the float path you’re formatting a `double`-converted float), so the
decimal produced can differ from Java.
Consider using `std::to_chars(..., std::chars_format::general)` to get a
shortest-roundtrip representation, and do it separately for float vs double so
the float cast matches `Float.toString` semantics.
##########
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:
Same as the integer path: `decimal_type.scale()` is not validated before
rescaling. An out-of-range target scale (especially negative) can slip through
and produce a `Literal::Decimal` with a scale outside `[-38, 38]`, which later
breaks `Decimal::ToString` and other scale-indexed operations.
--
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]
