Skip to content

feat: enforce physical column ordering in Parquet files#6287

Merged
g-talbot merged 80 commits intomainfrom
gtt/parquet-column-ordering-v2
Apr 13, 2026
Merged

feat: enforce physical column ordering in Parquet files#6287
g-talbot merged 80 commits intomainfrom
gtt/parquet-column-ordering-v2

Conversation

@g-talbot
Copy link
Copy Markdown
Contributor

@g-talbot g-talbot commented Apr 10, 2026

Summary

  • Sort schema columns are placed first (in their configured sort order), followed by remaining data columns alphabetically
  • This layout enables a two-GET streaming merge during compaction: the first GET reads the footer, the second streams from the start of the row group — sort columns arrive first, allowing the compactor to compute the global merge order before data columns arrive
  • Clean cherry-pick of the column-ordering work from feat: enforce physical column ordering in Parquet files for two-GET streaming merge #6281 (which was accidentally merged into the docs branch instead of main)

Test plan

  • reorder_columns unit test verifies sort columns first, then alphabetical
  • Round-trip test verifies column order preserved through Parquet write/read
  • All 154 quickwit-parquet-engine tests pass
  • Clippy clean

🤖 Generated with Claude Code

mattmkim and others added 30 commits March 30, 2026 14:21
@mattmkim
feat: replace fixed MetricDataPoint fields with dynamic tag HashMap
d892f23
@mattmkim
feat: replace ParquetField enum with constants and dynamic validation
4e928fe
@mattmkim
feat: derive sort order and bloom filters from batch schema
d8d71ed
@mattmkim
feat: union schema accumulation and schema-agnostic ingest validation
ad115bf
@mattmkim
feat: dynamic column lookup in split writer
d0a995e
@mattmkim
feat: remove ParquetSchema dependency from indexing actors
35c3942
@mattmkim
refactor: deduplicate test batch helpers
33c4070
@mattmkim
lint
7a5979f
@mattmkim
Merge branch 'main' into matthew.kim/metrics-wide-schema
440631b
@mattmkim
Merge branch 'main' into matthew.kim/metrics-wide-schema
5eae799
@g-talbot @claude
feat(31): sort schema foundation — proto, parser, display, validation…
5b2304c 
…, window, TableConfig

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot
fix: rustdoc link errors — use backticks for private items
4d42fd9
@g-talbot @claude
feat(31): compaction metadata types — extend split metadata, postgres…
b6eb595 
… model, field lookup

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
feat(31): wire TableConfig into sort path, add compaction KV metadata
76b703a 
Wire TableConfig-driven sort order into ParquetWriter and add
self-describing Parquet file metadata for compaction:

- ParquetWriter::new() takes &TableConfig, resolves sort fields at
  construction via parse_sort_fields() + ParquetField::from_name()
- sort_batch() uses resolved fields with per-column direction (ASC/DESC)
- SS-1 debug_assert verification: re-sort and check identity permutation
- build_compaction_key_value_metadata(): embeds sort_fields, window_start,
  window_duration, num_merge_ops, row_keys (base64) in Parquet kv_metadata
- SS-5 verify_ss5_kv_consistency(): kv_metadata matches source struct
- write_to_file_with_metadata() replaces write_to_file()
- prepare_write() shared method for bytes and file paths
- ParquetWriterConfig gains to_writer_properties_with_metadata()
- ParquetSplitWriter passes TableConfig through
- All callers in quickwit-indexing updated with TableConfig::default()
- 23 storage tests pass including META-07 self-describing roundtrip

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
feat(31): PostgreSQL migration 27 + compaction columns in stage/list/…
ff605b9 
…publish

Add compaction metadata to the PostgreSQL metastore:

Migration 27:
- 6 new columns: window_start, window_duration_secs, sort_fields,
  num_merge_ops, row_keys, zonemap_regexes
- Partial index idx_metrics_splits_compaction_scope on
  (index_uid, sort_fields, window_start) WHERE split_state = 'Published'

stage_metrics_splits:
- INSERT extended from 15 to 21 bind parameters for compaction columns
- ON CONFLICT SET updates all compaction columns

list_metrics_splits:
- PgMetricsSplit construction includes compaction fields (defaults from JSON)

