Skip to content

fix: validate transform parameters in metadata#757

Open
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix/validate-transform-params
Open

fix: validate transform parameters in metadata#757
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix/validate-transform-params

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • add Transform::Validate to reuse bind-time transform checks during metadata validation
  • reject invalid bucket/truncate parameters when parsing transform strings
  • make partition specs and sort orders reject non-positive bucket/truncate parameters during schema-bound validation

Validation

  • cmake -S . -B build-transform-params -G Ninja -DICEBERG_BUILD_BUNDLE=OFF -DICEBERG_BUILD_REST=OFF -DICEBERG_BUILD_SHARED=OFF
  • ctest --test-dir build-transform-params -R schema_test --output-on-failure

@huan233usc huan233usc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Correct and well-targeted: metadata validation now reaches the bind-time bucket/truncate parameter checks via ValidateBindMake, and the new tests genuinely exercise the parameter path (source types are valid for their transforms). Two minor, non-blocking nits inline.

Comment thread src/iceberg/transform.cc
return InvalidArgument("Invalid source type {} for transform {}",
source_type->ToString(), ToString());
}
ICEBERG_RETURN_UNEXPECTED(Bind(source_type));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a one-line comment on why both CanTransform and Bind are called here: CanTransform keeps type mismatches as InvalidArgument/"Invalid source type", whereas BindMake would report them as NotSupported. Otherwise a future cleanup may drop CanTransform as redundant and silently change the error kind.

Comment thread src/iceberg/transform.cc Outdated
StringUtils::ParseNumber<int32_t>(match[2].str()));

if (type_str == kBucketName) {
if (param <= 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fallintoplace fallintoplace force-pushed the fix/validate-transform-params branch from 45d8d9d to beb7824 Compare June 30, 2026 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants