feat(round_temporal): implement Substrait round_temporal/round_calendar (item 74) - #292
Merged
Merged
Conversation
…s (item 74) Real Substrait shared 9-unit domain (YEAR..MICROSECOND) confirmed against functions_datetime.yaml; options=(rounding, unit, multiple) -- origin excluded (real Substrait channel is arguments, not options; see spec §3.4). Drains the two protocol-alignment KNOWN_ASPIRATIONAL park entries now that the ops are wired into the function registry (Task 2-6 still needed to fix the pre-existing buggy protocol signatures and implement real backend bodies).
…ared unit/rounding domains (item 74) round_calendar's parameter order previously diverged from round_temporal's (origin before multiple vs after); both had meaningless non-None defaults for rounding/unit despite being required. Fixed both to a consistent signature with required rounding/unit and Any-typed origin (v1 never supplies non-None -- excluded from options per spec §3.4). Backend bodies (polars/ibis/narwhals) and the API builder are not yet updated -- expected RED at this checkpoint (Tasks 3-6).
… unwired-ops park (item 74 Task 3) - add round_temporal/round_calendar to SubstraitScalarDatetimeAPIBuilder and its protocol stub; rounding/unit validated against the closed domains, multiple must be a positive int, origin always raises (v1 scope, not a string option per real Substrait) - fix origin annotation: Optional[Any] -> Any (Optional[Any] is redundant and was misclassified 'unclassified' by the option/argument taxonomy walker; bare Any is the existing convention for option-kind Any params) - wire the x argument channel: two new OP_SPEC entries mirror to_timezone/ local_timestamp's pattern (fixed rounding+unit options, x varies raw/lit/ col/complex) -- these now exercise REAL polars/narwhals compiles, which correctly RED (24 cases) since Tasks 4/6 haven't replaced the stub bodies yet; ibis passes already (round_temporal.x/round_calendar.x OpSpecs use uppercase Substrait unit names, which ibis truncate() accepts natively) - drain KNOWN_ASPIRATIONAL (test_protocol_alignment.py) and _KNOWN_UNWIRED_TESTED_OPS/_KNOWN_METADATA_ONLY_TESTED_PARAMS entries now that the ops are registry-wired and OP_SPEC-tested; re-key _KNOWN_UNTESTED_OPTION_PARAMS with accurate reasons for rounding/unit/ multiple (wired option, backend bodies still stubs) and origin (permanently excluded, always raises) with live machine-checked probes (closed-by-default per test_untested_option_param_custom_reasons_still_hold)
…(item 74 Task 4) FLOOR/CEIL/ROUND_TIE_DOWN hand-rolled from dt.truncate + dt.offset_by (no native CEIL/tie-down primitive). ROUND_TIE_UP uses native dt.round - verified its tie rule IS tie-up (10:30->11:00) and it accepts every combined multiplier/unit string used here including calendar units (1mo/3mo/1y/1w). round_temporal declares YEAR/MONTH/WEEK unsupported (defense in depth; real gate is a capability fact, Task 7) - fixed-duration rounding is ambiguous for them; round_calendar covers all nine units. New cross-backend test file (36 polars/polars-lazy cases, all passing): floor/ceil for DAY/HOUR/MONTH/YEAR/WEEK, tie-break at an exact hour midpoint, multiple=2 hours, quarter-via-month-multiple-3, and the YEAR/MONTH/WEEK round_temporal capability-error gate. ibis/narwhals cases in this file correctly RED pending Tasks 5-6.
…tem 74 Task 5) x.truncate(unit) accepts Substrait canonical uppercase names directly (verified 12.0.0/duckdb) for FLOOR at multiple=1. TimestampValue.bucket() (an experimental-but-working native primitive) handles multiple>1 for every unit including calendar ones, bucketing from the UNIX epoch. CEIL/tie modes hand-rolled via interval addition + epoch_seconds distance comparison (ibis has no native round/ceil and rejects Polars-style combined multiplier truncate strings). New capabilities/datetime/rounding.py: real per-dialect gaps discovered by the probe -- - ibis-sqlite: TimestampTruncate has no HOUR/MINUTE/SECOND/MILLISECOND/ MICROSECOND support (only YEAR/MONTH/WEEK/DAY), and TimestampBucket has no sqlite compilation rule at all (multiple>1 unsupported everywhere) - ibis-polars: CEIL/tie on MONTH/YEAR needs interval addition, which ibis's polars sub-backend cannot translate (polars.duration() has no months/years kwarg); the whole unit is declared UNSUPPORTED there (closed-by-default -- partial per-rounding-mode support is exactly the silent-inconsistency this repo's facts exist to prevent) Restructured the cross-backend test file to split value-assertion classes from the now-real capability-error classes for these two dialect gaps (xfail-not-skip: the combo IS tested, via a raise assertion instead of a value one). All ibis cases now pass (43->83 across ibis+polars); narwhals cases correctly RED pending Task 6.
…r (item 74 Task 6) Narwhals dt.truncate/dt.offset_by use Polars-style combined <n><suffix> strings (verified 2.24.0, both dialects) -- unlike ibis, bare Substrait names are not accepted. No native round/ceil/tie primitive at all, so every mode but FLOOR is hand-rolled the same way as polars (truncate + offset_by + duration-distance comparison via total_seconds()), verified including calendar units and multiple>1 (e.g. '3mo' for a quarter). WEEK is declared UNSUPPORTED on round_calendar for both narwhals dialects (dt.truncate rejects the '1w' duration, verified 2026-08-16) -- matches the pre-existing MA truncate/floor_dt '1w' gap in options.py rather than hand-rolling day-of-week arithmetic (v1 scope decision, preserve-not-fix). Restructured the test file's WEEK/DAY class into three: DAY (all backends), WEEK (excludes narwhals), and a dedicated narwhals-WEEK capability-error class. Full cross-backend test file: 126/126 passing across every backend. Argument-channel tests: 32/32 passing (was 24 RED before this task).
… audit datetime capability facts (item 74 Task 7-8)
- Redirect polars/ibis/narwhals MA truncate/round_dt/ceil_dt/floor_dt
wrappers through the real round_temporal/round_calendar
implementation (item 74 Task 4-6) instead of the old
x.truncate("<n><unit>")-based approximation that silently fell back
to truncate for round/ceil and rejected multi-digit multipliers on
ibis-duckdb/narwhals.
- Retire now-false DURATION_MULTIPLIER capability facts in
value_classes_ma.py: every fixture honors every multiplier value via
the real redirect (re-probed 2026-08-16, disposition table in module
docstring). Only ibis-sqlite's TimestampBucket compilation gap
remains, documented as a residual note (no compilation rule exists
on that dialect for any rounding op — tracked as a backlog
follow-up, not a fact, since MATRIX_UNREACHABLE_DIALECT_FACTS
already exempts it structurally).
- Add rounding.py capability module declaring ibis-sqlite's structural
gap for round_temporal/round_calendar (all rounding units
unsupported on that dialect — no compilation rule).
- Fix options.py/value_classes_ma.py declaration evidence fixtures to
reflect each module's actual probed scope, resolving a duplicate
CapabilityDeclaration identity collision across MOUNTAINASH-source
IBIS declarations.
- Wire rounding.py into the capability protocol guard's expected
module list and domain-predicate table.
- Add ROUND_TEMPORAL/ROUND_CALENDAR smoke-test arg overrides.
- Retire stale test_multiplier_gate cross-backend regression test
(asserted BackendCapabilityError on cells now honored); keep
assume_timezone IANA_TIMEZONE coverage.
- Remove now-XPASSing strict-xfail entries for ibis-duckdb/narwhals
round/ceil in test_datetime_unit_dispatch.py's fallback-divergence
map — both ops are real implementations now.
- Drain KNOWN_UNTESTED_OPTION_PARAMS rounding/unit/multiple parks with
a live-checked reason (dedicated cross-backend test file coverage,
verified via _round_temporal_calendar_still_works()).
- Regenerate docs/reference/expression-coverage{.md,.json,-scoped.md}.
Full suite: tests/expressions/ + tests/core/ all green (17701 passed,
65 skipped, 3400 xfailed, 0 failed). ruff clean.
…emporal-calendar # Conflicts: # docs/reference/expression-coverage-scoped.md # docs/reference/expression-coverage.json # docs/reference/expression-coverage.md # tests/expressions/argument_types/test_coverage_guard.py
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Implements the Substrait
round_temporal/round_calendardatetime rounding operations (backlog item 74) across all three backends (polars, ibis, narwhals), and redirects the existing MAtruncate/round_dt/ceil_dt/floor_dtconvenience wrappers through the real implementation instead of the old approximation that silently fell back to truncate for round/ceil and rejected multi-digit multipliers on ibis-duckdb/narwhals.Changes by task
ROUND_TEMPORAL/ROUND_CALENDARFKEYs + function defs.xargument channel, drained the unwired-ops park.TimestampBucketcompilation rule — declared UNSUPPORTED).everystring).truncate/round_dt/ceil_dt/floor_dtwrappers (polars/ibis/narwhals) throughround_temporal/round_calendar. Retired now-falseDURATION_MULTIPLIERcapability facts (re-probed 2026-08-16: every fixture now honors every multiplier value via the real redirect). Addedrounding.pycapability module for the ibis-sqlite structural gap. Fixed a duplicateCapabilityDeclarationidentity collision inoptions.py/value_classes_ma.pyevidence fixtures.KNOWN_UNTESTED_OPTION_PARAMSrounding/unit/multiple parks with a live-checked reason.Verification
ruff check: clean.mypy: baseline unchanged (97 pre-existing import-stub errors, none in touched files).tests/expressions/cross_backend/test_datetime_rounding.py(126/126 passing across every backend, including ibis-sqlite gap coverage).docs/reference/expression-coverage{.md,.json,-scoped.md}.Companion spec/plan (mountainash-central, superpowers workflow) tracked backlog item 74.