Skip to content

fix(query): stop trusting unverified Parquet scan ordering - #48

Merged
VibeSWE merged 4 commits into
mainfrom
t3/fix-ordering-claims
Oct 21, 2026
Merged

VibeSWE merged 4 commits into
mainfrom
t3/fix-ordering-claims

Conversation

@VibeSWE

@VibeSWE VibeSWE commented Oct 19, 2026 •

Copy link
Copy Markdown

Fix incorrect timestamp ordering when persisted Parquet rows do not match their footer sorting declaration. Concurrent requests can acquire timestamps before acquiring the writer lock, so append order is not proof of timestamp order. The reader must retain the sort even when metadata advertises an ordering.

The public no-LIMIT reproduction returns all 120,000 correct rows but eight strict timestamp ascents on the original reader. This change returns the same population correctly ordered. Equal-timestamp row order is intentionally unconstrained.

Changes

  • Stop advertising unverified scan ordering on the manifest and hot-tier reader paths.
  • Prevent the legacy ListingTable reader from inferring order from Parquet footers, while preserving statistics and pruning. This also avoids the invalid-sort-column indexing panic on that path.
  • Tolerate absent or invalid footer sorting metadata without a catalog panic.
  • Exercise real staging-writer files, row-group boundaries, null placement, custom time partitions, aggregates and LIMIT behavior in regression tests. Two new tests use the production legacy listing builder and LocalFS: a writer-produced file with false ordering and a malformed footer sort index.

The optional serializer is isolated in #54. No serializer, typed TopK, dependency fork or ordering-certificate feature is included here.

Performance tradeoff

A completed application campaign compared original main A, this reader fix B at 35988060, and B plus a separate selective serializer C. Three counterbalanced blocks completed 18 fresh server lifetimes and 84,600 timed requests. An independent audit recomputed every cell and ratio from raw records.

  • Dense sorted-file LIMIT 10000 over 20,000 rows: B used 49.3–49.7% more CPU with the default response and 43.4–44.7% more CPU with null filling. The added TopK sort is visible in the plan.
  • The separate serializer reduced B's null-filled dense CPU by 4.9–5.2%, but did not offset the default dense-query cost.
  • All 600 original-reader no-LIMIT responses were incorrectly ordered; all 1,200 corrected-reader responses were ordered correctly. Wrong-result timings are excluded from comparisons.
  • Memory parity and production-capacity impact remain unestablished. These are workload-specific descriptive observations, not universal regression estimates or significance claims.

See the checked-in evidence summary for the matrix, source/ELF provenance limits, raw-artifact hashes and interpretation. The incomplete earlier R181 campaign is not pooled. Historical optimization failures remain failures.

Validation and disposition

Full-crate CI now passes on 4282f781e81404e02423ebdfe6485ae24f5b2d95 (CI run): Rust tests, Clippy, image build, and standalone/distributed smoke suites are green. Both new production-path regression tests passed: listing_table_ignores_footer_sort_order and listing_table_plans_footer_with_invalid_sort_column. Formatting and independent Opus/Sol source reviews also passed. The source-level negative-control expectation (tests fail without the wrapper) has not been executed.

The measured campaign predates the legacy listing repair. Ordered legacy queries may now pay for a required sort; that path’s performance has not been measured.

Merge authorized by the user on 2026-10-21 after review of the correctness-first tradeoff and completed CI. The known dense-query cost is accepted for this correctness repair; it has not been eliminated and no CPU/RAM parity claim is made. The optional serializer is reviewed separately. A safe future ordered-scan optimization would require verified physical ordering and a trustworthy certificate; existing files cannot acquire that guarantee merely from footer flags or min/max statistics.

VibeSWE and others added 2 commits October 19, 2026 00:45
Manifest and hot-tier Parquet scans declared `time DESC NULLS LAST`
output ordering whenever the footer-derived `sort_order_id` said so (one
file needed no further proof; multi-file groups were chained by min/max
statistics). The staging writer records that footer claim without
sorting: concurrent requests are appended in writer-lock order and
time-partitioned rows keep client order. With the claim, DataFusion
drops the sort under `SortPreservingMergeExec` and
`ORDER BY p_timestamp DESC NULLS LAST LIMIT n` over a historical window
returns wrong rows.

Remove the ordering declaration and its helpers. The scan body moves to
a private `parquet_scan_plan` so tests exercise the real plan code
without the CLI global. `pub fn partitioned_files` keeps its signature;
its flag is now always `false`.

Add regression tests over files from the real staging writer, checked
against an independent tie-aware sort of the decoded rows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/query/stream_schema_provider.rs
@VibeSWE
VibeSWE marked this pull request as ready for review October 21, 2026 13:40
@VibeSWE
VibeSWE merged commit ef2bc79 into main Oct 21, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant