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
Reuse TT numeric reductions in temporal row queries - #279
Observed requirement and smallest sufficient change: PQL and temporal row queries currently repeat numeric traversal/reduction logic. Reuse resolved TT inputs and numeric accumulation for eligible temporal aggregates, preserving each frontend’s row order, empty/NULL behavior and conservative fallbacks.
Focused verification / stopping condition: Release native build; 44 focused compiler/numeric tests passed at this checkpoint. Stop at this slice’s verified behavior; fitting redesign, new temporal/LP capabilities and live sweeps are separate.
File
Function / scope
What changed
Why
packages/diskgraph/diskgraph-pybind/README.md
API and workflow documentation
Describe supported routes, preparation boundaries and explicit limitations
Keep usage and performance claims aligned with the implementation
Exercise 600 real node reads across the 512-row decoder boundary
Cover the PQL Nodes route independently of projected temporal batches
Verified repair — 2026-11-11
Head: e891b9e9e9fc3e359acc8cd64bc9d212fa26ab5e. Declined numeric groups now fall back once per operation; independent signed-zero/nonfinite checks and a 600-node native decode regression.
Validation: repaired combined stack passed 146 Rust TT integration tests and 579 Python native/conformance tests with a freshly rebuilt extension. Public Python suite: 1,951 passed; optional-native import checks: two modules skipped as expected without DiskGraph. Generated artifact manifest and 403-component notice checks passed. No live sweeps or merges.
The change adds grouped numeric execution for compatible PQL aggregates and eligible aggregate-only temporal queries. It shares aggregate input resolution and accumulator behavior across execution paths. Temporal queries use the grouped path for supported one-hop, second-based numeric operations and retain the existing evaluator when eligibility checks fail or grouped execution returns no result. New tests compare results across numeric edge cases and verify temporal window behavior.
Sequence Diagram(s)
sequenceDiagram
participant TemporalQuery
participant resolve_named_path
participant execute_numeric_group
participant accumulate_numeric_batch
TemporalQuery->>resolve_named_path: Resolve frame relation path
resolve_named_path-->>TemporalQuery: Return resolved edges
TemporalQuery->>execute_numeric_group: Submit eligible operations and projected rows
execute_numeric_group->>accumulate_numeric_batch: Accumulate selected numeric columns
accumulate_numeric_batch-->>execute_numeric_group: Return accumulator status
execute_numeric_group-->>TemporalQuery: Return aggregate results or no grouped result
Loading
Merge Risk: Low · up to e891b
The numeric results have no established production defect, but the signed-zero test could miss a regression in the shared path. Add a route assertion for confidence; the remaining risk is bounded.
Security architecture risk: Low · up to e891b
The shared execution path retains conservative eligibility checks and isolates incomplete results before falling back. No introduced security vulnerability was established, but incomplete coverage of downstream consumers and authorization prevents a minimal-risk assessment.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The confirmed propagation surface is query execution in the bindings and train-table library. Caller-supplied temporal plans reach the shared reducer through the existing API; tenant, service, or environment-wide collateral exposure cannot be established from the supplied deployment and authorization context.
Trust Boundaries and Controls
observed — The optimized temporal path still uses frame materialization that checks decoded timestamps against exact bounds and preserves established ordering. Shared execution eligibility does not replace these row-selection controls.
Resilience and Maintainability Implications
inferred — Accumulator and result ownership is local to each execution, containing partial failures without cross-call numeric state. The 512-row decode batches do not bound total selected-row memory, but the existing traversal already materialized an edge vector; the evidence does not establish a newly introduced unbounded-memory security condition.
Pre-merge checks: 2 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Change Map Design Review
The full marked change-map comment and its linked problem are unavailable. The supplied summary cannot establish whether the same comment explains the observed failure or release requirement, necessit…
Provide the full comment starting with <!-- kumo-pr-change-map --> and the linked problem context. Review the design against both and the repository code.
Passed checks: 2 passed
Check name
Status
Explanation
Pr Size
The PR changes 8 files. The limit is 40 files.
Public Resource Access
The changed lines add no external resource references. The added file paths are local test fixtures.
Full details: Change Map Design Review
Explanation
The full marked change-map comment and its linked problem are unavailable. The supplied summary cannot establish whether the same comment explains the observed failure or release requirement, necessity, and stopping condition. The diff has 1,036 additions and deletions, so file-level scope descriptions are permitted. The code shows shared numeric execution and fallback paths, but that does not establish the proposed design’s necessity.
Resolution
Provide the full comment starting with <!-- kumo-pr-change-map --> and the linked problem context. Review the design against both and the repository code.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
2744-2770: Minor · Performance & Scalability · Quick win
Run the whole group per operation when the grouped attempt declines.
execute_numeric_group can return None after a full traversal and decode. This happens when accumulate_numeric_batch finds a non-finite value. The loop then handles only stage.ops[position] with the per-operation path and advances position by one. On the next iteration, map_while builds the group again from the remaining suffix. If that suffix has more than one operation, the grouped path traverses and decodes the same rows again, and it fails the same way.
Example: a stage has k compatible Sum operations and the entity has one inf value. That entity and timestamp then costs k−1 failed grouped traversals plus k per-operation traversals. The cost is quadratic in the group size, on every row that has a non-finite value.
The loop also allocates a groupVec for every Aggregate operation on every row, including single-operation groups.
When the grouped attempt declines, evaluate all members of group with execute_aggregate and then skip past the group.
♻️ Proposed fix
- if group.len() > 1 {+ if group.len() > 1 {
let cutoff = observation_ts.unwrap_or(Abs::from_raw(0));
if let Ok(Some(values)) = execute_numeric_group(&group, &[entity_node], &[cutoff],
...
}) {
for (aggregate, mut column) in group.iter().zip(values) {
aliases[aggregate.alias_idx] = column.pop().unwrap();
}
- position += group.len();- continue;+ } else {+ for aggregate in &group {+ aliases[aggregate.alias_idx] = execute_aggregate(+ plan, entity_node, aggregate, entity_cols, observation_ts,+ link_frequencies, link_split_values,+ );+ }
}
+ position += group.len();+ continue;
}
The signed-zero check never reaches the shared path.
The test queries roots [3, 4] in one batch. accumulate_numeric_batch returns false when it finds the inf value of user 3. execute_numeric_group then returns None for the whole batch. The general evaluator therefore computes both the actual and reference results.
As a result, the shared reducers never see the -0.0/0.0 rows of user 4. A signed-zero regression in observe_numeric_row with keep_first_equal would pass this test. Query user 4 by itself so that the shared path runs for it.
💚 Proposed fix
def test_nonfinite_and_signed_zero_keep_general_numeric_policy(numeric_graph):
- actual = query(numeric_graph, [3, 4], operations=("sum", "mean", "min", "max"))- reference = query(numeric_graph, [3, 4], general=True, operations=("sum", "mean", "min", "max"))- for name in actual:- for left, right in zip(actual[name].to_pylist(), reference[name].to_pylist()):- assert left == right or math.isnan(left) and math.isnan(right), name- if left == 0.0:- assert math.copysign(1, left) == math.copysign(1, right), name+ operations = ("sum", "mean", "min", "max")+ # Separate batches: a nonfinite anchor makes the whole batch fall back.+ for keys in ([3], [4]):+ actual = query(numeric_graph, keys, operations=operations)+ reference = query(numeric_graph, keys, general=True, operations=operations)+ for name in actual:+ for left, right in zip(actual[name].to_pylist(), reference[name].to_pylist()):+ assert left == right or math.isnan(left) and math.isnan(right), name+ if left == 0.0:+ assert math.copysign(1, left) == math.copysign(1, right), name
The existing grouped PQL tests use a small fixture. The 600-row Python test uses NumericRows::Projected, so it does not test the 512-row NumericRows::Nodes chunks.
Add Rust tests that compare grouped and per-operation PQL results for more than 512 rows, nulls, non-finite fallback, and window bounds. Check that the grouped path runs rather than silently falling back.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Key 3 tests the non-finite fallback. Key 4 already provides finite signed zeros, but the test can pass if shared execution declines and both queries use the general evaluator. Add a route assertion for key 4.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 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
PQL and temporal row queries currently repeat numeric traversal/reduction logic. Reuse resolved TT inputs and numeric accumulation for eligible temporal aggregates, preserving each frontend’s row order, empty/NULL behavior and conservative fallbacks.
Stack slice 1/5. Targets main; subsequent slices add common IR, prepared execution, native batch consumption and the public materializer.
Change map
#279 (comment)
Testing
Release native build; 44 focused compiler/numeric tests passed at this checkpoint.
Dependency order: #279 → #280 → #281 → #282 → #283.