Also fixes pre-existing compilation errors on upstream-10b-parquet-actors:
- Missing StageMetricsSplitsRequestExt import
- index_id vs index_uid type mismatches in publish/mark/delete
- IndexUid binding (to_string() for sqlx)
- ListMetricsSplitsResponseExt trait disambiguation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
fix(31): close port gaps — split_writer metadata, compaction scope, p…
723168f 
…ublish validation

Close critical gaps identified during port review:

split_writer.rs:
- Store table_config on ParquetSplitWriter (not just pass-through)
- Compute window_start from batch time range using table_config.window_duration_secs
- Populate sort_fields, window_duration_secs, parquet_files on metadata before write
- Call write_to_file_with_metadata(Some(&metadata)) to embed KV metadata in Parquet
- Update size_bytes after write completes

metastore/mod.rs:
- Add window_start and sort_fields fields to ListMetricsSplitsQuery
- Add with_compaction_scope() builder method

metastore/postgres/metastore.rs:
- Add compaction scope filters (AND window_start = $N, AND sort_fields = $N) to list query
- Add replaced_split_ids count verification in publish_metrics_splits
- Bind compaction scope query parameters

ingest/config.rs:
- Add table_config: TableConfig field to ParquetIngestConfig

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
fix(31): final gap fixes — file-backed scope filter, META-07 test, de…
9ca263d 
…ad code removal

- file_backed_index/mod.rs: Add window_start and sort_fields filtering
  to metrics_split_matches_query() for compaction scope queries
- writer.rs: Add test_meta07_self_describing_parquet_roundtrip test
  (writes compaction metadata to Parquet, reads back from cold file,
  verifies all fields roundtrip correctly)
- fields.rs: Remove dead sort_order() method (replaced by TableConfig)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
fix(31): correct postgres types for window_duration_secs and zonemap_…
73a20ef 
…regexes

Gap 1: Change window_duration_secs from i32 to Option<i32> in both
PgMetricsSplit and InsertableMetricsSplit. Pre-Phase-31 splits now
correctly map 0 → NULL in PostgreSQL, enabling Phase 32 compaction
queries to use `WHERE window_duration_secs IS NOT NULL` instead of
the fragile `WHERE window_duration_secs > 0`.

Gap 2: Change zonemap_regexes from String to serde_json::Value in
both structs. This maps directly to JSONB in sqlx, avoiding ambiguity
when PostgreSQL JSONB operators are used in Phase 34/35 zonemap pruning.

Gap 3: Add two missing tests:
- test_insertable_from_metadata_with_compaction_fields: verifies all 6
  compaction fields round-trip through InsertableMetricsSplit
- test_insertable_from_metadata_pre_phase31_defaults: verifies pre-Phase-31
  metadata produces window_duration_secs: None, zonemap_regexes: json!({})

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
style: rustfmt
75c15a0 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
test(31): add metrics split test suite to shared metastore_test_suite…
ef21859 
…! macro

11 tests covering the full metrics split lifecycle:
- stage (happy path + non-existent index error)
- stage upsert (ON CONFLICT update)
- list by state, time range, metric name, compaction scope
- publish (happy path + non-existent split error)
- mark for deletion
- delete (happy path + idempotent non-existent)

Tests are generic and run against both file-backed and PostgreSQL backends.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot
fix(31): read compaction columns in list_metrics_splits, fix cleanup_…
b4dac46 
…index FK
@g-talbot @claude
fix(31): correct error types for non-existent metrics splits
db51a96 
- publish_metrics_splits: return NotFound (not FailedPrecondition) when
  staged splits don't exist
- delete_metrics_splits: succeed silently (idempotent) for non-existent
  splits instead of returning FailedPrecondition
