Fix all parquet_companion code-review findings (read/indexing path + merge + FFI) - #183
Open
schenksj wants to merge 6 commits into
Open
Fix all parquet_companion code-review findings (read/indexing path + merge + FFI)#183schenksj wants to merge 6 commits into
schenksj wants to merge 6 commits into
Conversation
Addresses 15 findings from a code review of the parquet_companion indexing path (indexing, transcode, schema derivation, arrow_ffi_import, merge, hash rewriting/touchup, streaming FFI). Correctness: - hash_parquet_path: use stable xxh64 instead of std DefaultHasher, whose algorithm is unspecified across toolchains and would silently break doc->parquet resolution for existing splits. - terms-agg include/exclude on hash-redirected fields: enlarge the size / segment_size sent to tantivy and truncate the post-filtered result back to the requested size, so rare included terms are not lost from an unfiltered top-N. - transcode: error on a projected column missing from a parquet file instead of silently producing a malformed columnar (also fixes the empty-projection-reads-all-columns path). - columns_to_transcode: skip non-"raw" tokenized text fields, which a parquet raw-string fast field cannot reproduce. - doc/row pairing: RowSelection now selects each physical row once (duplicate-safe) and the TANT batch pairing maps decoded rows to all requesting indices, erroring on a true count mismatch instead of padding empty documents. - arrow FFI import: split config is a single source of truth on the context (auto-rolled, rolled, and final splits now share one identity); add_arrow_batch validates column names/types, not just count. - timestamp/Date32/Date64 conversions use checked_mul at all sites. - Decimal256: error on f64 parse failure instead of silently yielding 0.0. - merge: reject duplicate parquet relative paths across manifests. - streaming FFI: build the fallible schema export before writing the array into consumer memory, avoiding a stranded array with a live release callback on schema-conversion failure. Efficiency: - hash touchup: run the per-hash parquet reads concurrently instead of serially. - indexing hot loop: resolve column->tantivy-field mapping once per batch instead of per row. Quality: - remove dead convert_complex_to_json (duplicated convert_arrow_to_owned_value with divergent, incorrect coverage). Adds regression tests for the terms-agg size-enlargement behavior. All parquet_companion, arrow_ffi, aggregation, merge, and hash rewriter unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Addresses the high-severity findings not covered by the prior review-fix commit: the merge-integrity cluster (F1/F2/F3/M7), the writer merge policy (F5), the streaming producer panic path (F7), and name-mapping collisions (F8). Merge integrity (merge.rs, merge_impl.rs): - F2: the deletion guard read `num_deleted_docs` at the top level, but tantivy nests it under `deletes: Option<DeleteMeta>`, so the guard never fired. Read `seg.deletes.num_deleted_docs`; fix the test fixture to use tantivy's real shape. - F1: combined-manifest row offsets concatenated in input order, but quickwit's combine_index_meta puts the LAST split's docs first ([n-1, 0, 1, …, n-2]). Concatenate manifests in that order so positional consumers (transcode, legacy retrieval, hash touch-up) map doc_ids to the right parquet rows. Update the two order-sensitive tests. - M7: assert the combined row count equals the actual merged doc count as a backstop (combine_parquet_manifests now takes expected_merged_docs). - F3: stop swallowing combine errors — a source split with a manifest that fails to combine now fails the merge instead of silently producing a split with no manifest (and thus no retrievable documents). Indexing writer (indexing.rs): - F5: set NoMergePolicy and call wait_merging_threads() so background merges can't reorder docs (breaking transcode's doc_id == parquet_row assumption) or GC segment files mid-bundle. Reject multi-segment output when fast fields are parquet-served, rather than transcode wrong values. Streaming FFI (streaming_ffi.rs): - F7: wrap the producer future in catch_unwind so a producer panic becomes an Err on the channel instead of a clean EOF that silently truncates results. Schema derivation (schema_derivation.rs): - F8: validate resolved tantivy field names before building the schema — reject duplicate mapped/identity names and columns colliding with reserved companion names (__pq_*, _phash_*, *__uuids) with a clean error instead of a SchemaBuilder panic across JNI. Adds regression tests. All parquet_companion (339), schema_derivation (45), merge (11), and arrow/streaming FFI (61) unit tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Code review flagged that rejecting any column named `_phash_*` or `*__uuids` is too aggressive: those companion fields are added only conditionally (fingerprint on Hybrid Str fields; uuid tokenizer modes) and each already handles a real collision precisely at its creation site — indexing.rs disables the fingerprint for a shadowed field, and add_field_for_arrow_type bails cleanly when a companion name collides. A legitimately-named column like `session__uuids` or `_phash_temp` was being wrongly rejected. Only `__pq_file_hash` / `__pq_row_in_file` are added unconditionally with no guard, so restrict the reserved-name check to those two exact names. Update the tests to assert `_phash_*`/`*__uuids` columns are now allowed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Addresses the remaining PARQUET_COMPANION_INDEXING_REVIEW.md findings beyond the high-severity set (which two prior commits covered): the mediums, lows, efficiency, and design observations across the indexing/merge/FFI paths. Config & schema derivation: - M1: field-scoped config (tokenizer_overrides, ip_address_fields, json_fields) is now normalized once from parquet-column-name keys to tantivy-field-name keys via the name mapping, so derivation and the indexing loop share one convention (skip_fields stays parquet-keyed as it applies pre-mapping). - M2: reject compact string-indexing overrides (exact_only, text_uuid_*) on non-string columns instead of registering U64/ExactOnly against a numeric field and corrupting hash-rewrite reads. - M3: Dictionary-encoded (categorical) columns are derived by and decoded to their value type at index and retrieval time rather than dropped as Unknown. - M4: types the transcode path can't reconstruct from parquet (IP, Bytes, FixedSizeBinary, Decimal256) now always get native fast data in every mode, and the Bytes arm consults should_add_fast — fixing silently-empty IP aggregations (ParquetOnly) and hard errors (Decimal256/FixedSizeBinary, Hybrid). FixedSizeList added to the JSON type map. - M5/E7/L13: name mapping walks top-level fields (Iceberg field-ids on group nodes now resolve), builds a one-pass field_id→column index, and validates that explicit mapping keys name real columns (typos no longer silent). Merge & manifest: - M8: reject merges whose source manifests have incompatible storage_config (only split[0]'s is retained). - M9: manifest validate() now checks first-file row_offset==0, true file contiguity, and contiguous non-overlapping segment ranges covering total_rows. - L11/D3: missing meta.json during merge is a hard error; string_hash_fields mismatch is a bail!, not a release-mode-noop debug_assert. - L7: unpack_page_locations errors on a non-multiple-of-record length. Statistics: - M10: track nan_count separately (all-NaN vs empty), surfaced through ColumnStatistics; L5: reject stats requests for skipped/unknown fields; L6: no-ceiling truncation reports an unbounded max instead of a floor; E4: fewer allocations in observe_string; L1: UInt64 min/max saturates instead of wrapping negative. Value conversion & retrieval: - M12: FixedSizeList and other complex/unknown types serialize as JSON instead of an empty string; L9: guard u32 TANT offset overflow past 4 GiB; E3: share the manifest via Arc across batch fan-out instead of deep-cloning it per file. FFI ingestion: - M16: add_arrow_batch is atomic per batch (validate all rows before committing any); M17: finish_all_splits attempts all partitions and the JNI caller no longer tears down the registry entry before try_unwrap succeeds; M18: timestamps are normalized before accumulation; M19: Date32 stats use the ISO Date branch; L14/L15: dead branch removed / stale ownership comment fixed; L16: bounded partition-writer count; E6: cache the last partition-key routing. Indexing efficiency & robustness: - E1: project only indexed columns during parquet reads (skip wasted I/O on wide tables); E8: parse JSON once; L2: char-safe error previews; L3: path-boundary check in compute_relative_path; L4: require offset index on all columns; docid_mapping L8: deterministic zero-row-file resolution. Validation module: - M6: fix cache poisoning (transient storage errors no longer cached as absent); document the module is currently unwired. field_extraction M15: range/date_range/extended_stats/percentile_ranks trigger transcode; L17: dead ES-style range extraction removed. string_indexing E5: no-match strip returns input unchanged. L10: documented the intentional lossy Decimal128→f64 shared design point. All 982 native lib tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sonnet review of 79fd598 found three issues; all addressed. Critical — M4 was half-done: the index-side should_add_fast change made IP/Bytes/FixedSizeBinary/Decimal256 native, but the read/merge side (field_source, merge_columnar_bytes, extract_string/extract_bytes, record_arrow_value) still treated them as parquet-served, so the native data was written then discarded and the target cases (IP-empty in ParquetOnly, Decimal256/FixedSizeBinary hard-error in Hybrid) were not actually fixed. Reverted the should_add_fast type additions (index/read now consistent again) and fixed the failure modes on the read side instead: - record_arrow_value gained an IpAddr arm (parse string → record_ip_addr) so IP columns transcode from parquet in ParquetOnly. - extract_bytes handles FixedSizeBinary; the str-direct transcode path handles Decimal256 via value_as_string — removing the Hybrid-mode hard error. - Kept the Bytes arm consulting should_add_fast (correct: native in Disabled, parquet-served otherwise, consistent with field_source). Medium — E1 projection was dead code: build_column_mapping intentionally maps every arrow column, so the projection set always equaled the full column count and never applied. Build the projected indices from columns that are actually indexed (not in skip_fields AND present in the derived tantivy schema), so wide tables with skipped/unsupported columns are truly projected. Low — M17 ref-count guard comment overstated the guarantee: added a second strong_count check immediately before release_arc and reworded the comment to acknowledge the (now negligible) residual race honestly. All 982 native lib tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-review found the Decimal256 str-direct transcode arm used arrow's exact value_as_string() (e.g. "123.4500000000"), while native indexing (indexing.rs) and doc retrieval (doc_retrieval.rs) both format via the lossy f64 form (e.g. "123.45"): scale 0 → raw i256 string, else parsed/10^scale. The transcoded fast-field term would therefore never match the indexed term or the retrieved value for the same row — a silent inconsistency that replaced the prior hard error. Replicate the exact index-side formatting so all three paths agree. All 982 native lib tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
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.
Resolves the code-review findings for the
parquet_companionmodule acrosstwo review documents (the read/indexing-path review and
PARQUET_COMPANION_INDEXING_REVIEW.md). All high, medium, low, efficiency,and design findings are addressed except a full de-triplication refactor
(D1), whose concrete drift bugs (M12, M14) are fixed directly. Every change is
covered by the 982 passing native lib unit tests plus new regression tests.
Two rounds of Sonnet code review were run against the changes; both rounds'
findings were addressed (notably: the F8 reserved-name check was narrowed to
avoid false-positives, and the M4 fast-field fix was reworked to keep the
index and read sides consistent — see "Review corrections" below).
High-severity (8/8)
combine_index_meta(
[n-1, 0, …, n-2]), not input order.deletes.num_deleted_docs.a manifest-less split.
hash_parquet_pathuses stable xxh64, notDefaultHasher.NoMergePolicy+wait_merging_threads()andrejects multi-segment output when fast fields are parquet-served.
add_arrow_batchvalidates column names/types, not just count.__pq_*names return a cleanerror instead of panicking in
SchemaBuilder.Medium / low / efficiency / design
once); M2 reject string-mode overrides on non-string columns; M3 Dictionary
columns derived/decoded by value type; M4 IP/FixedSizeBinary/Decimal256 served
correctly from parquet (new transcode arms) instead of empty/hard-error;
M5/E7/L13 name mapping walks top-level fields, one-pass field-id index,
validates explicit keys.
validate()coversfirst-file offset, file contiguity, segment coverage; L7 page-location length
check; L11/D3 missing meta.json and hash-field mismatch are hard errors.
ColumnStatistics); L5reject stats on skipped/unknown fields; L6 unbounded-ceiling handling; L1
UInt64 saturation; E4 fewer allocations.
M13 row-count mismatch hard-errors; M14 checked date/timestamp math; L9 u32
TANT-offset overflow guard; L10 documented lossy Decimal128→f64; E3 share the
manifest via
Arcacross batch fan-out.all partitions + the JNI caller guards the registry teardown; M18 normalize
timestamps before accumulation; M19 Date32 ISO stats; L14/L15 dead branch /
stale comment; L16 bounded partition writers; E6 partition-key routing cache.
per-batch column resolution + concurrent hash touch-up; L2 char-safe error
previews; L3 path-boundary check; L4 all-column offset-index requirement;
L8 deterministic zero-row-file resolution.
range/date_range/extended_stats/percentile_ranks trigger transcode; L17 dead
ES-style range removed; E5 no-match strip returns input unchanged; D6
ES-shape range removed.
Review corrections (from the Sonnet passes)
__pq_*fields(companion
_phash_*/*__uuidscollisions are handled precisely at theircreation sites), so legitimately-named columns aren't rejected.
diverging from the read side. Instead the read side (
record_arrow_valueIpAddr arm,
extract_bytesFixedSizeBinary, str-direct Decimal256) wasfixed to match the unchanged
field_source, and the Decimal256 transcodestring form was aligned exactly with the index/retrieval formatting.
🤖 Generated with Claude Code