huan233usc commented on code in PR #793:
URL: https://github.com/apache/iceberg-cpp/pull/793#discussion_r3521222751
##########
src/iceberg/update/update_schema.cc:
##########
@@ -282,6 +289,34 @@ std::vector<SchemaField> ApplyChangesVisitor::MoveFields(
return reordered;
}
+/// \brief Cast a default value literal to a target primitive type.
+///
+/// `Literal::CastTo` only returns a value for identical decimal types, but
schema
+/// evolution permits widening a decimal to a larger precision at the SAME
scale. Such a
+/// promotion leaves the unscaled value unchanged, so the literal is rebuilt
with the
+/// target type instead of going through `CastTo`. A scale change is NOT
handled here: the
+/// unscaled value only keeps its meaning at the same scale (a decimal default
must match
+/// the column scale), so differing scales fall through to `CastTo` and are
rejected. All
+/// other conversions delegate to `CastTo`, which reports narrowing via
AboveMax/BelowMin
+/// sentinels that callers reject.
+///
+/// \pre `target_type` is non-null; every caller passes a resolved field type.
`value` is
+/// the caller's dereferenced default, so a missing default must be filtered
out before
+/// this is reached.
+Result<Literal> CastDefaultToType(const Literal& value,
+ const std::shared_ptr<PrimitiveType>&
target_type) {
+ if (value.type()->type_id() == TypeId::kDecimal &&
+ target_type->type_id() == TypeId::kDecimal) {
+ const auto& source_type = internal::checked_cast<const
DecimalType&>(*value.type());
+ const auto& decimal_type = internal::checked_cast<const
DecimalType&>(*target_type);
+ if (source_type.scale() == decimal_type.scale()) {
+ return Literal::Decimal(std::get<Decimal>(value.value()).value(),
Review Comment:
Good catch — fixed. The fast path now only fires when the literal actually
holds a `Decimal` value (`std::holds_alternative<Decimal>`), so a typed null or
sentinel falls through to `CastTo` and is rejected by the same null/sentinel
check the callers already apply. Added a regression test with
`Literal::Null(decimal(18,2))`.
##########
src/iceberg/update/update_schema.cc:
##########
@@ -282,6 +289,34 @@ std::vector<SchemaField> ApplyChangesVisitor::MoveFields(
return reordered;
}
+/// \brief Cast a default value literal to a target primitive type.
+///
+/// `Literal::CastTo` only returns a value for identical decimal types, but
schema
+/// evolution permits widening a decimal to a larger precision at the SAME
scale. Such a
+/// promotion leaves the unscaled value unchanged, so the literal is rebuilt
with the
+/// target type instead of going through `CastTo`. A scale change is NOT
handled here: the
+/// unscaled value only keeps its meaning at the same scale (a decimal default
must match
+/// the column scale), so differing scales fall through to `CastTo` and are
rejected. All
+/// other conversions delegate to `CastTo`, which reports narrowing via
AboveMax/BelowMin
+/// sentinels that callers reject.
+///
+/// \pre `target_type` is non-null; every caller passes a resolved field type.
`value` is
+/// the caller's dereferenced default, so a missing default must be filtered
out before
+/// this is reached.
+Result<Literal> CastDefaultToType(const Literal& value,
+ const std::shared_ptr<PrimitiveType>&
target_type) {
+ if (value.type()->type_id() == TypeId::kDecimal &&
+ target_type->type_id() == TypeId::kDecimal) {
+ const auto& source_type = internal::checked_cast<const
DecimalType&>(*value.type());
+ const auto& decimal_type = internal::checked_cast<const
DecimalType&>(*target_type);
+ if (source_type.scale() == decimal_type.scale()) {
+ return Literal::Decimal(std::get<Decimal>(value.value()).value(),
Review Comment:
Right — added a `FitsInPrecision(target precision)` check before relabeling,
so an out-of-range value is rejected here instead of passing `Validate` and
later failing the JSON parser. Regression test added (decimal(4,2) column with
a value needing more digits).
##########
src/iceberg/update/update_schema.cc:
##########
@@ -282,6 +289,34 @@ std::vector<SchemaField> ApplyChangesVisitor::MoveFields(
return reordered;
}
+/// \brief Cast a default value literal to a target primitive type.
+///
+/// `Literal::CastTo` only returns a value for identical decimal types, but
schema
+/// evolution permits widening a decimal to a larger precision at the SAME
scale. Such a
+/// promotion leaves the unscaled value unchanged, so the literal is rebuilt
with the
+/// target type instead of going through `CastTo`. A scale change is NOT
handled here: the
+/// unscaled value only keeps its meaning at the same scale (a decimal default
must match
+/// the column scale), so differing scales fall through to `CastTo` and are
rejected. All
+/// other conversions delegate to `CastTo`, which reports narrowing via
AboveMax/BelowMin
+/// sentinels that callers reject.
+///
+/// \pre `target_type` is non-null; every caller passes a resolved field type.
`value` is
+/// the caller's dereferenced default, so a missing default must be filtered
out before
+/// this is reached.
+Result<Literal> CastDefaultToType(const Literal& value,
+ const std::shared_ptr<PrimitiveType>&
target_type) {
+ if (value.type()->type_id() == TypeId::kDecimal &&
+ target_type->type_id() == TypeId::kDecimal) {
+ const auto& source_type = internal::checked_cast<const
DecimalType&>(*value.type());
+ const auto& decimal_type = internal::checked_cast<const
DecimalType&>(*target_type);
+ if (source_type.scale() == decimal_type.scale()) {
+ return Literal::Decimal(std::get<Decimal>(value.value()).value(),
+ decimal_type.precision(), decimal_type.scale());
+ }
+ }
+ return value.CastTo(target_type);
Review Comment:
Good point — this is a pre-existing gap in `Literal::CastTo` itself:
`CastFromInt/Long/Float/Double` have no decimal target branch (only
`CastFromString` produces a decimal), so numeric→decimal defaults were never
supported, independent of this PR. Since it means extending the shared literal
cast layer rather than the evolution path here, I would prefer to do it as a
follow-up PR to keep this one scoped to the evolution change. Filed as a
follow-up.
--
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]
