fix(rust, C++): fix potential issues - #58
Merged
Merged
Conversation
The writer builds a real B+tree index over i64 values for a Long column, but neither of attr_query.rs's column-type matches (nor the HTTP reader's) had a Long arm, so every such query fell through to UnsupportedColumnType. Adds the arm to all three sites and a regression test covering the seekable and sequential query paths.
…ream finding #24) Both the HTTP path (http_reader select_query_paged) and the seekable file path (reader select_query) passed the compile-time DEFAULT_NODE_SIZE to the R-tree traversal instead of the per-file header.index_node_size(), so any file written with a non-default node size was walked over the wrong node ranges. The file-path twin was undocumented — found while testing the recorded HTTP defect. select_query_seq was never affected. The mock HTTP range client now truncates a range past EOF instead of panicking, matching real server behaviour; this is what makes in-crate HTTP tests over small fixtures possible at all.
…leaf debug_assert (upstream finding #26) The leaf-level range extension (+1 node, fetched only to size the last feature of a batch) had no guard excluding that node from candidate evaluation. An oracle sweep (~950 boundary-targeted and random queries against brute force) shows the double emission the finding predicted is NOT reachable — adjacent leaf ranges always take the overlap-merge branch — so the guard is defensive: it encodes the invariant and closes the latent worst case of an unbounded RangeFrom emitted for a sizing-node hit at the end of a range. The sweep did expose a second, real defect in both query arms: debug_assert_eq!(node_pos, num_items - 1) compares a position within the fetched range against the global item count and panics in debug builds whenever the final feature is hit via a non-initial node range. Corrected to assert against the last leaf of the level. Both fixes applied to the BBox and PointIntersects arms; three oracle tests added over a mocked HTTP client.
…(upstream finding #2a) The writer stores Byte as u8 and the index path already decodes u8; the feature-value path was the one remaining site reading i8, turning a stored 200 into -56. Adds the first direct decode_attributes unit test, pinning Byte, UByte and Binary against the writer's wire format.
…xed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix). Byte/UByte join the fixed-width struct table as '<B'; Binary is a u32 LE length plus raw bytes, surfaced as a list of ints — the JSON array-of-numbers shape Rust emits, and JSON-serializable where bytes would not be.
…d Binary JSON shape The doc block claimed the Rust reader decodes Byte as i8 — fixed there this session (finding #2a), so all four implementations now agree on u8. Binary's Array.from conversion at the CityJSON boundary already existed but was untested; both new tests were validated by mutation (removing Array.from / making Byte signed each fails loudly).
…ixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix this session). Byte/UByte decode as unsigned single bytes, Binary as a u32-LE-length- prefixed byte string emitted as a JSON array of numbers (verified at runtime against nlohmann). key.cpp's mirror-image divergence comment is retired: Rust agrees on both the index and value paths now.
…ffset subtraction (upstream finding #5) Gt/Lt/Ne were lowered as 'inclusive range minus find_exact', subtracting FEATURE OFFSETS — but one feature can be indexed under several keys (its CityObjects carry different values of the indexed attribute), so the subtraction deleted features that genuinely match via another key. All three tree traversal families (in-memory, streaming, HTTP) gain strict-bound range variants sharing one in_bounds leaf predicate — the design the C++ reader proved out; the inclusive methods remain as thin wrappers. All four lowering sites (memory, stream, both http cfg copies) now do one traversal for Gt/Lt and two half-open traversals for Ne. First-ever Ne coverage; multi-key-feature expectations run through the memory, stream and http paths. Also fixes a pre-existing cursor bug the new tests exposed: StreamMultiIndex::query's empty-result early return skipped the cursor restore, so the NEXT query resolved index offsets from the wrong base.
…g Rust (#20.11, #19) Rust reads every schema-optional header string through an Option accessor: present-but-empty emits "", only genuinely absent omits the key. C++ flattened both to an empty std::string and Python to "", then gated on emptiness/truthiness — silently dropping keys the oracle keeps. C++: nine FileInfo strings become std::optional<std::string> (breaking API change, intended); has_poc_email is deleted, subsumed by poc_email.has_value(). crs (derived; #20.10 stays open) and poc_address_* (non-emptiness IS Rust's own rule there) deliberately stay plain strings. Python: FileInfo.identifier/.title become str | None with presence gates. TypeScript was already correct and is now pinned by test, mutation-verified. Each implementation pins both halves against a hand-built header, since no corpus fixture can express a present-but-empty string.
Marks #2a, #5, #17, #24 and #26 FIXED with their fix records; #24 gains the undocumented file-reader twin; #26's premise is corrected honestly (double emission shown unreachable; guard added defensively; the real defect its reproduction exposed — the wrong last-leaf debug_assert — is recorded). Item 3's stale-comment NOTE is resolved; §19 bullet 2 is struck as stale on both premises (address model is all-strings; the one remaining from_chars already matches Rust including '+'); §20.1's Byte family is closed; §20.10 (referenceSystem) is explicitly recorded as deferred pending a maintainer decision. The TS 'four deliberate divergences' notes (query.ts, key.ts, README, docs/ts.md) re-badge the Byte item as resolved — its old claim conflated Rust's index and value paths.
…d the sizing leaf (finding #26, final-review wave) The wrong debug_assert fixed in the BBox and PointIntersects arms had a third instance in PointNearest. Correcting it immediately exposed a reachable defect the review had judged latent: with no sizing-tail guard, a nearest hit landing on a range's borrowed sizing leaf (last of its fetched batch) found no next node to size against and emitted an unbounded RangeFrom — right feature, read to EOF, on ~8% of boundary queries in a 1506-query sweep. The guard now covers all three arms; brute-force nearest oracle tests added, including one forcing the level's last leaf through a non-initial range.
The Byte/UByte/Binary decode test ended with the Binary record, so a wrong u32 length read had nothing left to desynchronise. A trailing Int record now follows the payload and is reachable only if the walk resumed on the right byte; mutation-verified (length 2 -> 3 fails at the resumption point, not just the bounds check). Changelog note for the decode fix this pins (a578e69): the C++ reader previously REJECTED Byte/UByte/Binary attribute columns outright, so any file whose writer emitted them was unreadable by the C++ reader; that is un-broken — all three now decode, matching the Rust reader.
HideBa
changed the title
HideBa
added a commit
that referenced
this pull request
Aug 14, 2026
* fix(rust): query indexed Long columns (upstream finding #17) The writer builds a real B+tree index over i64 values for a Long column, but neither of attr_query.rs's column-type matches (nor the HTTP reader's) had a Long arm, so every such query fell through to UnsupportedColumnType. Adds the arm to all three sites and a regression test covering the seekable and sequential query paths. * fix(rust): honour header index_node_size on spatial query paths (upstream finding #24) Both the HTTP path (http_reader select_query_paged) and the seekable file path (reader select_query) passed the compile-time DEFAULT_NODE_SIZE to the R-tree traversal instead of the per-file header.index_node_size(), so any file written with a non-default node size was walked over the wrong node ranges. The file-path twin was undocumented — found while testing the recorded HTTP defect. select_query_seq was never affected. The mock HTTP range client now truncates a range past EOF instead of panicking, matching real server behaviour; this is what makes in-crate HTTP tests over small fixtures possible at all. * fix(rust): guard the sizing leaf in http_stream_search; fix its last-leaf debug_assert (upstream finding #26) The leaf-level range extension (+1 node, fetched only to size the last feature of a batch) had no guard excluding that node from candidate evaluation. An oracle sweep (~950 boundary-targeted and random queries against brute force) shows the double emission the finding predicted is NOT reachable — adjacent leaf ranges always take the overlap-merge branch — so the guard is defensive: it encodes the invariant and closes the latent worst case of an unbounded RangeFrom emitted for a sizing-node hit at the end of a range. The sweep did expose a second, real defect in both query arms: debug_assert_eq!(node_pos, num_items - 1) compares a position within the fetched range against the global item count and panics in debug builds whenever the final feature is hit via a non-initial node range. Corrected to assert against the last leaf of the level. Both fixes applied to the BBox and PointIntersects arms; three oracle tests added over a mocked HTTP client. * fix(rust): decode Byte feature attributes as u8, matching the writer (upstream finding #2a) The writer stores Byte as u8 and the index path already decodes u8; the feature-value path was the one remaining site reading i8, turning a stored 200 into -56. Adds the first direct decode_attributes unit test, pinning Byte, UByte and Binary against the writer's wire format. * fix(py): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix). Byte/UByte join the fixed-width struct table as '<B'; Binary is a u32 LE length plus raw bytes, surfaced as a list of ints — the JSON array-of-numbers shape Rust emits, and JSON-serializable where bytes would not be. * test(ts): close the Byte divergence notes; pin UByte/Binary decode and Binary JSON shape The doc block claimed the Rust reader decodes Byte as i8 — fixed there this session (finding #2a), so all four implementations now agree on u8. Binary's Array.from conversion at the CityJSON boundary already existed but was untested; both new tests were validated by mutation (removing Array.from / making Byte signed each fails loudly). * fix(cpp): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix this session). Byte/UByte decode as unsigned single bytes, Binary as a u32-LE-length- prefixed byte string emitted as a JSON array of numbers (verified at runtime against nlohmann). key.cpp's mirror-image divergence comment is retired: Rust agrees on both the index and value paths now. * fix(rust): evaluate Gt/Lt/Ne with strict bounds at the leaf, not by offset subtraction (upstream finding #5) Gt/Lt/Ne were lowered as 'inclusive range minus find_exact', subtracting FEATURE OFFSETS — but one feature can be indexed under several keys (its CityObjects carry different values of the indexed attribute), so the subtraction deleted features that genuinely match via another key. All three tree traversal families (in-memory, streaming, HTTP) gain strict-bound range variants sharing one in_bounds leaf predicate — the design the C++ reader proved out; the inclusive methods remain as thin wrappers. All four lowering sites (memory, stream, both http cfg copies) now do one traversal for Gt/Lt and two half-open traversals for Ne. First-ever Ne coverage; multi-key-feature expectations run through the memory, stream and http paths. Also fixes a pre-existing cursor bug the new tests exposed: StreamMultiIndex::query's empty-result early return skipped the cursor restore, so the NEXT query resolved index offsets from the wrong base. * fix(cpp,py): gate header metadata on presence, not emptiness, matching Rust (#20.11, #19) Rust reads every schema-optional header string through an Option accessor: present-but-empty emits "", only genuinely absent omits the key. C++ flattened both to an empty std::string and Python to "", then gated on emptiness/truthiness — silently dropping keys the oracle keeps. C++: nine FileInfo strings become std::optional<std::string> (breaking API change, intended); has_poc_email is deleted, subsumed by poc_email.has_value(). crs (derived; #20.10 stays open) and poc_address_* (non-emptiness IS Rust's own rule there) deliberately stay plain strings. Python: FileInfo.identifier/.title become str | None with presence gates. TypeScript was already correct and is now pinned by test, mutation-verified. Each implementation pins both halves against a hand-built header, since no corpus fixture can express a present-but-empty string. * docs: update upstream-findings statuses after the remaining-fixes pass Marks #2a, #5, #17, #24 and #26 FIXED with their fix records; #24 gains the undocumented file-reader twin; #26's premise is corrected honestly (double emission shown unreachable; guard added defensively; the real defect its reproduction exposed — the wrong last-leaf debug_assert — is recorded). Item 3's stale-comment NOTE is resolved; §19 bullet 2 is struck as stale on both premises (address model is all-strings; the one remaining from_chars already matches Rust including '+'); §20.1's Byte family is closed; §20.10 (referenceSystem) is explicitly recorded as deferred pending a maintainer decision. The TS 'four deliberate divergences' notes (query.ts, key.ts, README, docs/ts.md) re-badge the Byte item as resolved — its old claim conflated Rust's index and value paths. * fix(rust): PointNearest arm — correct last-leaf debug_assert and guard the sizing leaf (finding #26, final-review wave) The wrong debug_assert fixed in the BBox and PointIntersects arms had a third instance in PointNearest. Correcting it immediately exposed a reachable defect the review had judged latent: with no sizing-tail guard, a nearest hit landing on a range's borrowed sizing leaf (last of its fetched batch) found no next node to size against and emitted an unbounded RangeFrom — right feature, read to EOF, on ~8% of boundary queries in a 1506-query sweep. The guard now covers all three arms; brute-force nearest oracle tests added, including one forcing the level's last leaf through a non-initial range. * test(cpp): prove the attribute walk re-syncs after a Binary payload The Byte/UByte/Binary decode test ended with the Binary record, so a wrong u32 length read had nothing left to desynchronise. A trailing Int record now follows the payload and is reachable only if the walk resumed on the right byte; mutation-verified (length 2 -> 3 fails at the resumption point, not just the bounds check). Changelog note for the decode fix this pins (a578e69): the C++ reader previously REJECTED Byte/UByte/Binary attribute columns outright, so any file whose writer emitted them was unreadable by the C++ reader; that is un-broken — all three now decode, matching the Rust reader.
HideBa
added a commit
that referenced
this pull request
Aug 14, 2026
* fix(rust): query indexed Long columns (upstream finding #17) The writer builds a real B+tree index over i64 values for a Long column, but neither of attr_query.rs's column-type matches (nor the HTTP reader's) had a Long arm, so every such query fell through to UnsupportedColumnType. Adds the arm to all three sites and a regression test covering the seekable and sequential query paths. * fix(rust): honour header index_node_size on spatial query paths (upstream finding #24) Both the HTTP path (http_reader select_query_paged) and the seekable file path (reader select_query) passed the compile-time DEFAULT_NODE_SIZE to the R-tree traversal instead of the per-file header.index_node_size(), so any file written with a non-default node size was walked over the wrong node ranges. The file-path twin was undocumented — found while testing the recorded HTTP defect. select_query_seq was never affected. The mock HTTP range client now truncates a range past EOF instead of panicking, matching real server behaviour; this is what makes in-crate HTTP tests over small fixtures possible at all. * fix(rust): guard the sizing leaf in http_stream_search; fix its last-leaf debug_assert (upstream finding #26) The leaf-level range extension (+1 node, fetched only to size the last feature of a batch) had no guard excluding that node from candidate evaluation. An oracle sweep (~950 boundary-targeted and random queries against brute force) shows the double emission the finding predicted is NOT reachable — adjacent leaf ranges always take the overlap-merge branch — so the guard is defensive: it encodes the invariant and closes the latent worst case of an unbounded RangeFrom emitted for a sizing-node hit at the end of a range. The sweep did expose a second, real defect in both query arms: debug_assert_eq!(node_pos, num_items - 1) compares a position within the fetched range against the global item count and panics in debug builds whenever the final feature is hit via a non-initial node range. Corrected to assert against the last leaf of the level. Both fixes applied to the BBox and PointIntersects arms; three oracle tests added over a mocked HTTP client. * fix(rust): decode Byte feature attributes as u8, matching the writer (upstream finding #2a) The writer stores Byte as u8 and the index path already decodes u8; the feature-value path was the one remaining site reading i8, turning a stored 200 into -56. Adds the first direct decode_attributes unit test, pinning Byte, UByte and Binary against the writer's wire format. * fix(py): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix). Byte/UByte join the fixed-width struct table as '<B'; Binary is a u32 LE length plus raw bytes, surfaced as a list of ints — the JSON array-of-numbers shape Rust emits, and JSON-serializable where bytes would not be. * test(ts): close the Byte divergence notes; pin UByte/Binary decode and Binary JSON shape The doc block claimed the Rust reader decodes Byte as i8 — fixed there this session (finding #2a), so all four implementations now agree on u8. Binary's Array.from conversion at the CityJSON boundary already existed but was untested; both new tests were validated by mutation (removing Array.from / making Byte signed each fails loudly). * fix(cpp): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix this session). Byte/UByte decode as unsigned single bytes, Binary as a u32-LE-length- prefixed byte string emitted as a JSON array of numbers (verified at runtime against nlohmann). key.cpp's mirror-image divergence comment is retired: Rust agrees on both the index and value paths now. * fix(rust): evaluate Gt/Lt/Ne with strict bounds at the leaf, not by offset subtraction (upstream finding #5) Gt/Lt/Ne were lowered as 'inclusive range minus find_exact', subtracting FEATURE OFFSETS — but one feature can be indexed under several keys (its CityObjects carry different values of the indexed attribute), so the subtraction deleted features that genuinely match via another key. All three tree traversal families (in-memory, streaming, HTTP) gain strict-bound range variants sharing one in_bounds leaf predicate — the design the C++ reader proved out; the inclusive methods remain as thin wrappers. All four lowering sites (memory, stream, both http cfg copies) now do one traversal for Gt/Lt and two half-open traversals for Ne. First-ever Ne coverage; multi-key-feature expectations run through the memory, stream and http paths. Also fixes a pre-existing cursor bug the new tests exposed: StreamMultiIndex::query's empty-result early return skipped the cursor restore, so the NEXT query resolved index offsets from the wrong base. * fix(cpp,py): gate header metadata on presence, not emptiness, matching Rust (#20.11, #19) Rust reads every schema-optional header string through an Option accessor: present-but-empty emits "", only genuinely absent omits the key. C++ flattened both to an empty std::string and Python to "", then gated on emptiness/truthiness — silently dropping keys the oracle keeps. C++: nine FileInfo strings become std::optional<std::string> (breaking API change, intended); has_poc_email is deleted, subsumed by poc_email.has_value(). crs (derived; #20.10 stays open) and poc_address_* (non-emptiness IS Rust's own rule there) deliberately stay plain strings. Python: FileInfo.identifier/.title become str | None with presence gates. TypeScript was already correct and is now pinned by test, mutation-verified. Each implementation pins both halves against a hand-built header, since no corpus fixture can express a present-but-empty string. * docs: update upstream-findings statuses after the remaining-fixes pass Marks #2a, #5, #17, #24 and #26 FIXED with their fix records; #24 gains the undocumented file-reader twin; #26's premise is corrected honestly (double emission shown unreachable; guard added defensively; the real defect its reproduction exposed — the wrong last-leaf debug_assert — is recorded). Item 3's stale-comment NOTE is resolved; §19 bullet 2 is struck as stale on both premises (address model is all-strings; the one remaining from_chars already matches Rust including '+'); §20.1's Byte family is closed; §20.10 (referenceSystem) is explicitly recorded as deferred pending a maintainer decision. The TS 'four deliberate divergences' notes (query.ts, key.ts, README, docs/ts.md) re-badge the Byte item as resolved — its old claim conflated Rust's index and value paths. * fix(rust): PointNearest arm — correct last-leaf debug_assert and guard the sizing leaf (finding #26, final-review wave) The wrong debug_assert fixed in the BBox and PointIntersects arms had a third instance in PointNearest. Correcting it immediately exposed a reachable defect the review had judged latent: with no sizing-tail guard, a nearest hit landing on a range's borrowed sizing leaf (last of its fetched batch) found no next node to size against and emitted an unbounded RangeFrom — right feature, read to EOF, on ~8% of boundary queries in a 1506-query sweep. The guard now covers all three arms; brute-force nearest oracle tests added, including one forcing the level's last leaf through a non-initial range. * test(cpp): prove the attribute walk re-syncs after a Binary payload The Byte/UByte/Binary decode test ended with the Binary record, so a wrong u32 length read had nothing left to desynchronise. A trailing Int record now follows the payload and is reachable only if the walk resumed on the right byte; mutation-verified (length 2 -> 3 fails at the resumption point, not just the bounds check). Changelog note for the decode fix this pins (a578e69): the C++ reader previously REJECTED Byte/UByte/Binary attribute columns outright, so any file whose writer emitted them was unreadable by the C++ reader; that is un-broken — all three now decode, matching the Rust reader.
HideBa
added a commit
that referenced
this pull request
Aug 14, 2026
* fix(rust): query indexed Long columns (upstream finding #17) The writer builds a real B+tree index over i64 values for a Long column, but neither of attr_query.rs's column-type matches (nor the HTTP reader's) had a Long arm, so every such query fell through to UnsupportedColumnType. Adds the arm to all three sites and a regression test covering the seekable and sequential query paths. * fix(rust): honour header index_node_size on spatial query paths (upstream finding #24) Both the HTTP path (http_reader select_query_paged) and the seekable file path (reader select_query) passed the compile-time DEFAULT_NODE_SIZE to the R-tree traversal instead of the per-file header.index_node_size(), so any file written with a non-default node size was walked over the wrong node ranges. The file-path twin was undocumented — found while testing the recorded HTTP defect. select_query_seq was never affected. The mock HTTP range client now truncates a range past EOF instead of panicking, matching real server behaviour; this is what makes in-crate HTTP tests over small fixtures possible at all. * fix(rust): guard the sizing leaf in http_stream_search; fix its last-leaf debug_assert (upstream finding #26) The leaf-level range extension (+1 node, fetched only to size the last feature of a batch) had no guard excluding that node from candidate evaluation. An oracle sweep (~950 boundary-targeted and random queries against brute force) shows the double emission the finding predicted is NOT reachable — adjacent leaf ranges always take the overlap-merge branch — so the guard is defensive: it encodes the invariant and closes the latent worst case of an unbounded RangeFrom emitted for a sizing-node hit at the end of a range. The sweep did expose a second, real defect in both query arms: debug_assert_eq!(node_pos, num_items - 1) compares a position within the fetched range against the global item count and panics in debug builds whenever the final feature is hit via a non-initial node range. Corrected to assert against the last leaf of the level. Both fixes applied to the BBox and PointIntersects arms; three oracle tests added over a mocked HTTP client. * fix(rust): decode Byte feature attributes as u8, matching the writer (upstream finding #2a) The writer stores Byte as u8 and the index path already decodes u8; the feature-value path was the one remaining site reading i8, turning a stored 200 into -56. Adds the first direct decode_attributes unit test, pinning Byte, UByte and Binary against the writer's wire format. * fix(py): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix). Byte/UByte join the fixed-width struct table as '<B'; Binary is a u32 LE length plus raw bytes, surfaced as a list of ints — the JSON array-of-numbers shape Rust emits, and JSON-serializable where bytes would not be. * test(ts): close the Byte divergence notes; pin UByte/Binary decode and Binary JSON shape The doc block claimed the Rust reader decodes Byte as i8 — fixed there this session (finding #2a), so all four implementations now agree on u8. Binary's Array.from conversion at the CityJSON boundary already existed but was untested; both new tests were validated by mutation (removing Array.from / making Byte signed each fails loudly). * fix(cpp): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix this session). Byte/UByte decode as unsigned single bytes, Binary as a u32-LE-length- prefixed byte string emitted as a JSON array of numbers (verified at runtime against nlohmann). key.cpp's mirror-image divergence comment is retired: Rust agrees on both the index and value paths now. * fix(rust): evaluate Gt/Lt/Ne with strict bounds at the leaf, not by offset subtraction (upstream finding #5) Gt/Lt/Ne were lowered as 'inclusive range minus find_exact', subtracting FEATURE OFFSETS — but one feature can be indexed under several keys (its CityObjects carry different values of the indexed attribute), so the subtraction deleted features that genuinely match via another key. All three tree traversal families (in-memory, streaming, HTTP) gain strict-bound range variants sharing one in_bounds leaf predicate — the design the C++ reader proved out; the inclusive methods remain as thin wrappers. All four lowering sites (memory, stream, both http cfg copies) now do one traversal for Gt/Lt and two half-open traversals for Ne. First-ever Ne coverage; multi-key-feature expectations run through the memory, stream and http paths. Also fixes a pre-existing cursor bug the new tests exposed: StreamMultiIndex::query's empty-result early return skipped the cursor restore, so the NEXT query resolved index offsets from the wrong base. * fix(cpp,py): gate header metadata on presence, not emptiness, matching Rust (#20.11, #19) Rust reads every schema-optional header string through an Option accessor: present-but-empty emits "", only genuinely absent omits the key. C++ flattened both to an empty std::string and Python to "", then gated on emptiness/truthiness — silently dropping keys the oracle keeps. C++: nine FileInfo strings become std::optional<std::string> (breaking API change, intended); has_poc_email is deleted, subsumed by poc_email.has_value(). crs (derived; #20.10 stays open) and poc_address_* (non-emptiness IS Rust's own rule there) deliberately stay plain strings. Python: FileInfo.identifier/.title become str | None with presence gates. TypeScript was already correct and is now pinned by test, mutation-verified. Each implementation pins both halves against a hand-built header, since no corpus fixture can express a present-but-empty string. * docs: update upstream-findings statuses after the remaining-fixes pass Marks #2a, #5, #17, #24 and #26 FIXED with their fix records; #24 gains the undocumented file-reader twin; #26's premise is corrected honestly (double emission shown unreachable; guard added defensively; the real defect its reproduction exposed — the wrong last-leaf debug_assert — is recorded). Item 3's stale-comment NOTE is resolved; §19 bullet 2 is struck as stale on both premises (address model is all-strings; the one remaining from_chars already matches Rust including '+'); §20.1's Byte family is closed; §20.10 (referenceSystem) is explicitly recorded as deferred pending a maintainer decision. The TS 'four deliberate divergences' notes (query.ts, key.ts, README, docs/ts.md) re-badge the Byte item as resolved — its old claim conflated Rust's index and value paths. * fix(rust): PointNearest arm — correct last-leaf debug_assert and guard the sizing leaf (finding #26, final-review wave) The wrong debug_assert fixed in the BBox and PointIntersects arms had a third instance in PointNearest. Correcting it immediately exposed a reachable defect the review had judged latent: with no sizing-tail guard, a nearest hit landing on a range's borrowed sizing leaf (last of its fetched batch) found no next node to size against and emitted an unbounded RangeFrom — right feature, read to EOF, on ~8% of boundary queries in a 1506-query sweep. The guard now covers all three arms; brute-force nearest oracle tests added, including one forcing the level's last leaf through a non-initial range. * test(cpp): prove the attribute walk re-syncs after a Binary payload The Byte/UByte/Binary decode test ended with the Binary record, so a wrong u32 length read had nothing left to desynchronise. A trailing Int record now follows the payload and is reachable only if the walk resumed on the right byte; mutation-verified (length 2 -> 3 fails at the resumption point, not just the bounds check). Changelog note for the decode fix this pins (a578e69): the C++ reader previously REJECTED Byte/UByte/Binary attribute columns outright, so any file whose writer emitted them was unreadable by the C++ reader; that is un-broken — all three now decode, matching the Rust reader.
HideBa
added a commit
that referenced
this pull request
Aug 14, 2026
) * fix(rust, C++): fix potential issues (#58) * fix(rust): query indexed Long columns (upstream finding #17) The writer builds a real B+tree index over i64 values for a Long column, but neither of attr_query.rs's column-type matches (nor the HTTP reader's) had a Long arm, so every such query fell through to UnsupportedColumnType. Adds the arm to all three sites and a regression test covering the seekable and sequential query paths. * fix(rust): honour header index_node_size on spatial query paths (upstream finding #24) Both the HTTP path (http_reader select_query_paged) and the seekable file path (reader select_query) passed the compile-time DEFAULT_NODE_SIZE to the R-tree traversal instead of the per-file header.index_node_size(), so any file written with a non-default node size was walked over the wrong node ranges. The file-path twin was undocumented — found while testing the recorded HTTP defect. select_query_seq was never affected. The mock HTTP range client now truncates a range past EOF instead of panicking, matching real server behaviour; this is what makes in-crate HTTP tests over small fixtures possible at all. * fix(rust): guard the sizing leaf in http_stream_search; fix its last-leaf debug_assert (upstream finding #26) The leaf-level range extension (+1 node, fetched only to size the last feature of a batch) had no guard excluding that node from candidate evaluation. An oracle sweep (~950 boundary-targeted and random queries against brute force) shows the double emission the finding predicted is NOT reachable — adjacent leaf ranges always take the overlap-merge branch — so the guard is defensive: it encodes the invariant and closes the latent worst case of an unbounded RangeFrom emitted for a sizing-node hit at the end of a range. The sweep did expose a second, real defect in both query arms: debug_assert_eq!(node_pos, num_items - 1) compares a position within the fetched range against the global item count and panics in debug builds whenever the final feature is hit via a non-initial node range. Corrected to assert against the last leaf of the level. Both fixes applied to the BBox and PointIntersects arms; three oracle tests added over a mocked HTTP client. * fix(rust): decode Byte feature attributes as u8, matching the writer (upstream finding #2a) The writer stores Byte as u8 and the index path already decodes u8; the feature-value path was the one remaining site reading i8, turning a stored 200 into -56. Adds the first direct decode_attributes unit test, pinning Byte, UByte and Binary against the writer's wire format. * fix(py): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix). Byte/UByte join the fixed-width struct table as '<B'; Binary is a u32 LE length plus raw bytes, surfaced as a list of ints — the JSON array-of-numbers shape Rust emits, and JSON-serializable where bytes would not be. * test(ts): close the Byte divergence notes; pin UByte/Binary decode and Binary JSON shape The doc block claimed the Rust reader decodes Byte as i8 — fixed there this session (finding #2a), so all four implementations now agree on u8. Binary's Array.from conversion at the CityJSON boundary already existed but was untested; both new tests were validated by mutation (removing Array.from / making Byte signed each fails loudly). * fix(cpp): decode Byte/UByte/Binary feature attributes, matching the fixed Rust reader The rejection cited Rust's unreachable!() — removed long ago; Rust now decodes all three (Byte unsigned as of finding #2a's fix this session). Byte/UByte decode as unsigned single bytes, Binary as a u32-LE-length- prefixed byte string emitted as a JSON array of numbers (verified at runtime against nlohmann). key.cpp's mirror-image divergence comment is retired: Rust agrees on both the index and value paths now. * fix(rust): evaluate Gt/Lt/Ne with strict bounds at the leaf, not by offset subtraction (upstream finding #5) Gt/Lt/Ne were lowered as 'inclusive range minus find_exact', subtracting FEATURE OFFSETS — but one feature can be indexed under several keys (its CityObjects carry different values of the indexed attribute), so the subtraction deleted features that genuinely match via another key. All three tree traversal families (in-memory, streaming, HTTP) gain strict-bound range variants sharing one in_bounds leaf predicate — the design the C++ reader proved out; the inclusive methods remain as thin wrappers. All four lowering sites (memory, stream, both http cfg copies) now do one traversal for Gt/Lt and two half-open traversals for Ne. First-ever Ne coverage; multi-key-feature expectations run through the memory, stream and http paths. Also fixes a pre-existing cursor bug the new tests exposed: StreamMultiIndex::query's empty-result early return skipped the cursor restore, so the NEXT query resolved index offsets from the wrong base. * fix(cpp,py): gate header metadata on presence, not emptiness, matching Rust (#20.11, #19) Rust reads every schema-optional header string through an Option accessor: present-but-empty emits "", only genuinely absent omits the key. C++ flattened both to an empty std::string and Python to "", then gated on emptiness/truthiness — silently dropping keys the oracle keeps. C++: nine FileInfo strings become std::optional<std::string> (breaking API change, intended); has_poc_email is deleted, subsumed by poc_email.has_value(). crs (derived; #20.10 stays open) and poc_address_* (non-emptiness IS Rust's own rule there) deliberately stay plain strings. Python: FileInfo.identifier/.title become str | None with presence gates. TypeScript was already correct and is now pinned by test, mutation-verified. Each implementation pins both halves against a hand-built header, since no corpus fixture can express a present-but-empty string. * docs: update upstream-findings statuses after the remaining-fixes pass Marks #2a, #5, #17, #24 and #26 FIXED with their fix records; #24 gains the undocumented file-reader twin; #26's premise is corrected honestly (double emission shown unreachable; guard added defensively; the real defect its reproduction exposed — the wrong last-leaf debug_assert — is recorded). Item 3's stale-comment NOTE is resolved; §19 bullet 2 is struck as stale on both premises (address model is all-strings; the one remaining from_chars already matches Rust including '+'); §20.1's Byte family is closed; §20.10 (referenceSystem) is explicitly recorded as deferred pending a maintainer decision. The TS 'four deliberate divergences' notes (query.ts, key.ts, README, docs/ts.md) re-badge the Byte item as resolved — its old claim conflated Rust's index and value paths. * fix(rust): PointNearest arm — correct last-leaf debug_assert and guard the sizing leaf (finding #26, final-review wave) The wrong debug_assert fixed in the BBox and PointIntersects arms had a third instance in PointNearest. Correcting it immediately exposed a reachable defect the review had judged latent: with no sizing-tail guard, a nearest hit landing on a range's borrowed sizing leaf (last of its fetched batch) found no next node to size against and emitted an unbounded RangeFrom — right feature, read to EOF, on ~8% of boundary queries in a 1506-query sweep. The guard now covers all three arms; brute-force nearest oracle tests added, including one forcing the level's last leaf through a non-initial range. * test(cpp): prove the attribute walk re-syncs after a Binary payload The Byte/UByte/Binary decode test ended with the Binary record, so a wrong u32 length read had nothing left to desynchronise. A trailing Int record now follows the payload and is reachable only if the walk resumed on the right byte; mutation-verified (length 2 -> 3 fails at the resumption point, not just the bounds check). Changelog note for the decode fix this pins (a578e69): the C++ reader previously REJECTED Byte/UByte/Binary attribute columns outright, so any file whose writer emitted them was unreadable by the C++ reader; that is un-broken — all three now decode, matching the Rust reader. * release(cpp): 0.8.1 — public encoded-geometry API in the vcpkg port Bump the C++ project version so the installed flatcitybufConfigVersion.cmake matches the vcpkg port that ships it. Since v0.8.0 the library made Feature::raw() public (the encoded geometry: the five flat count arrays and the boundaries index list) and gained the geometry_analysis example. Also drop the square-bracket placeholder around the copyright holder in LICENSE — vcpkg installs this file verbatim as the port copyright. The tag for this release is cpp-v0.8.1, joining the npm-v*/python-v* per-language scheme: a bare v0.8.1 would collide with the tag release.yml cuts when the Rust crates reach 0.8.1, silently skipping that release (its TAG_EXISTS guard matches refs/tags/v<version>).
HideBa
added a commit
that referenced
this pull request
Aug 14, 2026
The cpp-v0.8.1 tag (579969f) was cut on the release branch before #58 landed, so the shipped 0.8.1 port predates every C++ fix in it. This bump gives the vcpkg port a tag that carries them: - Byte/UByte/Binary feature attributes decode as the writer stores them (Byte unsigned; Binary a u32-LE-length-prefixed byte string emitted as a JSON array of numbers). - Nine FileInfo strings became std::optional<std::string> and has_poc_email was deleted, so header metadata is gated on presence rather than emptiness, matching Rust. Minor, not patch: the FileInfo change breaks the API for anyone resolving flatcitybuf through the registry.
HideBa
added a commit
to HideBa/vcpkg
that referenced
this pull request
Aug 14, 2026
Minor, not patch: cityjson/flatcitybuf#58 turned nine FileInfo strings into std::optional<std::string> and deleted has_poc_email, so header metadata is gated on presence rather than emptiness. It also fixed Byte/UByte/Binary feature-attribute decoding. The cpp-v0.8.1 tag was cut on a release branch before microsoft#58 landed, so the 0.8.1 port predates every one of those fixes. relax-flatbuffers-version-check.patch still applies unchanged; the release build installs a ConfigVersion of 0.9.0, matching this port.
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
1 participant
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.
No description provided.