Repository navigation
Conversation
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>
There was a problem hiding this comment.
The correctness fix is needed, but its sorted-query cost remains a merge blocker under this PR's performance requirements. At d594084e, two completed ABBA blocks measured sorted top-10,000 median latency increases of 26.3% / 25.1%, with request CPU increases of 6.3% / 17.7%. The fixture is one 20,000-row Parquet file with a single row group; output bytes match on that valid-sorted fixture. The original falsely ordered plans remain incorrect on shuffled/overlap controls and must not be restored.
The current diagnosis is that removing the ordering claim restores a real sort and additional decoding. The split between those costs still needs measured plan/operator evidence. Per-file scheduling alone cannot address a single-file case.
Before resolving this finding, please retain the independent wrong-result controls, demonstrate any replacement rule actually activates through the production optimizer configuration, and compare latency/CPU/peak memory on the retained fixture. If a dense-TopK/full-sort alternative is used, test underestimated row counts with both spill enabled and disabled and compare its resource/error behavior to the heap path. A row estimate or estimated-byte gate is not a hard memory bound. Do not accept a new availability regression to hide the sorted-query slowdown.
Evidence and the independent design review are retained in the benchmark archive under pr-qualification-20261019/pr48-pr50-balanced and pr48-opus-design-r1. This thread tracks qualification of the existing slowdown; it does not assert that the unexecuted local candidate is already faster or safe to merge.
There was a problem hiding this comment.
The local full-sort experiment at 331bdcf22c3192391e8e647ccb448ee2424dd1be has been rejected; it was never pushed to this PR. Matched component controls confirm a new query-memory failure: with the accurate historical 20k-row fixture and an 8 MiB pool, the existing heap returns the oracle-correct result (1,626,679-byte pool peak), while the candidate fails requesting a 10 MiB merge reservation. The 80k-actual/16k-estimated control also fails only in the candidate with spilling enabled. No timing matrix or native campaign ran.
The source suite had four passes and two failures, including this availability defect and a separate footer-without-sorting-columns panic requiring diagnosis. Original PR48 correctness protections remain intact, but the measured slowdown and this review thread remain unresolved. A narrower single-batch/fallback design is only a feasibility investigation; no performance or resource-parity claim is made.
Verified results and graphic: Mayfly qualification page, revision 40. This page requires existing wiki access.
Description
Fix incorrect query ordering caused by advertising unverified timestamp ordering to DataFusion. Writer-produced files can contain out-of-order rows despite a
DESC NULLS LASTfooter claim; trusting that claim lets a query omit the required sort and return the wrong rows.Manifest and hot-tier scans now omit that unverified ordering claim. The public
partitioned_filessignature remains unchanged, and regression tests exercise production scan planning with real Parquet data and independent row oracles.Manifest construction also handles absent, empty, or invalid footer sorting metadata conservatively instead of panicking. Every row group keeps its position in the metadata comparison; an invalid column index discards that entire group's claim. Valid key order, nested paths, direction and null placement are preserved. These footer declarations are not promoted into verified ordering.
Retained-plan diagnosis and draft disposition (R37)
Keep PR48 draft; no merge or performance qualification. The public correctness defect is real: retained concurrent-ingestion responses contain eight timestamp inversions on the main-equivalent baseline and zero on PR48. The completed investigation found no justified small safe change that restores the old optimization. No further benchmark or speculative patch is launched by this disposition.
All eight retained R19 baseline/PR48 EXPLAIN ANALYZE plans were compared against their original response bodies and exact SQL/fixture pins. For the synthetic sorted 20k-row file with LIMIT 10k:
Correction to the earlier R22 interpretation: the evidence does not show that A reads half the physical file or that C doubles physical I/O/decode. Both report the same scanned bytes and predicate matches. C does additional selection/output work. Time-filter pushdown remains in both; C's dynamic TopK threshold cannot prune the already-delivered single batch. The separate EXPLAIN timers cannot be equated to whole-process CPU in the measured request windows.
R19's CPU +33.18%/+34.67% and query p50 +37.27%/+40.42% remain workload-specific block observations. The plan difference is a credible contributor, not proof that the entire gap is unavoidable correctness cost. The actual public no-LIMIT reproduction uses a different union plan: its populated scan needs the protecting sort. The tested public LIMIT queries already have active TopK in both builds and return correct results; this does not justify trusting all LIMIT scans.
Recovering the ordered-scan optimization safely requires trustworthy evidence about the complete physical timestamp sequence, including batch/row-group boundaries and concatenated files. Existing footer flags, min/max values, a sorted prefix, or LIMIT presence are insufficient. New write-time verification and a certificate could enable a future fast path for eligible new files, but add verification/metadata work and do not certify existing files. Their CPU, RAM, compatibility and planning costs are not qualified. The rejected reserve/full-sort/heap experiments remain rejected.
R35/R36 reduced and audited the cache-sensitive file-RSS offset; residual +0.822 MiB after queries is still not memory parity. Source remains unchanged at
35988060d465aa74030990988e4ad0295d936d4e. No acceptance of the resource cost, 20% improvement claim, API change, or new implementation follows. Evidence:pr48-retained-plan-cpu-r37/{REPORT.md,comparison.csv,evidence.json}. Active investigation concludes here with the PR retained as draft for a separately scoped verified-ordering design, if pursued.Independent cold-copy evidence review (R36)
The independent Opus audit reproduced R35's +0.008 MiB pre-query / +0.822 MiB post-query file-RSS median gaps, checked all eight zero-residency checkpoints, bound the real server PID to all twelve snapshots, and verified 44 correct query responses. This is sufficient to stop pursuing the earlier large file-RSS offset as the next optimization target. It is not memory parity, sole-cause proof, or a merge qualification. PR48 remains draft.
Limits retained: only two lifetimes per arm; the post-idle snapshots duplicate post-query values and add no samples; the last cold check precedes actual native start by about 162–171 ms; all queries use the sorted sequential control, not the concurrent stream that reproduces the ordering bug. One private state copy is reused and metadata is rewritten between lifetimes, although target data remain unchanged. Anonymous memory and startup faults vary by run order, so total/anonymous-memory comparisons cannot qualify the change. PRESENT section-page counts identify residency, not direct code touches or exact causal attribution.
Timing clarification: 364.84 s was setup before launch, not total engineering elapsed time. Start through final seal was about 717 s, within the 1,200 s allowance, while the separately capped closure still exceeded 240 s by 11.35 s. The deviation remains recorded; no pass reclassification. Historical source/dependency provenance limitations remain unchanged.
The next step is a read-only comparison of already retained R19 A/C EXPLAIN ANALYZE plans, including active versus empty branches, scan work and LIMIT pushdown. No duplicate native experiment is needed to obtain those plans. The synthetic 20k-row CPU regression remains unresolved; correctness reproduction R27 remains established. Evidence:
pr48-cold-evidence-audit-r36/{REPORT.md,evidence.json}. This review supersedes the R35 chart's historical “independent audit pending” caption; plotted values are unchanged.Controlled cold-copy memory diagnostic (R35)
PR48 stays draft. Starting both unchanged executables from independent, verified-cold copies substantially reduced the observed file-backed RSS gap. Before queries, the PR48-minus-baseline median gap changed from R33's warm-original +11.080 MiB to +0.008 MiB; after queries, from +6.723 MiB to +0.822 MiB. These are comparisons between diagnostics, not a code optimization or a speedup. They support cache-state influence without establishing sole causation, memory parity, or adoption.
One fixed ABBA sequence completed four fresh lifetimes. All 8 cold checkpoints, 12 memory/PRESENT-page snapshots, and 44 query oracles passed. Only the new private executable copies received cache advice; historical binaries and unrelated cache were untouched. Both copies remain retained. Same real 8,000-row sorted dataset, 4 CPU / 12 GiB, 2 GiB query pool, ordinary release/Jemalloc. No source or compiler changes. No new throughput, latency, or CPU-performance conclusion.
Post-query total RSS varied: baseline 152.473 / 124.984 MiB, PR48 133.367 / 124.383 MiB. Anonymous-memory variation prevents a memory-superiority claim. The residual file gap is chiefly in executable pages; the earlier large read-only-mapping excess did not recur. Per-section PRESENT-page intersections do not prove direct touches and are not additive at shared boundaries. The historical baseline dependency-hash caveat remains.
Recorded deviation: evidence closure took 251.35 s against a 240 s cap. The overrun was disclosed after sealing; raw measurements were unchanged. Native time was 63.84/360 s; setup before launch was 364.84 s and start through final seal about 717/1,200 s. All owned scopes/listeners/lock holders closed. Final projected storage was 191.81244/192 GiB, including 3 GiB headroom; only unused completed reservations were released, no historical evidence deleted.
Independent Opus review R36 is complete; qualifications and timing corrections are stated above. The separate synthetic 20k-row CPU regression remains unresolved; this memory diagnostic does not erase it. Evidence:
pr48-owned-cache-residency-r35/{REPORT.md,results/chartdata.json,manifest.json}. No additional performance experiment is launched by this publication.Current public-API reproduction (R27)
A single finite trial reproduced incorrect ordering using supported public HTTP ingestion, natural Parquet persistence and graceful reopen. A main-equivalent baseline wrote 120,000 rows through eight concurrent producers and an 8,000-row sequential control. Baseline A and exact PR48 head C then queried the same unchanged target data and catalog.
Baseline returned all correct rows but violated descending timestamp order at eight request boundaries (maximum upward step 2 ms). PR48 returned the same complete population correctly sorted. All tested fixed-LIMIT queries and the ordinary
DESC LIMIT 1000control passed in both arms, as did the sequential controls. This establishes a no-LIMIT, explicit NULLS LAST ordering error, not a demonstrated LIMIT failure or a universal UI-query failure in this trial.The baseline plan used SortPreservingMerge over a populated Parquet branch advertising the unverified order. Two other SortExec branches produced zero rows. PR48 sorted the populated branch. Full id/value membership, request timestamp constancy, timestamp mapping and target Parquet/manifest/snapshot/schema hashes matched. Expected ordering was computed independently in Python from the complete native unordered mapping, allowing arbitrary tied IDs; no separate compressed-page decoder was used. All three native sessions exited 0 and owned scopes/listeners/lock closed. These are correctness diagnostics, not performance measurements. Independent R28 audit recomputed the raw request/response evidence and supports this narrow reproduction with no blockers. It reverified all 128 ingest acknowledgements, complete row mappings with timestamp ties allowed, eight baseline ordering violations versus zero in PR48, physical-plan differences, unchanged target data/catalog and successful closure. Plain DESC was exercised only with LIMIT 1000; the UI was not exercised. One timing-dependent trial does not establish frequency, and mutex interleavings were not instrumented.
A is pinned ordinary-release/Jemalloc binary with source tree equal to main
5015557, not a latest-remote-main binary claim; C is exact35988060. R23's earlier custom-time attempt was negative because the OSS API ignored that unsupported header. Historical C3 evidence remains separate and source-unpinned. R27 uses neither custom partitioning nor synthetic/rebound data.PR48 remains draft at the user's request. The next task is a lower-cost correction that preserves fast queries only where ordering is actually proven, with public API compatibility and measured conversion/query CPU and memory costs. No performance waiver, merge or new optimization is approved.
Validation
Fresh component red/green verification of footer fix
35988060: the original production code with the same saved tests had one preservation pass and four expected unwrap/bounds failures; the fixed code passed all five tests. The tests write real Parquet pages, rewrite footer declarations, reread those declarations and decode every row before exercising production manifest construction. Both arms use the same pinned dependencies. Rustfmt 1.96 and diff checks pass. This is extracted production-component coverage; full-root CI for this exact head passed: eight successful checks and two skips, including Rust tests, Clippy, and standalone/distributed smoke tests. No performance measurements were taken for the footer fix.Historical ordering evidence remains separate: 169 HTTP cases and 827 library tests (2 ignored) passed in the isolated experiment; the baseline failed 5 of 7 regression cases. That experiment included an unrelated staging-projection change excluded here. The production-file regression is ignored by default and requires
ORDERING_SOURCE_PARQUET.Memory mapping diagnostic (R33)
A separate fixed ABBA diagnostic captured 12 per-mapping snapshots across four fresh lifetimes. All 44 queries were correct; original data and prior evidence stayed unchanged, and all owned resources closed. This is memory attribution, not a performance repeat or a parity qualification.
After queries, PR48 minus baseline file-backed RSS was +6.723 MiB: +6.689 MiB in the main executable, about +0.033 MiB in shared libraries, and zero in other mapped files. Main-executable read-only mappings account for +6.238 MiB, executable mappings +0.455 MiB, with small writable differences. These are mapping permission classes, not proven ELF section or symbol attribution. Overall median RSS was 131.38 MiB on A and 137.75 MiB on C; anonymous RSS was about 0.36 MiB lower on C in this diagnostic. The offset was already present before SQL.
The binary is only 28,040 bytes larger, and mapped read-only virtual size differs by only 44 KiB. The result therefore identifies where the extra pages are resident, but does not explain why: layout, touched pages, fault-around or compilation effects remain unproven. It does not demonstrate an extra anonymous allocation from sorting. R34 static review verified the exact mapping attribution and found no recorded toolchain, profile, features, allocator, assets or normalized compiler-argument mismatch. Dependency reuse is strongly supported, but A's dependency bytes were not hashed at link time, so exact historical byte equivalence is not proven. The affected read-only mapping contains several sections; aggregate smaps cannot identify which hold the extra pages. Original executables were warm in page cache, and code layout also differs; neither cache state nor layout is established as the cause. No compiler or query change is justified by this result. A bounded successor will compare verified-cold private executable copies with the same data/query protocol, conditional on fresh admission within the unchanged 192 GiB cap. It may release unused completed-test reservations, but will retain all actual evidence and binaries; only private-copy cache advice is authorized, never a global cache flush. A smaller offset would support cache-state influence, not prove sole causation or waive resource parity. Static evidence:
pr48-elf-residency-static-r34/REPORT.mdandresults.json.All 12 mapping RSS sums matched nearby rollup/status RSS in this run. Snapshots were sequential, not atomic, and post-query capture began about 51–69 ms after the last body read. This distinct workload does not retroactively attribute every R31 page, waive its RSS gap, or prove CPU/memory parity. PR48 remains draft. Evidence:
pr48-memory-mappings-r33/REPORT.md, raw maps/smaps snapshots andresults/attribution.json.Real writer-produced sorted-file comparison (R31)
Completed one comparison on the audited R27 sequential 8,000-row file, copied byte-for-byte into private isolated state. Eight fresh lifetimes in ABBA / BAAB blocks covered four query cases: no-LIMIT explicit DESC NULLS LAST, explicit LIMIT 1000/4000, and plain DESC LIMIT 1000. All 1,600 measured responses and 320 warmups passed the independent tie-safe oracle. Original data/catalog remained unchanged; all native exits were 0 and owned resources closed.
For no-LIMIT, PR48/baseline median ratios were:
CPU direction disagrees and empirical ranges overlap; no stable CPU cost or saving is established. No-LIMIT median latency is close. Sampled RSS is higher for PR48 in every case/block (about 2.9–5.9%); CPU/RAM parity remains unqualified. The cause of the RSS difference is not established. The anomalously large apparent CPU reduction for block-2 LIMIT 1000 includes a high baseline sample, retained without exclusion; it is not an established engine speedup.
The populated no-LIMIT branch gains a real sort in PR48; all three LIMIT variants retain active TopK in both arms. Native release/Jemalloc, 4 CPU / 12 GiB ceiling, 2 GiB query pool; this is a small-file diagnostic, not capacity or a 20% improvement. CPU windows include client-validation gaps and server background work; these gaps are roughly 2.5 seconds of a 3-second no-LIMIT window. HTTP latencies exclude oracle work. RSS is sampled at 50 ms, not a continuous peak. Two lifetimes per arm per block cannot establish statistical equivalence. Independent R32 audit supports the recorded correctness, ratios, identity and closure, with no data errors found. It confirms that strict RSS parity remains unestablished. Of the total median RSS difference (+4.0 to +8.2 MiB by case/block), the file-backed component differs by +5.3 to +6.2 MiB; anonymous-memory ranges overlap in seven of eight comparisons. A file-backed offset is already present before querying (A idle 47.7–48.0 MiB, C 58.1–59.6 MiB). No per-mapping snapshots were retained in R31, so this cannot yet be attributed to the main binary, libraries or sort. Charged cgroup memory omits already cached binary pages and includes launcher memory; it cannot replace RSS to waive the gap. A bounded per-mapping diagnostic is the next step, not a new optimization or merge waiver.
The audit also found four brief server CPU bursts responsible for much of the reported CPU-ratio variation; their cause is unknown. All original samples remain in the published results, with no post-hoc capping or exclusions. The wall windows are paced by external validation work, but server CPU counters do not include the driver CPU. Separate EXPLAIN observations are not measurements of every timed request. R31's LIMIT boundaries happen to align with 1,000-row timestamp groups, so this trial does not exercise a split tie group. Evidence:
pr48-real-sorted-audit-r32/REPORT.md,audit.json, and retained read-only verifiers. The verifier's corrected timezone-parsing error is audit history, not an R31 server failure.This actual writer-produced file is distinct from the synthetic 20k control below; neither result generalizes over the other. PR48 stays draft while the resource difference is assessed. The larger certificate design has not been implemented.
Draft blockers and performance evidence
The completed native comparison used two balanced A,C,D,D,C,A blocks: A is a retained binary with the same source tree as main
5015557, C is this exact PR head35988060, and D adds an experimental four-line TopK capacity reservation. All 12 fresh-server lifetimes and 2,400 measured responses passed the independent exact-row oracle for the sorted 20k-row, one-file, LIMIT 10k control. No noise, OOM or swap gate failed. These are ordinary release builds with matching production jemalloc, a 4 CPU / 12 GiB server ceiling and a 2 GiB query pool; not release-LTO or deployment-capacity results. Build provenance and results have been independently reviewed. The result audit rehashed all three binaries, independently validated archived response bodies and all measured-response digests, and recomputed the reported ratios. It supports rejecting the reserve candidate; it does not establish API-wide or capacity safety.This PR remains blocked under the requested CPU/RAM parity requirement. Within-block median ratios for the unchanged PR versus main were:
These are descriptive results from two blocks, not universal regression estimates. Server CPU spans each first-send to last-full-read window, including inter-request gaps; the external client performs validation between requests. RSS is sampled at approximately 50–102 ms, not a continuous peak. A/C also includes the catalog footer-safety change, so the full difference is not attributed solely to the query-provider edit. Main can omit sorting by trusting an unverified ordering declaration. That happens to produce correct answers for this sorted control, but is incorrect on other row layouts covered by the regression tests. The current writer does not provide a general physical-order guarantee. No safe fast path based solely on existing legacy footer claims has been established. Verification of new files or decoded contents remains under investigation; this patch is a correctness fix with a measured cost, not a performance improvement.
The reserve-only candidate is rejected and not included: its effect versus the unchanged PR overlapped noise in block 1, while block 2 used +4.11% CPU and +1.78% RSS with +6.19% median latency. Partial component/System-allocator gains did not establish a native benefit. The earlier full-sort wrapper failed availability/memory controls, EOF probing risked delayed dynamic pruning, and bulk heapification changed tie eviction and increased retained memory. None is included. No further repeats of these candidates are planned.
Writer footer claims and the legacy ListingTable path remain separate correctness limitations. No object-store, distributed, hot-tier HTTP, ingestion-throughput, API-wide, RAM-parity, or deployment-capacity qualification is claimed by this native control. Exact-head CI standalone/distributed smoke tests passed. The user has requested that this remain draft while a correction with less performance loss is investigated; no merge exception is being sought.
Results and graphics in Mayfly require existing wiki access. The latest head merges main's already-merged response-serialization fix and adds only the footer safety repair to the original PR48 change; it contains no experimental optimizer or dependency patch.
Need help on this PR? Tag
@codesmith-botwith what you need.