Type capture label keys and values as Dictionary(Int32,Utf8) - #1927
Draft
preinlein wants to merge 6 commits into
Draft
Type capture label keys and values as Dictionary(Int32,Utf8)#1927preinlein wants to merge 6 commits into
preinlein wants to merge 6 commits into
Conversation
Labels dominate capture parquet memory, and their key/value strings repeat heavily across rows. Typing the labels map key/value as Dictionary(Int32,Utf8) instead of Utf8 lets readers materialize each distinct string once and reference it by index, cutting resident memory of downstream capture readers rather than only the on-disk size that parquet page dictionary encoding already handled. - Add lading_capture_schema::label_dictionary_type() as the single source of the label logical type; use it in the schema and the parquet writer. - Build label key/value arrays with StringDictionaryBuilder<Int32Type> in the writer, and read them as dictionary arrays in validate and the roundtrip test. Breaking: the old (Utf8) and new (dictionary) capture schemas are not interchangeable; consumers must read the label columns as dictionary arrays.
Address review feedback on the label Dictionary(Int32,Utf8) change: - Extract resolve_label_dictionary helper in formats::parquet, used by both the validate path and the roundtrip test, removing the duplicated DictionaryArray -> StringArray downcast blocks. - Add parquet_round_trip_repeated_labels, a deterministic test roundtripping many rows that share identical label strings so the dictionary-dedup path is exercised under the production repetition pattern. - Soften the label_dictionary_type doc comment: drop the unsubstantiated 92% figure and reader-specific claim.
The `Dictionary(Int32, Utf8)` label typing broke a second in-repo reader that the first pass missed: `captool analyze` decoded the label `key`/`value` columns with `downcast_ref::<StringArray>()`, which returns `None` once those columns are `DictionaryArray<Int32Type>`, so `analyze_metric` failed on every capture written by the new writer. It compiles (auto-detected bin target), so there was no compile-time signal, and nothing exercised the path. - Make `resolve_label_dictionary` `pub` so `captool` (in the `lading` crate) uses the same canonical decode path as the `lading-capture` reader/validator instead of re-rolling the two-step downcast. - Route `captool analyze` label decoding through `resolve_label_dictionary`. - Add a `captool` test that writes a capture with the production `Format` writer and runs `list_metrics`/`analyze_metric` over it, so any future divergence between the writer's on-disk schema and the analyzer fails in CI. - Add a `lading-capture` roundtrip test that flushes between chunks, forcing shared label strings into independent per-batch dictionaries. - Add a `validate_parquet` test covering its own dictionary-decode and per-series label-reconstruction path. - Fix doc/import/casing nits from review.
Addresses a second review pass over the dictionary-label changes: - Rewrite the `formats` multi-batch test. The old `parquet_round_trip_multiple_batches` documented that flushing between chunks forces separate per-batch dictionaries, but `ArrowWriter` coalesces per-`write()` batches into one row group, so three flushes of four rows produced a single read batch — the doc claim was false and the test never exercised per-batch decode. `parquet_round_trip_multiple_read_batches` reads with `with_batch_size(2)` to genuinely emit several batches, decodes each batch's `Dictionary(Int32, Utf8)` labels via `resolve_label_dictionary`, and asserts `batch_count > 1` so the coverage claim cannot silently lapse. - Assert `mean` and `is_monotonic` on the staging series in the captool `analyze_metric` test, matching the prod series assertions. - Remove a redundant dictionary-label validate test; `validate.rs` already covers the `validate_parquet` decode path across happy and error cases.
The `parquet_round_trip_multiple_read_batches` doc claimed the test defends against a reader mishandling per-batch dictionaries (stale indices, assuming one global dictionary). It cannot: the writer emits a single row group with one dictionary page, so every read batch shares that dictionary — there are no divergent per-batch dictionaries to reconcile, and the test data (`prod`/`staging` alternating) yields an identical mapping in every batch regardless. Reword the doc to state what the test actually proves — that a file read as more than one `RecordBatch` decodes every label correctly through `resolve_label_dictionary` — and note the single-dictionary layout explicitly. Doc comment only; no behavior change.
Trim the verbose doc and inline comments introduced with the dictionary-label work down to what each actually needs to say. No code changes. Also drop a stale "independent dictionary" inline comment in the multi-batch test that contradicted the corrected doc (the file has one shared dictionary page).
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.
What does this PR do?
Types the capture parquet
labelsmapkeyandvaluecolumns asDictionary(Int32, Utf8)instead ofUtf8.lading_capture_schema::label_dictionary_type()as the single source of the label logical type, used by both the schema and the parquet writer.StringDictionaryBuilder<Int32Type>in the writer, and reads them back as dictionary arrays invalidateand the roundtrip property test.Motivation
Dictionary(Int32, Utf8)lets downstream readers materialize each distinct string once and reference it by index, cutting resident memory rather than only the on-disk size.Additional Notes
This is a breaking change. The old (
Utf8) and new (dictionary) capture schemas are not interchangeable; downstream consumers must read the label columns as dictionary arrays. It is intended to land as a coordinated cutover with the downstream reader change.