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
Read integer time columns in their declared unit in temporal queries - #205
Temporal queries (SamplerEngine.query_temporal and query_temporal_lengths) read an integer time column as epoch seconds, even when the graph config declares it in another unit (timestamp_unit: millis, micros, nanos) or as a date format (timestamp_format, e.g. %Y%m%d). Ingestion honours those hints, including when it writes the edge sort keys, so the two halves of a query disagreed.
Example: a seconds clock, an int64 event_time declared as milliseconds, and an edge sort key on that column. For a window holding two events, query_temporal_lengths returned [2] (from the sort key) while query_temporal returned no rows (the rowpack check compared raw milliseconds with a seconds cutoff). Without the sort key both returned nothing, and returned timestamps were 1000 times too large.
The fix:
When SamplerEngine opens an index, it reads the hints of integer columns declared with a time unit or format from the persisted diskgraph_config.json. Only those columns' time fields are parsed strictly, so unrelated config fields from another version are ignored. Indexes without the file keep reading epoch seconds; a file that cannot be read makes temporal queries fail with DiskgraphConfigError (sampling is unaffected).
The temporal query decodes those columns through the same RowAccess timestamp path ingestion uses (bp_col_stats::row_access_hints, RowAccess::get_timestamp). This covers the window filter, the rowpack fallback in lengths, sort order, returned timestamps and the recency reduction.
Group stages carry the hint forward for keys and for min, max and mode outputs, so recency after a group reads them correctly too. Those reductions and projected columns still return the stored values.
Breaking changes: none. Results change only for integer time columns declared in a unit other than seconds or with a format, which were wrong before.
Not changed here: the sort key window on index clocks other than seconds is also unchanged; #165 covers that.
This is independent of #165, which rewrites much of temporal_query.rs for non-second index clocks. The diff here is kept small: a hint lookup plus one decoding helper used at the three places that read time. If both land, whichever merges second rebases; the conflicts are limited to those call sites.
int64 and uint64 time in seconds, millis, micros, nanos and %Y%m%d, each with and without the column as the sort key. It asserts the retrieved rows, the lengths, which path the lengths used, the returned timestamps and recency.
millisecond values 400 ms either side of both window bounds, on both paths, rounding up as ingestion does.
recency over a hinted group key, over one and two stages of max, and after a key is overwritten by a count; max returns the stored value.
an unreadable config fails the query; unknown config fields outside integer time columns are ignored.
On main every case with a non-second unit or format fails; all pass with the fix.
Existing temporal and clock suites (test_temporal_query_shapes.py, test_temporal_sequence_limit.py, test_temporal_tie_order.py, test_index_clocks.py) pass, 389 tests including the new ones.
cargo test -p diskgraph-pybind --lib temporal_query and cargo clippy -p diskgraph-pybind --all-targets -- -D warnings pass.
Hotfix Context
Not a hotfix or backport.
Checklist
I listed all the breaking changes introduced by the PR: none.
I made sure to update examples and documentation with my changes: diskgraph-pybind/README.md states how integer time columns are read.
I have added and modified unit tests required to cover these changes.
An integer time column ingested with a timestamp_unit or timestamp_format
was read as epoch seconds by the temporal query, while its edge sort key
used the declared unit. Decode it with the same RowAccess path ingestion
uses, for the window filter, lengths, returned timestamps and recency.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
recency over a group key, or over a min, max or mode of a hinted integer
column, read the raw integer as seconds after a group stage.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Observed failure: an integer time column declared with timestamp_unit (millis, micros, nanos) or timestamp_format was read as epoch seconds by query_temporal and query_temporal_lengths. Its edge sort key used the declared unit, so lengths and retrieval disagreed ([2] against no rows), and returned timestamps were 1000 times too large.
Existing behavior: ingestion and sort keys decode these columns with RowAccess::timestamp_in. The temporal query decoded rowpack time values with scalar_at, which knows only Arrow timestamp and date types.
Smallest sufficient change: load the column hints from the persisted graph config when SamplerEngine opens (parsing only integer columns' time fields; an unreadable file fails temporal queries instead of silently reading seconds) and decode hinted integer columns with the same RowAccess path wherever the query reads time: window filter, lengths fallback, returned timestamps and recency (carried through group stages).
Focused verification: new tests over five units/formats on both the sort key and rowpack paths, sub-second bounds, and grouped recency; they fail on main and pass here. Existing temporal and clock suites, the temporal_query Rust tests and clippy pass.
Stopping condition: both paths agree with ingestion for hinted integer columns on a seconds clock. Non-second index clocks (#165) and indexes without a persisted config are unchanged.
Load hints of integer time columns (lenient except for those columns; unreadable config is an error) and decode them through RowAccess; pass hints to the time filter, lengths fallback, returned timestamps and recency, and carry them through group stages.
Read integer times in the unit ingestion and the sort keys used.
Reviewing files that changed from the base of the PR and between 5ce51c6 and c624a90.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
Walkthrough
Temporal queries now use persisted timestamp units or formats to interpret integer time columns during frame selection and bounded length checks. The same hints apply to recency reductions and propagate through group keys and min, max, and mode results. Missing configuration yields empty hints. Configuration read or parse errors cause temporal queries to fail. Tests cover timestamp declarations, storage paths, boundary rounding, grouped reductions, and configuration handling.
Sequence Diagram(s)
sequenceDiagram
participant GraphIndexReader
participant SamplerEngine
participant TemporalQuery
participant FrameMaterialization
GraphIndexReader->>SamplerEngine: Provide timestamp declarations or configuration error
SamplerEngine->>TemporalQuery: Pass time hints to temporal execution
TemporalQuery->>FrameMaterialization: Decode frame timestamps with column hints
Loading
Merge Risk: Moderate · previous review 5ce51
A timestamp column with an invalid persisted dtype can produce incorrect temporal results. Reject that configuration before merging.
Pre-merge checks: 3
Check name
Status
Explanation
Change Map Design Review
PASS — The supplied change-map summary identifies the observed rowpack-versus-sort-key failure, the existing ingestion timestamp path, the focused fix, verification, and scope boundary. The diff suppo…
Pr Size
The pull request changes 6 files. The threshold is 40 files, so the PR passes this check.
Public Resource Access
The pull request adds or modifies no external URL, repository location, or package source location. The only resource-like reference added is the local runtime file diskgraph_config.json; it is not …
Full details: Change Map Design Review
Explanation
PASS — The supplied change-map summary identifies the observed rowpack-versus-sort-key failure, the existing ingestion timestamp path, the focused fix, verification, and scope boundary. The diff supports that design: GraphIndexReader reads persisted time-column metadata once; SamplerEngine caches hint errors and passes hints only to temporal queries; temporal_query.rs reuses row_access_hints for frame filtering, lengths, recency, and grouped min/max/mode outputs. The malformed-config path now propagates an error, while absent configs retain legacy behavior. The added tests cover units, formats, signed and unsigned integers, both query paths, grouping, rounding, and config failures. The change stays within the temporal-query and config-reader layers and does not alter unrelated reductions or index-clock behavior.
Full details: Public Resource Access
Explanation
The pull request adds or modifies no external URL, repository location, or package source location. The only resource-like reference added is the local runtime file diskgraph_config.json; it is not an external resource.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Falling back to epoch seconds would silently return wrong rows for
hinted integer time columns. Only hinted columns are parsed strictly,
so config fields from another version fail nothing else, and sampling
still opens the index.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A recency over a hinted integer column built a new reader for every
anchor and every group. Build one per reduction when the aggregate is
planned and reuse it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SamplerEngine parsed diskgraph_config.json a second time at open for
temporal time hints. The index reader already parses it for semantic
types, so it now also keeps the columns that declare a timestamp_unit
or timestamp_format, and an unreadable config still fails temporal
queries only.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Duplicate tables and the first reported config error then follow the
file, as before the single parse.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On the seconds clock a timestamp_unit of seconds reads every value
unchanged, yet it sent recency through the RowAccess decoder per anchor:
115 ms against 63 ms on main for 4,096 anchors. Keep hints only for a
format or a finer unit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 · Reject an unknown dtype on a timestamp-declared column. · temporal_query.rs:563-580
packages/diskgraph/diskgraph-pybind/src/temporal_query.rs:563-580 Major · Data Integrity & Integration · Quick win
Reject an unknown dtype on a timestamp-declared column.
If ingested_dtype contains an unknown value, this filter skips the column instead of returning a config error. The graph parser has already retained the timestamp declaration. A query can then read that column’s integer values as epoch seconds and return incorrect temporal results. Return a parse error for an invalid dtype; continue to skip valid non-integer dtypes. This is another path to the malformed-config outcome flagged in the previous review. (raw.githubusercontent.com)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
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
Temporal queries (
SamplerEngine.query_temporalandquery_temporal_lengths) read an integer time column as epoch seconds, even when the graph config declares it in another unit (timestamp_unit: millis, micros, nanos) or as a date format (timestamp_format, e.g.%Y%m%d). Ingestion honours those hints, including when it writes the edge sort keys, so the two halves of a query disagreed.Example: a seconds clock, an int64
event_timedeclared as milliseconds, and an edge sort key on that column. For a window holding two events,query_temporal_lengthsreturned[2](from the sort key) whilequery_temporalreturned no rows (the rowpack check compared raw milliseconds with a seconds cutoff). Without the sort key both returned nothing, and returned timestamps were 1000 times too large.The fix:
SamplerEngineopens an index, it reads the hints of integer columns declared with a time unit or format from the persisteddiskgraph_config.json. Only those columns' time fields are parsed strictly, so unrelated config fields from another version are ignored. Indexes without the file keep reading epoch seconds; a file that cannot be read makes temporal queries fail withDiskgraphConfigError(sampling is unaffected).RowAccesstimestamp path ingestion uses (bp_col_stats::row_access_hints,RowAccess::get_timestamp). This covers the window filter, the rowpack fallback in lengths, sort order, returned timestamps and therecencyreduction.min,maxandmodeoutputs, sorecencyafter a group reads them correctly too. Those reductions and projected columns still return the stored values.Breaking changes: none. Results change only for integer time columns declared in a unit other than seconds or with a format, which were wrong before.
Not changed here: the sort key window on index clocks other than seconds is also unchanged; #165 covers that.
This is independent of #165, which rewrites much of
temporal_query.rsfor non-second index clocks. The diff here is kept small: a hint lookup plus one decoding helper used at the three places that read time. If both land, whichever merges second rebases; the conflicts are limited to those call sites.Change map
#205 (comment)
Testing Done
test_temporal_int_time_units.py:%Y%m%d, each with and without the column as the sort key. It asserts the retrieved rows, the lengths, which path the lengths used, the returned timestamps andrecency.recencyover a hinted group key, over one and two stages ofmax, and after a key is overwritten by acount;maxreturns the stored value.test_temporal_query_shapes.py,test_temporal_sequence_limit.py,test_temporal_tie_order.py,test_index_clocks.py) pass, 389 tests including the new ones.cargo test -p diskgraph-pybind --lib temporal_queryandcargo clippy -p diskgraph-pybind --all-targets -- -D warningspass.Hotfix Context
Not a hotfix or backport.
Checklist
diskgraph-pybind/README.mdstates how integer time columns are read.🤖 Generated with Claude Code