Skip to content

Commit 53ab059

Browse files
committed
Tighten comments added on this branch
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).
1 parent 2b76270 commit 53ab059

4 files changed

Lines changed: 28 additions & 40 deletions

File tree

lading/src/bin/captool/analyze/parquet.rs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -258,13 +258,11 @@ mod tests {
258258
tmp
259259
}
260260

261-
/// `analyze_metric` reads captures written by the production parquet writer.
261+
/// `analyze_metric` reads captures written by the production [`Format`] writer.
262262
///
263-
/// This binds `captool analyze` to the capture writer: the file is produced
264-
/// by `lading_capture`'s [`Format`], so any change to the on-disk label
265-
/// encoding (e.g. the `Dictionary(Int32, Utf8)` label columns) that the
266-
/// analyzer fails to track breaks this test in CI rather than shipping a
267-
/// silently broken tool.
263+
/// Binds `captool analyze` to the writer's on-disk schema: a label-encoding
264+
/// change the analyzer fails to track breaks this test rather than shipping
265+
/// silently broken.
268266
#[test]
269267
#[expect(
270268
clippy::float_cmp,

lading_capture/src/formats.rs

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -400,13 +400,11 @@ mod tests {
400400
}
401401
}
402402

403-
/// Roundtrip many rows that share identical label key/value strings.
403+
/// Roundtrip many rows sharing identical label strings.
404404
///
405-
/// The property test draws random labels, so cross-row repetition is
406-
/// incidental. This locks in that the `Dictionary(Int32, Utf8)` label columns
407-
/// roundtrip correctly under the production pattern where a handful of
408-
/// distinct strings (e.g. `env=prod`) repeat across every row and are
409-
/// dictionary-deduplicated by the writer.
405+
/// The property test's random labels only repeat by chance; this pins the
406+
/// production pattern where a few distinct strings (`env=prod`) repeat across
407+
/// every row and are dictionary-deduplicated.
410408
#[test]
411409
fn parquet_round_trip_repeated_labels() {
412410
let run_id = Uuid::from_u128(0x1234_5678_9abc_def0_1234_5678_9abc_def0);
@@ -417,8 +415,8 @@ mod tests {
417415
.into_iter()
418416
.collect();
419417

420-
// 500 rows all carrying the same two labels, plus one row with a distinct
421-
// label so the dictionary holds more than a single entry.
418+
// 500 rows sharing two labels, plus one distinct row so the dictionary
419+
// holds more than one entry.
422420
let mut input_lines: Vec<Line> = (0u64..500)
423421
.map(|i| Line {
424422
run_id,
@@ -468,17 +466,14 @@ mod tests {
468466
/// Decode `Dictionary(Int32, Utf8)` labels when a file is read as several
469467
/// `RecordBatch`es.
470468
///
471-
/// The parquet reader emits one `RecordBatch` per `batch_size` rows, so
472-
/// production files — which exceed the default batch size — are always read as
473-
/// several batches, a path a small single-batch roundtrip never reaches. Here a
474-
/// small `batch_size` forces multiple batches, and each batch's labels are
475-
/// decoded through the production `resolve_label_dictionary` helper and checked
476-
/// against the values written. The test asserts it actually produced more than
477-
/// one batch so the multi-batch coverage cannot silently lapse.
469+
/// The reader emits one `RecordBatch` per `batch_size` rows, so production
470+
/// files are read as several batches — a path a single-batch roundtrip misses.
471+
/// A small `batch_size` forces that here; each batch's labels are decoded via
472+
/// the production `resolve_label_dictionary` and checked, and the test asserts
473+
/// more than one batch so the coverage cannot silently lapse.
478474
///
479-
/// This does not exercise divergent per-batch dictionaries: the writer emits a
480-
/// single row group with one dictionary page, so every read batch shares that
481-
/// one dictionary rather than decoding an independent, index-restarted one.
475+
/// It does not exercise divergent per-batch dictionaries: the writer emits one
476+
/// row group with a single dictionary page, shared by every batch.
482477
#[test]
483478
fn parquet_round_trip_multiple_read_batches() {
484479
let run_id = Uuid::from_u128(0x0fed_cba9_8765_4321_0fed_cba9_8765_4321);
@@ -512,7 +507,7 @@ mod tests {
512507
let bytes = buffer.into_inner();
513508

514509
// A batch size below the row count forces the reader to emit several
515-
// batches, each decoding its labels from an independent dictionary.
510+
// batches.
516511
let bytes_buf = Bytes::copy_from_slice(&bytes);
517512
let reader = ParquetRecordBatchReaderBuilder::try_new(bytes_buf)
518513
.expect("reader builder")

lading_capture/src/formats/parquet.rs

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,17 +29,14 @@ use crate::line;
2929
/// Resolve a label map column typed as [`label_dictionary_type`] to a typed
3030
/// dictionary view of its `Utf8` values.
3131
///
32-
/// Both the label `key` and `value` columns are written as
33-
/// `Dictionary(Int32, Utf8)`, so every reader resolves them the same way. This
34-
/// is the single canonical decode path: readers in other crates (e.g. `captool`)
35-
/// call it too, so a schema change here surfaces in every reader at once. On
36-
/// failure returns a message prefixed with `column_name` (e.g. `"Labels keys"`)
37-
/// so callers can surface distinct errors for the key and value columns.
32+
/// The single decode path for both the `key` and `value` columns, shared across
33+
/// crates (e.g. `captool`) so a schema change surfaces everywhere at once.
34+
/// `column_name` prefixes the error so callers can tell the two columns apart.
3835
///
3936
/// # Errors
4037
///
41-
/// Returns an error message if `column` is not a `Dictionary(Int32, _)` or its
42-
/// dictionary values are not `Utf8`.
38+
/// Errors if `column` is not a `Dictionary(Int32, _)` or its values are not
39+
/// `Utf8`.
4340
pub fn resolve_label_dictionary<'a>(
4441
column: &'a dyn Array,
4542
column_name: &str,
@@ -237,8 +234,7 @@ impl<W: Write + Seek + Send> Format<W> {
237234
label_offsets.push(0i32);
238235
label_offsets.extend_from_slice(&self.buffers.label_offsets);
239236

240-
// Build the labels map array using pre-allocated buffers. Keys and
241-
// values are dictionary-encoded (Int32,Utf8) so downstream readers
237+
// Keys and values are dictionary-encoded (Int32,Utf8) so readers
242238
// materialize each distinct string once; labels dominate capture memory.
243239
let mut keys_builder = StringDictionaryBuilder::<Int32Type>::new();
244240
for key in &self.buffers.label_keys {

lading_capture_schema/src/lib.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,10 @@ fn labels_entry_field() -> Arc<Field> {
7777

7878
/// Arrow logical type for label keys and values.
7979
///
80-
/// Labels are a large portion of the memory used by capture consumers, and their
81-
/// key/value strings repeat heavily across rows. Typing them as
82-
/// `Dictionary(Int32, Utf8)` lets readers materialize each distinct string once
83-
/// and reference it by index, cutting resident memory rather than merely the
84-
/// on-disk size (which parquet page dictionary encoding already handled).
80+
/// Label strings repeat heavily across rows and dominate capture memory.
81+
/// `Dictionary(Int32, Utf8)` lets readers hold each distinct string once,
82+
/// cutting resident memory (not just on-disk size, which parquet already
83+
/// dictionary-encoded).
8584
#[must_use]
8685
pub fn label_dictionary_type() -> DataType {
8786
DataType::Dictionary(Box::new(DataType::Int32), Box::new(DataType::Utf8))

0 commit comments

Comments
 (0)