Skip to content

fix(spec): reject a non-positive or non-integer bucket number - #956

Open
jackylee-ch wants to merge 2 commits into
apache:mainfrom
jackylee-ch:fix/validate-bucket-count
Open

jackylee-ch wants to merge 2 commits into
apache:mainfrom
jackylee-ch:fix/validate-bucket-count

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

CoreOptions::bucket() parses the bucket option with unwrap_or(DEFAULT_BUCKET), so a bad value never surfaced: bucket=0 was taken verbatim (silently turning off read-side bucket pruning), and a non-integer like bucket=abc silently fell back to dynamic bucketing (-1) instead of the fixed bucketing the user intended.

This validates the option at create/alter time, mirroring Java SchemaValidation.validateBucket: -1 (dynamic), -2 (postpone) and any value >= 1 are allowed; 0, values <= -3, and non-integers are rejected with a clear error. (The Java bucket=-1 + bucket-key arm is left to the existing bucket-key validation.)

Tests cover the 0 / non-integer rejections and the accepted -1 / -2 / >=1 values.

@jackylee-ch
jackylee-ch force-pushed the fix/validate-bucket-count branch from 543fa8a to 721a86e Compare September 30, 2026 14:39
@JingsongLi

Copy link
Copy Markdown
Contributor

Production review found one P2 validation gap in Schema::validate_bucket_count (crates/paimon/src/spec/schema.rs, around line 1244).

The new validator trims the value before parsing, but CoreOptions::bucket() parses the original persisted string without trimming and falls back to -1 on a parse failure. Consequently, bucket=" 4 " passes create/alter validation but selects dynamic bucketing at runtime. This leaves the silent fallback that this PR aims to prevent reachable through the newly accepted input grammar. The runtime fallback already existed; the issue here is the incomplete validation contract.

I reproduced this through a native primary-key table: create with bucket=" 4 ", write four rows, prepare/commit and read them back. The persisted manifest entries have total_buckets = -1; the same flow with bucket="4" records 4. The regression assertion fails on this head. A temporary control removing the validator's .trim() passes, rejecting the padded value before creating the table and preserving the valid fixed-bucket write. Either use the existing runtime/Java integer grammar for validation or normalize the value consistently before persisting and consuming it.

Validation: all 128 existing schema/schema-change tests passed; the additional real write/commit/read probe reproduced the mismatch. Please add coverage that checks the resulting bucket mode, including the alter-options path.

`CoreOptions::bucket()` parses the option with `unwrap_or(DEFAULT_BUCKET)`, so
`bucket=0` was taken verbatim (silently disabling read-side bucket pruning) and
a non-integer such as `bucket=abc` silently fell back to dynamic (-1) instead of
the fixed bucketing the user asked for.

Validate the option at create/alter time, mirroring Java `validateBucket`: allow
-1 (dynamic), -2 (postpone) and any value >= 1; reject 0, values <= -3, and
non-integers with a clear error.
`validate_bucket_count` trimmed the value before parsing, but
`CoreOptions::bucket()` parses the raw persisted string without trimming and
falls back to -1 on failure. So `bucket=' 4 '` passed create/alter validation
yet selected dynamic bucketing at runtime — the exact silent fallback this
validator aims to prevent, reachable through the newly accepted padded input.

Parse the raw value the same way `bucket()` does (no trim), so a value the
runtime cannot parse is rejected up front. Added coverage that a padded value
is rejected while the runtime reads it as -1, and that the accepted spelling
runs as the requested fixed count.
@jackylee-ch
jackylee-ch force-pushed the fix/validate-bucket-count branch from 721a86e to dc58f19 Compare October 2, 2026 01:10
@jackylee-ch

Copy link
Copy Markdown
Contributor Author

Addressed.

validate_bucket_count now parses the raw value exactly as CoreOptions::bucket() does — without trimming — so a padded bucket=' 4 ' is rejected up front instead of passing validation and then running as dynamic (-1) at runtime. The validator and the runtime now share one integer grammar.

This runs on both create and alter: validate_bucket_count is called from validate_final_schema, which create and schema-change share (per its own comment), so the padded value is rejected on the alter-options path too.

Regression bucket_padded_value_is_rejected_matching_runtime pins the resulting mode: it asserts CoreOptions::bucket() reads ' 4 ' as -1 (the silent fallback), that ' 4 ' is now rejected at build with the integer error, and that the accepted '4' runs as the fixed count 4. I verified non-vacuity: restoring the validator's .trim() makes the padded value pass validation and the test fail; parsing the raw value passes.

Rebased onto current main. The schema bucket_* tests pass; clippy -p paimon --all-targets -D warnings is clean.

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