Skip to content

Fix unbounded clock windows and snapshot test fixtures - #163

Open
Victsm wants to merge 1 commit into
mainfrom
codex/fix-index-clock-regressions
Open

Victsm wants to merge 1 commit into
mainfrom
codex/fix-index-clock-regressions

Conversation

@Victsm

@Victsm Victsm commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #123.

Summary

Unbounded past windows such as SUM(ORDERS.ORDER_SCORE, -INF, 0) fail the new clock validation because the compiler's -2**60-day sentinel overflows to zero when converted to seconds. Saturate that conversion consistently with the other clock conversions, retaining rejection of genuinely sub-tick windows.

Update the snapshot-ingestion test doubles with timestamp_precision='seconds', matching the real DiskgraphIngestArgs default. No production ingestion fallback or API change is added.

Change map

Maintained change map.

Testing Done

  • Native release extension/ingest build passed.
  • Rust time tests: 73 passed. The new regression fails on the base with zero instead of i64::MIN.
  • Python index-clock suite against the rebuilt extension: 33 passed, including 12 unbounded day/week cases and existing sub-tick rejection cases.
  • Seven selected snapshot-ingestion tests: 7 failed before the fixture correction; 7 passed after it. The isolated runner binds this checkout and excludes ancestor service setup.
  • Changed snapshot-test file: flake8 and isort checks passed.

Limits: the two original high-level PQL tests were attempted with ancestor setup excluded and stopped at missing fixture column statistics (AGE: COUNT/NUM_NA), before reaching the query path. They remain for normal CI qualification. Whole-file rustfmt reports existing formatting differences outside the changed lines; no unrelated formatting was included.

The baseline duration regression fails with 0 instead of i64::MIN; all seven selected snapshot cases fail before the fixture correction. No GPU/cloud jobs or live sweeps were run.

Checklist

  • Breaking changes: none intended; restores unbounded past-window validation.
  • No documentation change needed: existing query/default-clock behavior is preserved.
  • Focused regression tests added and updated.

@Victsm

Victsm commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator Author

Change map

File Function / scope What changed Why
packages/diskgraph/diskgraph-core/src/time.rs Seconds::dur_from_days; duration regression Use saturating multiplication and test positive/negative overflow The existing unbounded PQL sentinel wraps to zero; #123's tick validation then rejects a valid window. Other clock conversions already saturate.
packages/diskgraph/diskgraph-pybind/python/tests/test_index_clocks.py test_unbounded_past_windows_are_not_rejected_as_sub_tick Exercise day/week unbounded past windows on all six index clocks Guard the Python builder boundary while retaining the existing sub-tick rejection tests.
test/pool/test_activities.py Snapshot-ingestion SimpleNamespace fixtures Supply the real argument type's timestamp_precision='seconds' default #123's worker reads the new field; incomplete test doubles currently fail before exercising ingestion/recovery behavior.

Observed failures: unbounded-window validation raises “shorter than one tick”; snapshot fixture cases raise missing timestamp_precision or time out waiting for the skipped ingest call. The duration multiplication predates #123; its new validation exposes the overflow. Keep the production clock validation and transport dataclass unchanged. Stop after focused duration, Python clock-boundary, and snapshot-fixture verification; no additional clock features, validation framework, or workflow changes.

@copy-pr-bot

copy-pr-bot Bot commented Nov 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Victsm
Victsm deployed to gha-kumo-workflow-tests November 3, 2026 00:25 — with GitHub Actions Active
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 9ac307d and f17b5d7.

Files selected for processing: 3
  • packages/diskgraph/diskgraph-core/src/time.rs
  • packages/diskgraph/diskgraph-pybind/python/tests/test_index_clocks.py
  • test/pool/test_activities.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

Day-to-second conversion now saturates at the i64 limits when multiplication overflows. Tests cover extreme day values and temporal windows across configured clocks and window units. Seven snapshot-ingest test fixtures now set timestamp precision to seconds.

Merge Risk: Minimal · up to f17b5

No actionable merge risk remains in the reviewed changes.

Pre-merge checks: Passed 2 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Change Map Design Review Inconclusive The required conversation comment beginning <!-- kumo-pr-change-map --> is not present in the supplied PR conversation or repository checkout, so the comment-specific design claims (observed failure… Provide the full change-map conversation comment, including its linked problem, per-change rationale, inspected behavior, verification evidence, and stopping condition. Then reassess the design against the code diff.
Passed Passed checks: 2 passed
Check name Status Explanation
Pr Size Passed The pull request changes 3 files. The limit is 40 files, so the pull request passes this check.
Public Resource Access Passed The pull request adds no external URL, repository location, or package source location. Added lines only change Rust arithmetic, add Python regression-test logic, and set `timestamp_precision='seconds…
Full details: Public Resource Access

Explanation

The pull request adds no external URL, repository location, or package source location. Added lines only change Rust arithmetic, add Python regression-test logic, and set timestamp_precision='seconds' in test fixtures. The s3:// test fixture reference is unchanged and is excluded by the check.

Full details: Change Map Design Review

Explanation

The required conversation comment beginning &lt;!-- kumo-pr-change-map --&gt; is not present in the supplied PR conversation or repository checkout, so the comment-specific design claims (observed failure, stopping condition, and focused verification) cannot be assessed from the required source.

Resolution

Provide the full change-map conversation comment, including its linked problem, per-change rationale, inspected behavior, verification evidence, and stopping condition. Then reassess the design against the code diff.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

This branch was successfully deployed

1 active deployment
gha-kumo-workflow-tests — f17b5d7c Deployed Nov 3, 2026 by Victsm via import #1305
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant