wgtmac commented on code in PR #805:
URL: https://github.com/apache/iceberg-cpp/pull/805#discussion_r3672993529
##########
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:
`Decimal::FromString` normalizes negative scales to 0 before this rescale,
which can overflow its int128 intermediate. For example,
`Literal::Double(4e38).CastTo(decimal(38, 0))` may wrap `4 * 10^38` to a
38-digit value and accept the wrong literal instead of rejecting it. It also
rejects representable negative-scale cases such as `1e39 -> decimal(2, -38)`.
Could we retain the parsed coefficient and exponent scale, rescale directly to
the target scale, and add regressions for both cases?
--
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]
