osipovartem commented on PR #11177: URL: https://github.com/apache/arrow-rs/pull/11177#issuecomment-5791792402
Independent read-only review completed: **APPROVED** (no blocking correctness, regression, performance, API-compatibility, or test findings). The review follow-up is included in `d6db05a73`: - `TopLevelRowSink` now forwards `try_append_value`, so fallible validation is preserved through the top-level wrapper. - duplicate-key coverage includes escaped-equivalent and nested keys; - an overwritten non-finite value remains rejected, confirming whole-input validation; - Criterion now includes descending-key and duplicate-key objects. Performance-first decision: the direct streaming parser remains unchanged. A buffered sort/compact approach was measured and rejected after causing material parsing regressions. Focused measurements for the new cases: | Input | Direct | Value tree | Result | |---|---:|---:|---:| | Descending keys | 1.403 us | 1.621 us | direct ~13% faster | | Duplicate keys | 795 ns | 678 ns | direct ~17% slower | The duplicate-key case is an adversarial input where the direct parser observes every member while the Value-tree path collapses duplicates during deserialization. The common-path benchmark set remains at parity or faster, and no extra buffering, sorting, or per-row materialization was introduced. Validation: - `cargo +1.95.0 test -p parquet-variant-json` (76 unit tests + 6 doctests) - focused `parquet-variant-compute` unshred tests (11 passed) - `cargo +1.95.0 clippy -p parquet-variant-json --all-targets -- -D warnings` -- 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]
