Repository navigation
Keep accepted current-minute rows queryable after periodic flushes - #53
Merged
Merged
Conversation
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
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.
A normal non-forced staging flush cleared query-visible memory even when current-minute Arrow disk writers stayed open. Accepted rows could disappear until a later flush converted those writers. Memory now follows the exact disk filename owner; writer selection and memory eviction share one clock sample under the existing mutex. Forced flush and clear empty memory.
Review fixes R1/R2: empty segment maps release all buckets; sparse maps shrink when capacity exceeds saturating
2*len+16. Disk ingestion computes the filename once and uses borrowed lookups for both maps, allocating owned keys only on misses. Query user streams again use directmem.push, skipping filename formatting/sorting; Query internal streams remain disk-owned and segmented. No new locks or Arrow payload copies at ingest/eviction.Cost qualification remains pending. Repeated disk batches with existing memory/disk keys remove one filename clone allocation. A new file with both keys absent instead has one extra transient filename allocation (three filename/result/key Strings versus two previously); a sparse-map shrink adds rehashing and allocator work and can temporarily hold old and new tables. These are static code counts, not measured timing or allocation totals. Retaining valid current rows also requires more payload RAM than the incorrect baseline, plus per-filename metadata and per-segment buffer capacities. There is no new hard byte cap or measured CPU/RAM zero-regression claim.
Compatibility: HTTP and exported Rust signatures remain unchanged, including
Writer.mem: Option<MemWriter<4096>>andpush_disk(String, ...).Stream.writerexposes inferred internals despite the private staging module. Both observable internal behavior changes are disclosed: disk-owned rows live in private segments, so publicread_buffer/mutable_bufferexpose only unsegmented data (recordbatch_clonedis the complete view); publictake_flushable_disknow also clears ownerless memory and evicts detached-owner segments, whereas it previously only detached disk writers. Query user streams retain direct-buffer behavior. Across disk-owned segments, HashMap batch order replaces reverse ingest order, soLIMITwithoutORDER BYmay choose an arbitrary segment; SQL does not guarantee that ordering. No expensive sorting, visibility narrowing, or mirrored buffers is added.Validation at the review-fix head: rustfmt 1.96.0 on the changed files and
git diff --checkpass; source inspection confirms unchanged public fn signatures and borrowed lookup paths. Five targeted regression tests are prepared, including a 128-owner capacity burst with retained Arrow pointer identity, Query user direct buffers, Query internal retention, and exact minute-end Stream flushing. Test memory setup is deterministic without global env mutation. This head has not been compiled or runtime-tested: the parent still owns an active native build pinned to the old head24b484d77287cef8de084774b9abeb3bcf34846e. Full native tests, fixed production ELF/rollover verification, and performance qualification remain pending. Earlier reduced-harness results (baseline failure, three fixed passes) apply only to the old head and are not proof for this revision.Parent follow-up after the active builder: run
cargo +1.96.0 test --lib --release --offline --locked --jobs 2 current_minute_query_visibility -- --nocapture --test-threads=1in the admitted isolated build scope, also withENABLE_MEMORY_STAGING=false, then complete native rollover and CPU/RAM qualification. New evidence: localpr-qualification-20261019/pr53-cost-fix/assessment.txt; earlier logs stay undercurrent-minute-fix.The detached-Arrow-to-Parquet conversion/publication gap, object-store upload/snapshot races, and PR50 lifecycle issues remain outside scope. Original main/PR50 rollover failures and recovery after reopening are baseline evidence only. This draft qualifies no benchmark and authorizes no merge.
Need help on this PR? Tag
@codesmith-botwith what you need.