huan233usc commented on code in PR #757:
URL: https://github.com/apache/iceberg-cpp/pull/757#discussion_r3501409924
##########
src/iceberg/transform.cc:
##########
@@ -232,6 +232,18 @@ bool Transform::CanTransform(const Type& source_type)
const {
std::unreachable();
}
+Status Transform::Validate(const std::shared_ptr<Type>& source_type) const {
+ if (!source_type) {
+ return InvalidArgument("Source type cannot be null for transform {}",
ToString());
+ }
+ if (!CanTransform(*source_type)) {
+ return InvalidArgument("Invalid source type {} for transform {}",
+ source_type->ToString(), ToString());
+ }
+ ICEBERG_RETURN_UNEXPECTED(Bind(source_type));
Review Comment:
Worth a one-line comment on why both `CanTransform` and `Bind` are called
here: `CanTransform` keeps type mismatches as `InvalidArgument`/"Invalid source
type", whereas `Bind`→`Make` would report them as `NotSupported`. Otherwise a
future cleanup may drop `CanTransform` as redundant and silently change the
error kind.
##########
src/iceberg/transform.cc:
##########
@@ -554,9 +566,15 @@ Result<std::shared_ptr<Transform>>
TransformFromString(std::string_view transfor
StringUtils::ParseNumber<int32_t>(match[2].str()));
if (type_str == kBucketName) {
+ if (param <= 0) {
Review Comment:
The bucket/truncate positivity rule (and its message) is now duplicated here
and in `BucketTransform::Make`/`TruncateTransform::Make`. Fine for fail-fast on
unbound transforms, but easy to drift — consider a shared helper if it grows.
--
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]
