fix(gfql): cudf 26.02 dt.date/dt.time removal in temporal predicates (replaces #1855) - #1862
Merged
Conversation
Replaces #1855, which was built on a false premise. That PR treated 'cudf works but cupy cannot JIT' as a capability tier and threaded a two-tier gate through five modules. It is not a tier -- it is a broken install: cudf-cu12 HARD-REQUIRES cupy-cuda12x>=13.6.0, importing cudf imports cupy, and cudf.Series.values RETURNS a cupy.ndarray. The only place that configuration exists is a dev box with cudf installed and no GPU. So the capability machinery goes: lazy_cupy_import's NVRTC probe, CudfRuntimeCaps, and the gating in engine_arrays.py, mercator.py, ring/util.py. Also _unary_ufunc_on_series, which was purely the cupy-broken fallback and collapses to np_fn(s) -- what master already does. No perf is lost, because those fallbacks cannot fire in a valid install. row/pipeline.py needs NO change either: hasattr(f, 'ceil') already returns False on 26.02, falls to np.ceil(f), dispatches to cupy, and works. What was real: comparison.py called Series.dt.date / .dt.time unguarded, and 26.02 removed both. 168 lines across 8 files becomes 21 lines in 1, plus a pin that simulates the accessor removal so it is caught without a GPU. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
The first pin asserted pandas behaviour and never called comparison.py, so the changed lines had ZERO coverage and CI's changed-line gate correctly failed it -- the fix was shipping untested while the test passed. It now hides .dt.date/.dt.time (pd.Series.dt is a CachedAccessor, so the wrapper goes around the value it produces, not the descriptor) and drives GT(DateValue) and GT(TimeValue) end to end, exercising both fallbacks and the scalar pairing. It then undoes the patch and asserts the un-hidden path agrees, which makes the fallback an equivalence rather than just 'does not crash'. All three changed ranges are now covered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
…ery engine Review points, both resolved by removing the branch rather than typing around it. STATIC TYPES: there is now nothing version-dependent to type. Series.dt.date / .dt.time are gone in cudf 26.02, but the substitute -- day-truncate, and time-of-day as a timedelta -- exists on every engine and every version. So the predicate just always uses it: no hasattr probe, no minimum-cudf assumption, and nothing to keep out of the public types pending a version floor. Verified the substitute is an EQUIVALENCE for both ordering and boundary equality, which is what makes the branch removable rather than merely convenient. CROSS-PLATFORM: the pin is parametrized over pandas and cudf and asserts identical answers, including the equality boundaries where object-dtype dates and datetime64 could diverge. The earlier pin simulated the accessor removal on pandas only, which is now moot -- nothing calls the removed accessor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
Real cudf 26.02 does not REMOVE Series.dt.date/.dt.time -- they exist and raise NotImplementedError on access. hasattr only swallows AttributeError, so it does not screen them: the capability branch this PR started with would have CRASHED rather than fallen back, and the pandas simulation that covered it asserted the wrong exception and passed anyway. Verified on GB10, cudf 26.02.01: both accessors raise NotImplementedError, and 43 temporal tests pass with the branch gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
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.
Replaces #1855, which was built on a false premise — reviewer caught it.
The premise was wrong
#1855 treated "cudf works but cupy cannot JIT" as a capability tier and threaded a two-tier gate through five modules. It is not a tier, it is a broken install:
cudf-cu12hard-requirescupy-cuda12x>=13.6.0cudf.Series.values**returns a `cupy.ndarray```The only place that configuration exists is a dev box with cudf installed and no GPU. I was coding graceful degradation for my own half-install.
What that removes, with no perf loss
Dropped:
lazy_cupy_import's NVRTC probe,CudfRuntimeCaps, and the gating inengine_arrays.py,mercator.py,ring/util.py. Also_unary_ufunc_on_series, which was purely the cupy-broken fallback — it collapses tonp_fn(s), exactly what master already does. No perf is lost, because those fallbacks cannot fire in a valid install.row/pipeline.pyneeds no change either:hasattr(f, "ceil")already returns False on 26.02, falls through tonp.ceil(f), dispatches to cupy, works.What was actually real
comparison.pycalledSeries.dt.date/Series.dt.timeunguarded and cudf 26.02 removed both. The fallbacks rely on a stated equivalence — a day-truncated datetime compares against a midnightTimestampexactly as a date does; a time-of-day timedelta against aTimedeltaexactly as a time does — with the scalar paired to the dtype the truncation produces, so no comparison silently changes meaning.168 lines across 8 files → 21 lines in 1, plus a pin that simulates the accessor removal so the regression is caught without a GPU.
I'd close #1855 in favour of this.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi