docs(rfc-0005): spec the effective-timestamp fallback (time==0 → observed) as a derived column - #178
Conversation
…rved) as a derived column Records whose time_unix_nano == 0 (~15% of real OTel-Demo corpora — v5: 205,155 records, v6: 202,484 — 100% of which carry observed_time_unix_nano) are unaddressable by time today: the DSL window filters time_unix_nano, so they sit outside every real query window, and the B1 bench's zero-ts guard correctly refuses such corpora. The OTLP logs data model recommends "Use Timestamp if it is present, otherwise use ObservedTimestamp" — this amendment adopts that as an ingest-side derived value (maintainer decision 2026-06-11, option 1), never overwriting the wire value. - RFC 0005 §3.2: new OPTIONAL effective_time_unix_nano column (§3.8 rule 1 additive), writer-derived as `time_unix_nano if != 0 else observed_time_unix_nano.unwrap_or(0)`, with the OTLP Timestamp/ObservedTimestamp field definitions quoted. - RFC 0005 §3.4: the existing partition time-fallback is named as this rule; partition tuple and stored column never disagree. - RFC 0005 §3.6: encoding row (no dict, page index, delta-encodes — min/max stats make the B1 window predicate pruneable). - RFC 0005 §3.9: old-file read rule — absent column defaults to the row's time_unix_nano (not None), so pre-amendment files answer time-window queries exactly as before; the absent-OPTIONAL ⇒ predicate-false convention explicitly does not apply here. - RFC 0005 §5/§6: scenario RFC0005.13 + testing-strategy entry. - RFC 0002 §6.2: range(...) now filters the effective column; the bare `ts` field stays on the verbatim time_unix_nano. - RFC 0001 §6.1 / RFC 0003 §6.6: cross-ref notes — record/receiver shapes unchanged, RFC0001.10 verbatim preservation explicitly intact. No RFC status changes. B1 guard keying off the effective span is a code follow-up, not this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 6 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR amends four RFCs to define and integrate a derived ChangesEffective timestamp derivation across RFC chain
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR amends the documentation RFCs to specify an effective timestamp used for Parquet partitioning and time-window queries: when time_unix_nano == 0, fall back to observed_time_unix_nano, and persist that derived value as a new OPTIONAL Parquet column (effective_time_unix_nano) without overwriting the wire timestamp.
Changes:
- RFC 0005: Specify the new derived Parquet column
effective_time_unix_nano, align partitioning and query-window semantics to it, and document the old-file (absent column) default behavior. - RFC 0002: Update the Query DSL documentation so
range(...)is defined to filtereffective_time_unix_nano(with the documented pre-amendment fallback). - RFCs 0001 and 0003: Add cross-reference amendment notes clarifying record/receiver shapes remain unchanged and the derivation happens in the Parquet writer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/rfcs/0005-parquet-storage.md | Adds effective_time_unix_nano to the Parquet schema and defines partitioning/window semantics + migration story. |
| docs/rfcs/0002-query-dsl.md | Updates range(...) semantics to filter the effective timestamp rather than time_unix_nano. |
| docs/rfcs/0001-template-miner.md | Notes the effective timestamp is writer-derived and does not change mined record shape. |
| docs/rfcs/0003-otlp-receiver.md | Notes the receiver contract is unchanged; effective timestamp is derived downstream. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/rfcs/0005-parquet-storage.md (1)
748-786:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep legacy files on the pre-amendment partition rule.
§3.9says files missingeffective_time_unix_nanoshould behave astime_unix_nano, but this reader-validation paragraph still applies the new observed-timestamp fallback to every file. That can make older rows withtime_unix_nano = 0and a non-zero observed timestamp failReader::open_partitioneven though they were written before this amendment.🔧 Suggested wording fix
- The derivation algorithm is identical to the writer's in §3.4: prefer - `time_unix_nano` if non-zero, else fall back to - `observed_time_unix_nano` if present and non-zero, else the - 1970-01-01T00 epoch. + For files that include `effective_time_unix_nano`, validate the + partition tuple against the §3.4 effective-timestamp rule. + For legacy files without that column, validate against the + pre-amendment `time_unix_nano` path only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rfcs/0005-parquet-storage.md` around lines 748 - 786, The partition-validation currently applies the new observed-timestamp fallback to all files; change Reader::open_partition so it detects per-file whether the amendment column effective_time_unix_nano exists and, for files that lack that column (pre-amendment), compute the derived time for validation as effective := time_unix_nano only (do not fall back to observed_time_unix_nano), while preserving the existing observed-time fallback behavior for files that do have effective_time_unix_nano; keep Reader::open_file's current behavior of skipping partition validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rfcs/0002-query-dsl.md`:
- Around line 369-385: Update the RFC amendment text to precisely define
effective_time_unix_nano as "time_unix_nano if time_unix_nano != 0 else
observed_time_unix_nano.unwrap_or(0)" (make the unwrap/zero handling explicit)
and explicitly document the semantics of range(from, to) as half-open [from, to)
(or state the chosen inclusive/exclusive policy); reference the existing symbols
range(...), effective_time_unix_nano, ts, time_unix_nano, and
observed_time_unix_nano in the amendment paragraph so the contract is
unambiguous.
---
Outside diff comments:
In `@docs/rfcs/0005-parquet-storage.md`:
- Around line 748-786: The partition-validation currently applies the new
observed-timestamp fallback to all files; change Reader::open_partition so it
detects per-file whether the amendment column effective_time_unix_nano exists
and, for files that lack that column (pre-amendment), compute the derived time
for validation as effective := time_unix_nano only (do not fall back to
observed_time_unix_nano), while preserving the existing observed-time fallback
behavior for files that do have effective_time_unix_nano; keep
Reader::open_file's current behavior of skipping partition validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f394ce1-ae94-4c2b-b89d-ef3ca7c91037
📒 Files selected for processing (4)
docs/rfcs/0001-template-miner.mddocs/rfcs/0002-query-dsl.mddocs/rfcs/0003-otlp-receiver.mddocs/rfcs/0005-parquet-storage.md
…mn precision A plain coalesce never falls back (time_unix_nano is REQUIRED with a 0 sentinel) — the example is now the CASE form. The §3.6 time row points at effective_time_unix_nano as the primary window column. RFC 0002 pins the exact derivation (unwrap_or(0)) and half-open [from,to) bounds, aligning with the querier + RFC 0010. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The window SHALL compile against effective_time_unix_nano (the implementing slice follows; today the querier filters time_unix_nano); the half-open shape is the already-implemented part. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…05.13) (#179) Implements the #178 amendment. effective_time_unix_nano (OPTIONAL, stats-bearing) derived by the shared effective_time_unix_nano() fn (also PartitionKey::derive + the bench adapter — the never-disagree rule is structural). Querier time windows filter the new column with the §3.9 carve-out: absent column → time_unix_nano as before; present → (eff in-window) OR (eff IS NULL AND ts in-window), the IS NULL arm identifying exactly the DataFusion-NULL-filled pre-amendment rows; OR-shape stays inside the pruning grammar (two-row-group pruning test). Bench B1 eligibility keys off the effective span — observed-only corpora (v5/v6, ~15%) become measurable. Old files keep answering identically (CLAUDE.md §3.5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
DOC-ONLY RFC amendment (no
statuschanges, no code): specs the effective-timestamp fallback — records withtime_unix_nano == 0fall back toobserved_time_unix_nanofor partitioning and time-window queries, per the OTLP logs data model. Maintainer-decided 2026-06-11, option 1: ingest-side, derived — never overwriting the wire value.The measured blocker
~15% of records in real OTel-Demo corpora (v5: 205,155 records; v6: 202,484) carry
timeUnixNanoabsent/0, and 100% of those carryobservedTimeUnixNano(verified by sampling). Today those records are unaddressable by time — the DSL window filterstime_unix_nano, so they sit outside every real query window — and the B1 bench's zero-ts guard correctly refuses such corpora. B1 is the last unmeasured thesis gate.The OTLP-spec citation
Quoted verbatim in the RFC 0005 §3.2 amendment, from the OTLP logs data model (Field:
Timestamp/ Field:ObservedTimestamp, https://opentelemetry.io/docs/specs/otel/logs/data-model/):The design
effective_time_unix_nano := time_unix_nano if time_unix_nano != 0 else observed_time_unix_nano.unwrap_or(0), computed by the Parquet writer from the two existing record fields — the same rule RFC 0005 §3.4 (andPartitionKey::derive) already runs for partitioning, now stored.MinedRecord/OtlpLogRecordare unchanged; the wiretime_unix_nanois stored verbatim including0— RFC 0001 scenario RFC0001.10 (verbatim preservation) is explicitly intact.time_unix_nanosoDELTA_BINARY_PACKED+ ZSTD collapse it; a real column (with min/max stats, §3.6) is what keeps the window predicate row-group-pruneable — a query-timecoalesce()would defeat stats pruning.range(...)filterseffective_time_unix_nano(RFC 0002 §6.2 amended); the baretsfield stays on the verbatimtime_unix_nano.effective := time_unix_nano— exactly the pre-amendment behaviour, so historical files keep answering queries identically. The querier's absent-OPTIONAL ⇒ predicate-false convention (RFC0007.4) explicitly does not apply to the window filter — that would silently hide all pre-amendment data.0preserved; old file without the column → behaves astime_unix_nano) + §6 testing-strategy entry.Files touched (docs/rfcs only)
docs/rfcs/0005-parquet-storage.md— §3.2 column + dated amendment (with the spec quotes), §3.4, §3.6, §3.9, §5 RFC0005.13, §6.docs/rfcs/0002-query-dsl.md— §6.2: thetstable row previously saidtime_unix_nanois "whatrange(...)filters"; that text fought the design head-on, so it carries a dated amendment note (the only RFC touched beyond the planned three — without it the doc set would self-contradict on what the window filters).docs/rfcs/0001-template-miner.md— §6.1 cross-ref note: record shape unchanged, RFC0001.10 intact.docs/rfcs/0003-otlp-receiver.md— §6.6 cross-ref note: receiver contract unchanged (wire-0→Nonerule for observed stands; nothing materialised onOtlpLogRecord).Invariants / hazards (CLAUDE.md §3.5, §4 H5/H6)
Schema change goes through the RFC gate as required: additive OPTIONAL column (§3.8 rule 1 — the preferred §3.5 path), explicit migration story for historical files (the §3.9 absent-column default), readers keep handling absent columns without error. No status change on any RFC.
Verification
mdbook buildclean (benign mermaid version warning only); amended sections re-read for internal consistency.git statusshows onlydocs/rfcs/*.md— no code, no schema bytes.🤖 Generated with Claude Code
Summary by CodeRabbit