Skip surrogates when truncating string upper bounds - #3880
Open
m0g3r wants to merge 1 commit into
Open
Conversation
truncate_upper_bound_text_string increments the last character of a truncated value to build an upper bound. When that character is U+D7FF the increment lands on U+D800, a lone surrogate. Surrogates are not Unicode scalar values and cannot be encoded as UTF-8, so serializing the bound raises UnicodeEncodeError and the write fails. Skip the surrogate range when incrementing, so the successor of U+D7FF is U+E000. The result stays a valid upper bound and is encodable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
truncate_upper_bound_text_string()(pyiceberg/utils/truncate.py) builds a truncated upper boundby incrementing the last character of the truncated value. The increment is unconditional:
The comment covers only the
chr()overflow case. It misses the surrogate range: when thecharacter being incremented is
U+D7FF, the successor isU+D800, a lone surrogate.chr()accepts it without raising, so an unencodable string is returned and the failure surfaces later,
at serialization:
Surrogates are not Unicode scalar values and have no UTF-8 encoding, so the bound cannot be
written.
This is reachable from ordinary writes.
StatsAggregator.max_as_bytes()calls this helper andthen serializes the result (
pyiceberg/io/pyarrow.py), and it runs under the default metrics mode(
DEFAULT_TRUNCATION_LENGTH = 16). Any string value longer than the truncation length whosecharacter at the truncation boundary is
U+D7FFaborts the write. Both stats call sites areaffected — the Parquet writer's
close()on the append/overwrite path, andparquet_file_to_data_file()used byadd_files.The fix skips the surrogate range when incrementing, so the successor of
U+D7FFisU+E000.Since
U+E000 > U+D7FF, the result is still a valid upper bound, and it is encodable. Existingbehavior is otherwise unchanged: the
chr()overflow case that previously raisedValueErrorandfell through to the next character now returns
Nonefrom the helper and falls throughidentically.
truncate_upper_bound_binary_string()is not affected — it increments a byte guarded by< 255.Are these changes tested?
Yes.
tests/utils/test_truncate.py— two regression tests: incrementing at the surrogate boundary,and the case where the last character is at the maximum code point so the increment falls back
to an earlier character that is itself on the surrogate boundary. Both assert the result is a
genuine upper bound and encodes as UTF-8.
tests/io/test_pyarrow_stats.py::test_metrics_surrogate_boundary_upper_bound— covers thewrite path that actually breaks, computing statistics from real Parquet metadata via
data_file_statistics_from_parquet_metadata(), mirroring the existingtest_metrics_invalid_upper_boundcase.All three fail on
mainwith theUnicodeEncodeErrorabove and pass with the change; this waschecked in both directions by reverting only
pyiceberg/utils/truncate.pyand re-running(
3 failed, 19 passedreverted;22 passedwith the fix).I also swept all 1,112,064 Unicode scalar values through the helper and confirmed every returned
bound both encodes as UTF-8 and compares greater than or equal to the input value.
make lint— passes, including mypymake test— 3972 passed, 3 skippedNot run: the integration suites (
make test-integrationand the cloud-storage suites), which needDocker and credentials unavailable here.
Are there any user-facing changes?
No API change. Writes that previously failed with
UnicodeEncodeErrornow succeed. Upper boundsfor affected values change from unencodable to
U+E000-terminated; bounds for all other valuesare unchanged.
AI assistance
Claude Code was used to find the bug, write the change and the tests, and run the verification
described above.