Skip to content

fix(gfql/cypher): preserve openCypher Duration components through toString + equality (#1361) - #1364

Merged
lmeyerov merged 1 commit into
masterfrom
issue-1361-temporal-wrong-rows
May 9, 2026
Merged

lmeyerov merged 1 commit into
masterfrom
issue-1361-temporal-wrong-rows

Conversation

@lmeyerov

@lmeyerov lmeyerov commented May 9, 2026

Copy link
Copy Markdown
Contributor

Summary

_normalize_duration_map (graphistry/compute/gfql/temporal_text.py) now keeps the three openCypher CIP Duration component groups (months, days, seconds+nanoseconds) separate through canonicalization instead of collapsing days into total nanoseconds.

Behavior changes (canonical strings)

  • duration({years: 12, months: 5, days: -14, hours: 16}) → P12Y5M-14DT16H (was P12Y5M-13DT-8H)
  • duration({days: 1, milliseconds: -1}) → P1DT-0.001S (was PT23H59M59.999S)
  • duration({...}) = duration({...}) now returns false for two durations with equal totals but different (days, seconds) component shapes (was incorrectly true because both sides normalized to the same total-ns value).

Why

openCypher CIP Duration: months, days, seconds+nanoseconds are kept as separate components — months have variable length (28-31 days), and days have variable length under DST. Within a duration, fractional larger units cascade into the next group only (fractional months → days × 30.436875, fractional days → seconds × 86400); whole-unit values do not cascade.

Pygraphistry was previously merging everything into total_nanoseconds and decomposing back via _format_signed_day_time_duration, which lost the days-vs-subday distinction and produced incorrect canonicals for negatives crossing component boundaries.

Scope

Closes 3 of 14 wrong-row cases under #1361 / #1353 item #2:

  • expr-temporal6-6-2 — toString({years: 12, months: 5, days: -14, hours: 16})
  • expr-temporal6-6-8 — toString({days: 1, milliseconds: -1})
  • expr-temporal7-6-8 — equality of two durations with same total seconds but different days+seconds shape

The remaining 11 are tck-gfql repo issues, filed and deferred:

  • 10 cases: tck-gfql#38 — port <gt> placeholder substitution gap in Temporal7.py
  • 1 case (expr-temporal2-6-5): niche historical TZ for Stockholm 1818 LMT — out of scope.

Test plan

  • All 25 existing duration tests in test_lowering.py pass.
  • 3 new behavioral tests added at test_lowering.py:4541+ covering the three Bucket C scenarios.
  • Full graphistry/tests/compute/gfql/ = 1900 passed, 117 skipped, 15 xfailed — no regressions.
  • ./bin/ruff.sh graphistry/compute/gfql/temporal_text.py graphistry/tests/compute/gfql/cypher/test_lowering.py clean.
  • CI green
  • cuDF dual-version validation on dgx-spark — duration canonicalization is pure-python text formatting (no engine-polymorphic dataframe ops); evaluating whether validation is required or can be skipped.

Cross-reference

…tring + equality (#1361)

`_normalize_duration_map` (graphistry/compute/gfql/temporal_text.py) now
keeps the three openCypher CIP `Duration` component groups (months,
days, seconds+nanoseconds) separate through canonicalization instead of
collapsing days into total nanoseconds.

Behavior changes (canonical strings):
- `duration({years: 12, months: 5, days: -14, hours: 16})` → `P12Y5M-14DT16H` (was `P12Y5M-13DT-8H`)
- `duration({days: 1, milliseconds: -1})`               → `P1DT-0.001S`     (was `PT23H59M59.999S`)
- `duration({...}) = duration({...})` returns `false` for two durations
  with equal totals but different `(days, seconds)` component shapes
  (was incorrectly `true` due to total-ns equivalence).

Closes 3 of the 14 wrong-row cases under #1361 / #1353 item #2:
expr-temporal6-6-2, expr-temporal6-6-8, expr-temporal7-6-8. The
remaining 11 are tck-gfql port issues (tck-gfql#36, tck-gfql#38)
deferred to that repo. The single Stockholm-1818 historical TZ case
(expr-temporal2-6-5) is also out of scope here.

All 25 prior duration tests + 3 new behavioral tests pass; full
`graphistry/tests/compute/gfql/` = 1900 passed, no regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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