m0g3r opened a new pull request, #3880:
URL: https://github.com/apache/iceberg-python/pull/3880
# Rationale for this change
`truncate_upper_bound_text_string()` (`pyiceberg/utils/truncate.py`) builds
a truncated upper bound
by incrementing the last character of the truncated value. The increment is
unconditional:
```python
to_inc = ord(chars[i])
# will raise exception if the highest unicode code is reached
_next = chr(to_inc + 1)
```
The comment covers only the `chr()` overflow case. It misses the surrogate
range: when the
character being incremented is `U+D7FF`, the successor is `U+D800`, a lone
surrogate. `chr()`
accepts it without raising, so an unencodable string is returned and the
failure surfaces later,
at serialization:
```
UnicodeEncodeError: 'utf-8' codec can't encode character '\ud800' in
position 15: surrogates not allowed
```
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 and
then 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 whose
character at the truncation boundary is `U+D7FF` aborts the write. Both
stats call sites are
affected — the Parquet writer's `close()` on the append/overwrite path, and
`parquet_file_to_data_file()` used by `add_files`.
The fix skips the surrogate range when incrementing, so the successor of
`U+D7FF` is `U+E000`.
Since `U+E000 > U+D7FF`, the result is still a valid upper bound, and it is
encodable. Existing
behavior is otherwise unchanged: the `chr()` overflow case that previously
raised `ValueError` and
fell through to the next character now returns `None` from the helper and
falls through
identically.
`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 the
write path that actually breaks, computing statistics from real Parquet
metadata via
`data_file_statistics_from_parquet_metadata()`, mirroring the existing
`test_metrics_invalid_upper_bound` case.
All three fail on `main` with the `UnicodeEncodeError` above and pass with
the change; this was
checked in both directions by reverting only `pyiceberg/utils/truncate.py`
and re-running
(`3 failed, 19 passed` reverted; `22 passed` with 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 mypy
- `make test` — 3972 passed, 3 skipped
Not run: the integration suites (`make test-integration` and the
cloud-storage suites), which need
Docker and credentials unavailable here.
## Are there any user-facing changes?
No API change. Writes that previously failed with `UnicodeEncodeError` now
succeed. Upper bounds
for affected values change from unencodable to `U+E000`-terminated; bounds
for all other values
are unchanged.
## AI assistance
Claude Code was used to find the bug, write the change and the tests, and
run the verification
described above.
--
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]
