You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix RDL temporal queries for all index clocks - #165
Make query_temporal and query_temporal_lengths follow the index clocks introduced by #123. Request anchors and relative bounds remain epoch seconds, while an index can store nanoseconds, microseconds, milliseconds, seconds, hours, or days.
Current RDL ingestion still defaults to seconds, so its normal ingestion/query flow already agrees on units. This fixes the native temporal APIs for other index clocks; it does not add an RDL ingestion-clock option.
Problem
The temporal executor compared second-based requests directly with non-second edge timestamps. Eligible histories could disappear, the wrong rows could pass a window, and exact lengths could disagree with retrieval. It also converted decoded timestamps to seconds too early, losing subsecond distinctions before ordering and reductions.
The review exposed two additional mistakes in the first implementation:
Integer event times used the anchor's rounding rule. On an hourly clock, a 10:30 event could become 10:00 and pass a 10:15 anchor.
Returned timestamps were reconstructed from rounded/saturated clock ticks. This changed an integer event's reported time and could turn a Date value in year 9999 into a timestamp in 2262. Saturation could also create false ordering ties.
Implementation and reuse of existing rules
Anchors follow graph sampling. Select the index clock once per native query call using with_index_clock! / UnitSystem. Convert the batch's anchors once with Abs<U>::from_secs, which uses the existing truncating/saturating conversion, and reuse them across frames. Reject unsupported reader clocks before macro dispatch.
Event times follow ingestion's upward-rounding rule. Convert row times with the existing core convert_ceil helper. Integer time fields are epoch seconds. On an hourly clock, the 10:15 anchor truncates to 10:00, while the 10:30 event rounds up to 11:00 and is excluded.
Windows reuse training-table validation. Each frame converts its relative bounds with Dur<U> and calls the existing tt_builder::check_tick. Nonzero sub-tick bounds fail; representable fractional-tick durations retain the existing truncation rule. The shared helper returns Rust error text, and its existing training-table callers still map that text to DiskgraphConfigError. There is no second validation implementation. The training-table path also retains main's unbounded-lower-window sentinel handling.
Keep comparison ticks separate from row values. Index ticks determine window eligibility. Sorting uses the time column's raw values in its stored unit, preserving subsecond ordering and avoiding saturation-induced ties. Returned sequence timestamps come directly from the row's own seconds. Those seconds travel with each sortable row, avoiding a second feature decode.
Preserve precision through reductions. The private timestamp scalar retains its value/unit through grouping, distinct counts, min/max and mode. Recency subtracts timestamps using wide integer arithmetic before returning floating-point seconds, including grouped recency. Aggregates share one normalized-seconds anchor list, created only when needed.
Keep the optimized retrieval paths. Indexed selection before feature decoding, shared sequence/aggregate projections, and clipped/ambiguous-key fallbacks remain in place. Existing graph sampling code is untouched.
The interval remains (start, stop], including explicit unbounded bounds.
Existing coarse-clock anchor/duration truncation and saturating conversion rules remain. No subsecond anchor inputs, traversal/intermediate-clock changes, ingestion/index-format changes, or index rebuilds.
Ordinary numeric reductions and exact integer sums retain their behavior.
The clock implementation changes three files. This branch also carries the three-file licensing CI correction from Tighten epserde vendored-header exemptions for v1.0.1 #197. It changes no runtime behavior, dependencies, public binding signatures, RDL-package code, or repository documentation.
Sortable rows temporarily carry their original seconds in addition to their ordering value. This preserves output correctness without decoding the time column again.
Reuse Daniel's fix from #197 (upstream commit de91b1ec):
Classify the 42 copied epserde files individually under their existing Apache-2.0 OR LGPL-2.1-or-later license and recorded provenance. Their upstream headers remain intact.
Keep NVIDIA-authored src/utils/alignment.rs and tests/test_review_regressions.rs, and any future file in that directory, subject to normal header validation.
Add the missing separate rights statement to the NVIDIA-authored regression test.
Add a boundary test that prevents widening the classifications to NVIDIA-owned or future files.
The existing checker is unchanged. Local validation passes: 6,038 tracked candidate files, all 13 Python manifests, and 22 licensing tests. The clock/query runtime is unchanged by this licensing correction.
Testing Done
Refreshed on main 502adef6 in merge commit 19e8fd8e. The conflict combined main's unbounded-lower-window handling with this PR's shared validation/error mapping. The temporal-query implementation is byte-identical to the previously reviewed head; the clock implementation remains limited to three files.
Fresh release wheel: rebuilt with main's Arrow/Parquet 59 and PyO3 0.29 dependencies, including the bundled ingest binary.
Local clock/window checks: 188 passed, 32 skipped, including the existing unbounded-lower-window tests for aggregate and has_edges operations.
Local temporal selection/shape regressions: 336 passed. Together, these are 524 passed, with 32 sub-tick refusal cases skipped on clocks where one second is representable.
Native Python CI: 839 passed, 35 skipped, 1 deselected on the refreshed head.
Rust workspace and workspace/all-target Clippy: pass in CI, with warnings denied.
RDL compatibility: all 792 complete conformance checks pass on RDL main ea7c7d2 against the refreshed native wheel, executing both engines without skips.
Regression evidence: 34 tests fail against the previous PR wheel; the unsupported-clock reproduction aborts that wheel instead of returning a configuration error. Corrected cases cover integer event fields, both window endpoints, fractional-tick windows, negative coarse anchors and extreme-date ordering/metadata.
git diff --check origin/main...HEAD passes.
Measured cost against current main
This is a correctness fix with a small measured CPU cost, not a throughput improvement. Repeated the paired probes after the dependency refresh: main 502adef6 versus this PR 19e8fd8e, using fresh release wheels, the same reusable seconds index, Python interpreter and pinned CPU. Three alternating process rounds per version used eight warmups and 25 timed calls; the table reports medians of per-round medians. Build/test jobs had finished before timing.
Batch-production request
Main
This PR
Change
Bounded history
13.90 ms
13.97 ms
+0.5%
History + count/sum/recency
86.10 ms
90.01 ms
+4.5%
Alternate-column rowpack filtering
67.86 ms
71.77 ms
+5.8%
Three windows + aggregates
210.37 ms
219.63 ms
+4.4%
Eight empty windows + aggregates
3.62 ms
3.75 ms
+0.13 ms
Populated cases use 32 anchors, 4,000 events/entity and history limit 32; the empty case uses 1,024 anchors. Exact length-query medians increased by 1.1–4.2%. Median peak RSS increased by at most about 12.1 MiB in these probes.
Returned sequence values, node IDs, timestamps, offsets, aggregates, exact lengths and decoding counters match main in every measured case. Bounded histories still decode only 1,024 rows out of 128,000 visited edges, and shared aggregates retain selective decoding of sequence-only columns. Existing saved-index data was reused successfully after main's dependency updates.
Seconds indexes provide the valid performance baseline: main's temporal queries return incorrect results on other clocks. Those clocks are covered by correctness tests. These warm-cache CPU probes do not establish large/cold-index or end-to-end training performance.
Separate BigQuery CI issue
The restricted BigQuery permission tests fail because their test identities are not configured in this CI environment. This is separate from the licensing correction and clock implementation; #196 addresses test eligibility. No BigQuery code or tests change here.
Observed failure: temporal queries compared second-based requests with non-second index times and collapsed timestamp precision before selection/reduction. Review reproductions also exposed wrong integer-event rounding and sequence timestamps reconstructed from rounded/saturated clock values.
Smallest sufficient change: reuse the existing index clock conversions and training-table window validation, keep row values separate from comparison ticks, and preserve the current native decoding/selection paths. Public inputs/outputs and graph sampling remain unchanged.
Dispatch once per batch; normalize anchors once; use upward event rounding; retain stored precision through sorting/reductions and original seconds for sequence metadata; reuse the shared window check; refuse unsupported clocks. Adapt the sort test's row shape and verify metadata stays attached to its row.
Make retrieval, ordering, reductions and exact lengths agree with the established clock rules without a second feature decode or changing graph sampling.
Expose the existing tick check within the crate and return Rust error text. Existing TT callers map it to the same Python configuration exception. unit_name remains shared through that check.
Let the detached temporal executor reuse the existing rule and wording without maintaining another implementation or constructing a Python exception there. TT validation behavior stays the same, including main's unbounded-lower-window sentinel handling.
Satisfy the existing NVIDIA header rule without changing the test or upstream attribution.
scripts/license_header_policy.json
Copied epserde file classifications
Register 42 exact upstream file paths and their existing dual license/provenance; retain normal checking for NVIDIA-owned and future files.
Apply the correct ownership policy instead of requiring NVIDIA headers on copied upstream code. Reuses #197.
test/scripts/test_check_license_headers.py
test_epserde_vendor_boundary_is_exact
Verify exact paths, license classification, and continued validation of NVIDIA-owned and future files.
Prevent a broad directory exemption from hiding new unlicensed code.
Verification after merging main 502adef6: fresh release wheel; 524 focused clock/window/temporal checks passed locally, 32 intentional sub-tick skips; all 792 complete RDL conformance checks passed. Native Python CI reports 839 passed; Rust workspace and all-target Clippy pass in CI. The temporal-query source is unchanged by the merge; the sole conflict preserves main's unbounded-lower-window guard and maps shared validation errors to the existing Python exception.
Performance: fresh paired probes against main 502adef6 show 0.5–5.8% additional CPU time in populated batch construction and +0.13 ms in the empty-history probe. Exact lengths, complete outputs and decoding counters match. Median peak RSS increases by at most about 12.1 MiB. Full setup, table and limits are in the description.
Licensing CI correction: reuse #197's exact classification of copied epserde files; preserve upstream attribution and normal checks for NVIDIA-owned/future files. Header and Python manifest audits pass locally, as do 22 licensing tests. No checker/runtime/dependency or BigQuery-test behavior changes.
Stopping condition: correct clock handling and existing retrieval behavior, with no public API/index-format change or repository documentation changes. Native iterator, concurrency, ingestion-clock API options and other optimizations remain separate.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Temporal queries now convert window bounds and row timestamps using the index clock. Timestamp values retain their original units for comparisons and reductions, while Python and Arrow outputs convert timestamps to seconds. The changes add tests for clock-dependent filtering, timestamp precision, reductions, and bounds shorter than one clock tick. The license policy adds explicit exclusions for vendored epserde files and tests their boundaries.
Sequence Diagram(s)
sequenceDiagram
participant Query
participant Reader
participant FrameProcessing
participant AggregateOutput
Query->>Reader: Read timestamp unit
Query->>FrameProcessing: Dispatch with index clock and cutoff
FrameProcessing->>FrameProcessing: Convert bounds and filter rows in ticks
FrameProcessing->>AggregateOutput: Pass selected rows and converted cutoff
Loading
Merge Risk: Moderate · up to 2ffa8
Integer timestamp queries can return incomplete or incorrect results for non-second units; this should be fixed before merging.
Pre-merge checks: 2 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Change Map Design Review
The required Change map comment is unavailable. The supplied context gives only a summary of its claims, and repository inspection finds no comment beginning with <!-- kumo-pr-change-map -->; `docs/…
Provide the complete PR conversation comment with the <!-- kumo-pr-change-map --> marker, including the file-level Change map, linked problem, existing behavior, verification, and stopping condition.
Passed checks: 2 passed
Check name
Status
Explanation
Pr Size
The pull request changes 6 files. This is below the failure threshold of more than 40 files.
Public Resource Access
No added or modified external URL or package source location was found. The policy additions reference repository-local epserde paths and packages/diskgraph/third_party/source-provenance.json; these…
Full details: Public Resource Access
Explanation
No added or modified external URL or package source location was found. The policy additions reference repository-local epserde paths and packages/diskgraph/third_party/source-provenance.json; these paths exist at the reviewed head. Existing upstream URLs in the provenance file are unchanged. Test fixture paths are local and excluded by the check.
Full details: Change Map Design Review
Explanation
The required Change map comment is unavailable. The supplied context gives only a summary of its claims, and repository inspection finds no comment beginning with <!-- kumo-pr-change-map -->; docs/development/review.md confirms that the design rationale must be in that conversation comment. The code diff can show implementation behavior, but it cannot establish whether each proposed change was necessary, correctly scoped, verified, or stopped at the right point without the proposal being reviewed.
Resolution
Provide the complete PR conversation comment with the <!-- kumo-pr-change-map --> marker, including the file-level Change map, linked problem, existing behavior, verification, and stopping condition.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
253-253: Minor · Functional Correctness · Quick win
Use the same ceil_to_clock rounding in index_time.
Ingestion rounds source timestamps up with ceil_to_clock, but rowpack filtering rounds them toward zero with convert_trunc. A positive sub-tick timestamp can therefore produce different results in the indexed and rowpack paths. Replace the negative example with a positive sub-tick regression case and make index_time use the same ceil_to_clock conversion as ingestion.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
1652-1655: Minor · Stability & Availability · Quick win
Return a configuration error for non-index reader clocks.
GraphIndexReader::open accepts metadata without validating timestamp_unit. The temporal-query methods then pass that value to with_index_clock! without validation. An existing index with Minutes, Weeks, Months, Years, or another non-index unit can therefore panic through unreachable! instead of returning a configuration error.
Validate reader.metadata().timestamp_unit.is_index_clock() before either macro dispatch and return a configuration error when it is false.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
integer time columns leak future rows on hours and days indexes. index_time (temporal_query.rs:254-256) converts plain integer epoch-seconds into the clock
by rounding down. On an hours index, an event at 10:30 with an anchor at 10:15 becomes 10:00 and is returned. Main excludes it. Fix: round up with convert_ceil (as ingest does), and
add an integer-time-column test for hours and days.
Sequence timestamps are rebuilt from clock ticks (:1053-1054). A Date 9999-12-31 on ns reports 2262, and an int column on hours reports the tick. Report the row's own seconds as main does, and use ticks only for comparisons.
Reuse tt_builder.rs::check_tick / unit_name for the sub-tick error instead of a second copy.
Tests: integer time columns, a sub-tick stop, a window that isn't a whole number of ticks (e.g. 5400 s on hours), and negative anchors on hours/days.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Major · Round parsed sub-second event times up on hour and day indexes. · access.rs:2575-2583
packages/diskgraph/diskgraph-rowpack/src/access.rs:2575-2583 Major · Functional Correctness · Quick win
Round parsed sub-second event times up on hour and day indexes.
On hour and day indexes, floor_parts::<U> at access.rs:2575-2583 discards the fraction before the event is stored. An event at 01:00:00.5 is stored at tick 01:00. A query with stop 01:00 then includes it, even though the event is after stop. The same truncation can drop events. An event at 00:00:00.5 is stored at 00:00, so (00:00, 01:00] excludes it, although the event belongs in that window.
The PR contract rounds event times upward with the index clock. Round the fractional part up to the next index tick here. Keep exact ticks unchanged. Apply the same correction to both the rowpack and sort-key paths, because both read this value.
Indexes built before this change keep floored ticks. Rebuild them after the fix.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Major · Preserve the configured unit for integer timestamp columns. · temporal_query.rs:524-531
packages/diskgraph/diskgraph-pybind/src/temporal_query.rs:524-531 Major · Functional Correctness · Heavy lift
Preserve the configured unit for integer timestamp columns.
ColumnSpec supports integer timestamps with a separate timestamp_unit, and those columns decode as Arrow Int64. scalar_at drops that unit, so index_time::<U>() treats a millisecond, microsecond, or nanosecond value as seconds. materialize_frame can then exclude valid rows. count_frame can also disagree when it uses the matching temporal sort key.
Use the unit-aware rowpack timestamp conversion for the time column, or preserve its configured unit before calling index_time::<U>(). Apply the same conversion in both bounded query paths.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Thanks, the clock work is the right direction. I built the PR and main and reproduced a few correctness issues:
Pre-1970 anchors leak future events on coarse clocks.Abs::<U>::from_secs truncates toward zero while events ceil. Hours clock, anchor -6300 s, events at -9000/-7200/-5400/-3600 s: the PR returns all four; -5400 and -3600 are after the anchor. Floor the anchors; test_integer_times_follow_ingest_rounding[negative] currently expects the leaking result.
Nanosecond clock saturation.convert_ceil saturates, so an anchor in 2300 and an event in 3000 both become i64::MAX and the event passes. Please use checked conversion and reject out-of-range anchors, as ingestion does with ceil_to_clock.
Recency uses the rounded anchor. On an hours clock with the anchor 4000 s after an event, recency is 3600; on a nanosecond clock with the anchor in 2300 it goes negative. Recency should use the requested cutoff seconds, with the rounded anchor used only for filtering.
Non-tick window bounds are silently shortened. On an hours clock, start_seconds=-5400 becomes -1 h. Please reject bounds that aren't a whole number of ticks, or round the lower bound outward.
count_distinct/mode on timestamps now group by nanosecond value but output floored seconds. Is that intended? If so, please state it in the PR body.
Deferring this implementation until main's time-granularity behavior is stable. Follow-up clock support for the temporal query paths is tracked in #220; a separate small PR will reject non-seconds indexes in the meantime.
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
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.
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Make
query_temporalandquery_temporal_lengthsfollow the index clocks introduced by #123. Request anchors and relative bounds remain epoch seconds, while an index can store nanoseconds, microseconds, milliseconds, seconds, hours, or days.Current RDL ingestion still defaults to seconds, so its normal ingestion/query flow already agrees on units. This fixes the native temporal APIs for other index clocks; it does not add an RDL ingestion-clock option.
Problem
The temporal executor compared second-based requests directly with non-second edge timestamps. Eligible histories could disappear, the wrong rows could pass a window, and exact lengths could disagree with retrieval. It also converted decoded timestamps to seconds too early, losing subsecond distinctions before ordering and reductions.
The review exposed two additional mistakes in the first implementation:
Implementation and reuse of existing rules
with_index_clock!/UnitSystem. Convert the batch's anchors once withAbs<U>::from_secs, which uses the existing truncating/saturating conversion, and reuse them across frames. Reject unsupported reader clocks before macro dispatch.convert_ceilhelper. Integer time fields are epoch seconds. On an hourly clock, the 10:15 anchor truncates to 10:00, while the 10:30 event rounds up to 11:00 and is excluded.Dur<U>and calls the existingtt_builder::check_tick. Nonzero sub-tick bounds fail; representable fractional-tick durations retain the existing truncation rule. The shared helper returns Rust error text, and its existing training-table callers still map that text toDiskgraphConfigError. There is no second validation implementation. The training-table path also retains main's unbounded-lower-window sentinel handling.The interval remains
(start, stop], including explicit unbounded bounds.Scope and tradeoffs
Change map
Per-file rationale and verification.
Licensing CI correction
Reuse Daniel's fix from #197 (upstream commit
de91b1ec):Apache-2.0 OR LGPL-2.1-or-laterlicense and recorded provenance. Their upstream headers remain intact.src/utils/alignment.rsandtests/test_review_regressions.rs, and any future file in that directory, subject to normal header validation.The existing checker is unchanged. Local validation passes: 6,038 tracked candidate files, all 13 Python manifests, and 22 licensing tests. The clock/query runtime is unchanged by this licensing correction.
Testing Done
Refreshed on main
502adef6in merge commit19e8fd8e. The conflict combined main's unbounded-lower-window handling with this PR's shared validation/error mapping. The temporal-query implementation is byte-identical to the previously reviewed head; the clock implementation remains limited to three files.has_edgesoperations.ea7c7d2against the refreshed native wheel, executing both engines without skips.git diff --check origin/main...HEADpasses.Measured cost against current main
This is a correctness fix with a small measured CPU cost, not a throughput improvement. Repeated the paired probes after the dependency refresh: main
502adef6versus this PR19e8fd8e, using fresh release wheels, the same reusable seconds index, Python interpreter and pinned CPU. Three alternating process rounds per version used eight warmups and 25 timed calls; the table reports medians of per-round medians. Build/test jobs had finished before timing.Populated cases use 32 anchors, 4,000 events/entity and history limit 32; the empty case uses 1,024 anchors. Exact length-query medians increased by 1.1–4.2%. Median peak RSS increased by at most about 12.1 MiB in these probes.
Returned sequence values, node IDs, timestamps, offsets, aggregates, exact lengths and decoding counters match main in every measured case. Bounded histories still decode only 1,024 rows out of 128,000 visited edges, and shared aggregates retain selective decoding of sequence-only columns. Existing saved-index data was reused successfully after main's dependency updates.
Seconds indexes provide the valid performance baseline: main's temporal queries return incorrect results on other clocks. Those clocks are covered by correctness tests. These warm-cache CPU probes do not establish large/cold-index or end-to-end training performance.
Separate BigQuery CI issue
The restricted BigQuery permission tests fail because their test identities are not configured in this CI environment. This is separate from the licensing correction and clock implementation; #196 addresses test eligibility. No BigQuery code or tests change here.