mirror of
https://github.com/GreptimeTeam/greptimedb.git
synced 2026-11-14 06:46:56 +00:00
discord9
f11173f193
I hereby agree to the terms of the [GreptimeDB CLA](https://github.com/GreptimeTeam/.github/blob/main/CLA.md). ## Refer to a related PR or issue link (optional) ## What's changed and your intention? ### The problem A raw series stream reports when a series is complete by setting `is_last` on a chunk. Its contract is "the final chunk for this series in the input stream exposing it", but the producer only ever set it when the next chunk of a **different** series arrived, or when the input ended. So a consumer could not tell a series that is finished from one that is still being written. It had to buffer a whole series before it could act on it, which is a cost paid whether or not the series is long. ### What this changes `FlatSeriesBatchDivider` already emits a package only once it holds every row of the series in it, so the record that ends such a package also ends the last series in it. `raw::make_stream` flattened those packages into a plain record sequence and dropped that fact. Records now carry it: - **The fact survives being held back.** A record held for the next predicate group keeps its flag instead of losing it in the round trip. - **A flagged record ends its group.** The group's end and the package's end become the same record, so a record of the *next* package can never be closed by the wrong boundary. - **The tail key is taken before the residual predicates run.** A predicate may drop the run that would have closed the series while the package still holds all of its rows. - **Closing a package clears `pending`, so the ordering check keeps its own baseline.** Without it the next package's first chunk would lose the key it must exceed and a cross-package regression would go unnoticed. - **A series that continues past the package that completed it is rejected**, not yielded: it would be split into two runs that both claim to be final. The prepared columnar source never carried the fact at all. Its per-range readers cut complete series out, but the row-level merge behind them re-cuts the stream by batch size, so a `Flat` package boundary is not recoverable there. An earlier revision re-divided that stream to restore packages; this revision drops that and yields ordinary `RecordBatch` instead, which is why the closing below applies to the legacy producer only. The one surviving change in `prepared.rs` skips empty batches. `SeriesBatch`'s doc said "batches of the same series", which contradicts the divider that produces it (a package may hold several complete series). Corrected. ### Limitations - **No performance claim.** This changes *when* a consumer can observe a complete series, not how much work the producer does. A consumer that already buffers whole series gains nothing until it drops that buffering. - Package-completion information is used only under `KeyThenTime`, which is what the one real flat producer declares: `Flat` comes from the legacy distributor and its stream is globally ordered. A series that reopens after the package that completed it is rejected by the ordered-key comparison (`legacy series primary keys are not strictly increasing`), covered by `a_key_closed_by_a_package_must_not_continue_in_a_later_one`. Under `Contiguous` nothing closes a flat series from package metadata, and there is no `Contiguous` flat producer to need it. - This does not change the `is_last` meaning or the wire representation; it stops under-reporting it. ### Testing Thirteen new tests. The central one, `a_complete_package_closes_its_last_series_without_the_next_package`, feeds one package and then never produces anything again (no next package, no EOF, no waker). It asserts both chunks arrive with `is_last` set. With the package fact removed the test hangs on the second chunk — that is how the old behaviour was confirmed. The others cover a package tail the predicate empties; a series continued after its package, in both the same-key and after-another-key shapes and under `KeyThenTime` (the only ordering a flat producer declares); an ordinary `RecordBatch`, which says nothing about where a series ends; and the batching case below. `record_batches_separated_by_pending_share_one_predicate_pass` and the corrected `filtered_package_tails_do_not_close_an_incomplete_ordinary_tail` pin a regression this revision also fixes: the fill loop awaited upstream only when a package tail was held, so a brief `Pending` no longer splits one predicate pass into many. Previously one hundred small record batches separated by `Pending` cost one hundred predicate passes. Existing tests that pin boundaries are unchanged and still pass: `upstream_error_is_terminal_after_yielded_prefix_without_flushing_pending`, `duplicate_or_descending_cross_batch_time_is_terminal`, `empty_filtered_group_keeps_the_preceding_series_last_flag`. Locally: `cargo nextest run -p mito2 -E 'test(series_scan)'` 45 passed, `cargo clippy -p mito2 --all-targets -- -D warnings` clean, `cargo fmt --all --check` clean. Also ran `-E 'test(series) or test(scan_test)'` (284 passed) to cover consumers of the raw producer. No CI run claimed. ## PR Checklist Please convert it to a draft if some of the following conditions are not met. - [x] I have written the necessary rustdoc comments. - [x] I have added the necessary unit tests and integration tests. - [ ] This PR requires documentation updates. - [x] API changes are backward compatible. - [x] Schema or data changes are backward compatible. - [ ] This PR needs to be backported to release branches, and I have added the `backport-<target>` labels (e.g. `backport-v1.3` targets `release/v1.3`).