- Tests now assert the correct error types on both backends

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot
style: rustfmt metastore tests and postgres
605708e
@g-talbot @claude
fix(31): address PR review — align metrics_splits with splits table
f21fff5 
- Migration 27: add maturity_timestamp, delete_opstamp, node_id columns
  and publish_timestamp trigger to match the splits table (Paul's review)
- ListMetricsSplitsQuery: adopt FilterRange<i64> for time_range (matching
  log-side pattern), single time_range field for both read and compaction
  paths, add node_id/delete_opstamp/update_timestamp/create_timestamp/
  mature filters to close gaps with ListSplitsQuery
- Use SplitState enum instead of stringly-typed Vec<String> for split_states
- StoredMetricsSplit: add create_timestamp, node_id, delete_opstamp,
  maturity_timestamp so file-backed metastore can filter on them locally
- File-backed filter: use FilterRange::overlaps_with() for time range and
  window intersection, apply all new filters matching log-side predicate
- Postgres: intersection semantics for window queries, FilterRange-based
  SQL generation for all range filters
- Fix InsertableMetricsSplit.window_duration_secs from Option<i32> to i32
- Rename two-letter variables (ws, sf, dt) throughout

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
style: fix rustfmt nightly formatting
f790519 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
feat(31): add shared invariants module to quickwit-dst
c55df1a 
Extract duplicated invariant logic into a shared `invariants/` module
within `quickwit-dst`. This is the "single source of truth" layer in
the verification pyramid — used by stateright models, production
debug_assert checks, and (future) Datadog metrics emission.

Key changes:
- `invariants/registry.rs`: InvariantId enum (20 variants) with Display
- `invariants/window.rs`: shared window_start_secs(), is_valid_window_duration()
- `invariants/sort.rs`: generic compare_with_null_ordering() for SS-2
- `invariants/check.rs`: check_invariant! macro wrapping debug_assert
- stateright gated behind `model-checking` feature (optional dep)
- quickwit-parquet-engine uses shared functions and check_invariant!

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
feat(31): check invariants in release builds, add pluggable recorder
70793f6 
The check_invariant! macro now always evaluates the condition — not just
in debug builds. This implements Layer 4 (Production) of the verification
stack: invariant checks run in release, with results forwarded to a
pluggable InvariantRecorder for Datadog metrics emission.

- Debug builds: panic on violation (debug_assert, Layer 3)
- All builds: evaluate condition, call recorder (Layer 4)
- set_invariant_recorder() wires up statsd at process startup
- No recorder registered = no-op (single OnceLock load)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
feat(31): wire invariant recorder to DogStatsD metrics
1e1e328 
Emit cloudprem.pomsky.invariant.checked and .violated counters with
invariant label via the metrics crate / DogStatsD exporter at process
startup, completing Layer 4 of the verification stack.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot
fix: license headers + cfg(not(test)) for quickwit-dst and quickwit-cli
f8ee0cd
@g-talbot @claude
chore: regenerate third-party license file
d40fd11 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
g-talbot and others added 14 commits April 8, 2026 10:54
@g-talbot
Merge branch 'gtt/phase-31-execute' into gtt/docs-claude-md
6f1bf5b
@g-talbot @claude
Merge main, address review feedback, fix merge conflict
4216827 
- Resolve postgres.rs conflict: keep check_invariant! macros, add
  window_duration_secs consistency check
- Group setup_dogstatsd_exporter + setup_invariant_recorder into
  single setup_metrics() function (fulmicoton-dd review)
- Rename `id` to `invariant_id` in invariant_recorder (fulmicoton-dd review)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot
Merge branch 'gtt/phase-31-execute' into gtt/docs-claude-md
4f118b3
@g-talbot @claude
style: rustfmt check_invariant macro argument
501f014 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot
Merge branch 'gtt/phase-31-execute' into gtt/docs-claude-md
cf37871
@g-talbot
Merge branch 'main' into gtt/docs-claude-md
03cf20f
@g-talbot @claude
revert: move code changes to separate PR
6e2bbe7 
Reverts c8bf8d7, cafcac5, a088f53 — these are code changes
(delete_metrics_splits error handling, doc comment tweaks) that
don't belong in a docs-only PR. They will land in a separate PR.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
remove ADR-004 from upstream PR — moving to DataDog/pomsky
86e0881 
This ADR contains company-specific information and should live
in the private fork, not in the upstream quickwit-oss repo.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
docs: rebrand for upstream — remove Datadog/Pomsky/Quickhouse references
b901734 
- Rewrite CLAUDE.md as generic Quickwit AI development guide
- Replace Quickhouse-Pomsky -> Quickwit branding across all docs
- Replace "Datadog" observability references with generic
  "production observability" language
- Remove "Husky (Datadog)" qualifier from gap docs (keep Husky
  citations — the blog post is public)
- Generalize internal knowledge (query rate numbers, product-specific
  lateness guarantees)
- Remove PomChi reference, private Google Doc link
- Add docs/internals/UPSTREAM-CANDIDATES.md for pomsky tracking

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
docs: remove ClickHouse references, track aspirational items
c0c127b 
- Remove all ClickHouse/ClickStack references from gap docs and ADRs
  (keep Prometheus, Mimir, InfluxDB, Husky as prior art)
- Restore gap-005 Option C (compaction-time dedup) without ClickHouse citation
- Mark /sesh-mode reference in CLAUDE.md as aspirational
- Add aspirational items section to UPSTREAM-CANDIDATES.md tracking
  items described in docs but not yet implemented (TLA+ specs, DST,
  Kani, Bloodhound, performance baselines, benchmark binaries)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
docs: fix aspirational items — TLA+ specs and Stateright models exist
e93bea7 
UPSTREAM-CANDIDATES.md incorrectly stated TLA+ specs and Stateright
models don't exist. They do (contributed in #6246): ParquetDataModel.tla,
SortSchema.tla, TimeWindowedCompaction.tla, plus quickwit-dst invariants
and Stateright model tests. Updated to accurately reflect that the
remaining aspirational piece is the simulation infrastructure (SimClock,
FaultInjector, etc.).

Also removed the /sesh-mode aspirational entry — it's actively being
used and the underlying specs/models are real.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
chore: add .planning to .gitignore
a7e4f1f 
Prevents GSD planning artifacts from being committed to the repository.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
revert: remove pomsky-specific Makefile changes
35fa272 
Reverts test env vars (CP_ENABLE_REVERSE_CONNECTION) and
load-cloudprem-ui target — these are pomsky-specific and
don't belong in upstream.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot
Merge branch 'main' into gtt/docs-claude-md
4006b20
@g-talbot g-talbot requested a review from mattmkim April 10, 2026 12:30
@g-talbot g-talbot mentioned this pull request Apr 10, 2026
Open
5 tasks
@g-talbot g-talbot changed the base branch from main to gtt/docs-claude-md April 10, 2026 14:15
@g-talbot g-talbot force-pushed the gtt/parquet-column-ordering-v2 branch from 9e5c6ef to cc4492e Compare April 10, 2026 14:18
@g-talbot g-talbot force-pushed the gtt/docs-claude-md branch from cc4492e to 4006b20 Compare April 10, 2026 14:18
g-talbot and others added 2 commits April 10, 2026 10:41
@g-talbot @claude
docs: add pitfall rule against silently swallowing unexpected state
b9bd5c8 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot @claude
feat: enforce physical column ordering in Parquet files for two-GET s…
946c229 
…treaming merge (#6281)

* feat: enforce physical column ordering in Parquet files

Sort schema columns are written first (in their configured sort order),
followed by all remaining data columns in alphabetical order. This
physical layout enables a two-GET streaming merge during compaction:
the footer GET provides the schema and offsets, then a single streaming
GET from the start of the row group delivers sort columns first —
allowing the compactor to compute the global merge order before data
columns arrive.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: verify input column order is actually scrambled

The sanity check only asserted presence, not ordering. Now it
verifies that host appears before service in the input (scrambled)
which is the opposite of the sort-schema order (service before host).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: rustfmt test code

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: collapse nested if to satisfy clippy::collapsible_if

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@g-talbot g-talbot force-pushed the gtt/parquet-column-ordering-v2 branch from cc4492e to 946c229 Compare April 10, 2026 14:42
g-talbot added a commit that referenced this pull request Apr 13, 2026
@g-talbot @claude
docs: update GAP-002, GAP-004, and ADR-002 with resolution status
6d7686c 
Mark completed items in GAP-002 (sort schema parser, configurable
directions, timeseries_id, schema-driven sort, metadata storage) and
GAP-004 (MetricsSplitMetadata fields, RowKeys, zonemap regexes,
sorting_columns, KV metadata). Update ADR-002 Implementation Status
to reflect the full PR stack (#6287-#6295).

Remaining open items: per-index metastore storage (Phase 32), null
ordering fix, Parquet column/offset index enabling, PostgreSQL
migration for row_keys + zonemap columns.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Base automatically changed from gtt/docs-claude-md to main April 13, 2026 23:16
@g-talbot g-talbot merged commit b469625 into main Apr 13, 2026
8 checks passed
@g-talbot g-talbot deleted the gtt/parquet-column-ordering-v2 branch April 13, 2026 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alanfgates alanfgates alanfgates approved these changes

@rishabh rishabh rishabh approved these changes

@mattmkim mattmkim Awaiting requested review from mattmkim

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@g-talbot @alanfgates @rishabh @mattmkim @hushy