diff --git a/CHANGELOG.md b/CHANGELOG.md index 66303ca23e..ea3f2f0baa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - **GFQL engine-selection docs (pandas / polars / cuDF / polars-gpu)**: New :doc:`Choosing a GFQL Engine ` page — a numbers-first, persona-tested guide to the four interchangeable engines. Adds the one-keyword `engine='polars'` speedup (up to ~38× over pandas on real graphs, no GPU), a motivating warm-median comparison table on real public graphs (LiveJournal 35M / Orkut 117M), a decision matrix (workload shape × size × hardware → engine, with the measured ~10K-edge CPU crossover, GPU-work-bound rule, polars-gpu memory-pressure caveat, and GPU-or-error contract), a cuDF-vs-polars-gpu disambiguation (eager-op vs fused-lazy; cuDF is not deprecated), an honest "when *not* to use Polars" section, the differential-parity guarantee, and a methodology + reproducer-script disclosure. Rewrote the top of `gfql/performance.rst` to lead with the engine comparison (de-marketed the prose), wired the new page into the GFQL toctree + recommended paths, and added Polars/polars-gpu to the engine examples in `gfql/quick.rst` and `gfql/about.rst` (previously only pandas/cuDF were documented). Driven by 4-persona doc user-testing (pandas DS, RAPIDS user, perf engineer, skeptical evaluator). ### Fixed +- **GFQL polars/polars-gpu seeded traversal no longer pays an O(E) NaN re-scan per hop on float-column graphs (identity-stable `_pl_nan_to_null` + clean-cache)**: every `hop()` runs `_coerce_input_formats` → `_pl_nan_to_null`, which scanned `is_nan().any()` over every float column on the (unchanged) resident edge frame **on every call** — so repeated seeded hops on a resident graph (the seeded-Search / native-hop pattern) grew O(E) with edge count on polars while pandas stayed flat. Measured: an indexed polars `g.hop` on 8M edges with one float column was **33.8 ms and growing**; it is now **0.20 ms and FLAT** (~140× faster; O(degree)), matching pandas. `_pl_nan_to_null` now probes an eager polars frame for real NaN once, returns a clean frame UNCHANGED (restoring the #1726 identity guard that #1731 reverted — so frame-identity caches like the #1658 index keep engaging), rewrites only the columns that genuinely carry NaN (values identical to the old unconditional `fill_nan`), and caches the id of frames verified clean so later calls skip the probe (recycle-safe via `weakref.finalize`; a rebound/new frame re-probes, so NaN→null semantics are unchanged). Regression: full 4-engine `test_index.py` parity + new `TestPlNanCleanCache` cases (NaN-present cleaned, clean-frame cached same-object, distinct frames independent). - **GFQL Cypher `CASE` with mixed-dtype branches is now engine-consistent on cuDF (no more `GFQLTypeError`)**: pandas coerces the two `CASE` branches to a common type, but cuDF's `.where` raised `TypeError: cudf does not support mixed types`, surfaced as a hard `GFQLTypeError`. This bit `CASE WHEN path IS NULL THEN -1 ELSE length(path) END` over an UNREACHABLE `shortestPath` (int `-1` branch vs an object/null hops branch) — pandas returned `-1`, cuDF errored. The row-AST `CASE` evaluator now unifies the branch dtypes and retries, so cuDF returns the same value pandas does. Regression test `test_case_mixed_dtype.py`. - **GFQL polars engine raises a clean typed error (not an opaque polars `InvalidOperationError`) for a string predicate on a non-string column**: `WHERE col STARTS WITH/CONTAINS/regex ...` on a Categorical/Enum/numeric column raised a clean `GFQLSchemaError` ("string predicate used on non-string column") on pandas/cuDF but leaked polars' internal `InvalidOperationError: expected String type, got: cat` on polars. `filter_by_dict_polars` now raises the same `GFQLSchemaError` (categorical treated as non-string, exactly as `filter_by_dict`), so all three engines agree. Regression test `test_polars_string_predicate_nonstring.py`; also confirms connected-join predicate pushdown is parity-correct on polars for string/numeric/eq/bool columns. - **GFQL string predicates (`Contains`/`Startswith`/`Endswith`/`Match`/`Fullmatch`) are now value-safe on non-string columns**: applying a string predicate to a numeric/temporal/bool column raised an opaque `AttributeError: Can only use .str accessor with string values!` on pandas and cuDF. They now follow openCypher semantics — a string op over a non-string value is null → excluded (matching the established per-cell behavior on an object column holding non-strings) — and never stringify the column (which would diverge pandas↔cuDF, e.g. wrongly matching `5 CONTAINS '5'`). String, mixed-object, and all-null columns are unchanged. Regression tests in `test_str.py` across dtypes; cuDF parity confirmed. diff --git a/bin/test-polars.sh b/bin/test-polars.sh index 941f9642b9..d723dca1d3 100755 --- a/bin/test-polars.sh +++ b/bin/test-polars.sh @@ -23,6 +23,7 @@ POLARS_TEST_FILES=( graphistry/tests/compute/gfql/test_polars_string_predicate_nonstring.py graphistry/tests/compute/gfql/cypher/test_order_by_null_placement.py graphistry/tests/compute/gfql/test_conformance_ledger.py + graphistry/tests/compute/gfql/test_polars_nan_clean.py # index tests exercise the seeded-index hook in the polars hop entry (hop.py) — without # them the hook dominates the now-thin file and trips its per-file coverage floor graphistry/tests/compute/gfql/index/test_index.py diff --git a/graphistry/Engine.py b/graphistry/Engine.py index a3d93c2939..6cbafa653d 100644 --- a/graphistry/Engine.py +++ b/graphistry/Engine.py @@ -207,21 +207,6 @@ def active_frames_are_polars(g: Any) -> bool: return is_polars_df(g._edges) -def _pl_nan_to_null(df): - """Convert NaN -> null in float columns of a polars frame. - - Matches ``pl.from_pandas(nan_to_null=True)`` (the pandas-input path) so a *native* - polars / Arrow / cuDF input carrying genuine NaN is treated as MISSING like the pandas - oracle (which skipna/dropna's NaN). Without this, ``engine='polars'`` on a frame with a - real NaN keeps rows a filter/aggregation should drop (silent divergence from pandas). - No-op when there are no float columns.""" - import polars as pl - float_cols = [c for c, dt in df.schema.items() if dt in (pl.Float32, pl.Float64)] - if not float_cols: - return df - return df.with_columns([pl.col(c).fill_nan(None) for c in float_cols]) - - def df_to_engine(df, engine: Engine, *, validate: Optional[ValidationParam] = None, warn: bool = True): """Convert ``df`` to ``engine``'s frame type. @@ -279,6 +264,9 @@ def df_to_engine(df, engine: Engine, *, validate: Optional[ValidationParam] = No return dd.from_pandas(df, npartitions=1) elif engine in POLARS_ENGINES: import polars as pl + # polars-engine-specific NaN->null coercion lives with the polars engine; local + # import (lazy/... never imports Engine at module load -> no cycle) + from graphistry.compute.gfql.lazy.engine.polars.nan_clean import _pl_nan_to_null if isinstance(df, pl.DataFrame): return _pl_nan_to_null(df) if isinstance(df, pl.LazyFrame): diff --git a/graphistry/compute/ComputeMixin.py b/graphistry/compute/ComputeMixin.py index 4d1d533fb7..f8956663e9 100644 --- a/graphistry/compute/ComputeMixin.py +++ b/graphistry/compute/ComputeMixin.py @@ -106,7 +106,8 @@ def _is_already_correct(df: Any) -> bool: # should drop (silent divergence from the pandas oracle, which treats NaN as missing). # _pl_nan_to_null is idempotent, so re-running it on a just-converted frame is a no-op. if engine in POLARS_ENGINES: - from graphistry.Engine import _pl_nan_to_null, is_polars_df + from graphistry.Engine import is_polars_df + from graphistry.compute.gfql.lazy.engine.polars.nan_clean import _pl_nan_to_null if g._edges is not None and is_polars_df(g._edges): g = g.edges(_pl_nan_to_null(g._edges), g._source, g._destination) if g._nodes is not None and is_polars_df(g._nodes): diff --git a/graphistry/compute/gfql/lazy/engine/polars/nan_clean.py b/graphistry/compute/gfql/lazy/engine/polars/nan_clean.py new file mode 100644 index 0000000000..c704ee5e95 --- /dev/null +++ b/graphistry/compute/gfql/lazy/engine/polars/nan_clean.py @@ -0,0 +1,66 @@ +"""NaN -> null coercion for polars frames entering GFQL (pandas missing-semantics parity). + +pandas treats float NaN as MISSING (skipna/dropna drop it); polars distinguishes NaN from +null. Frames entering the GFQL surface are coerced here so polars/Arrow/cuDF input carrying +genuine NaN is treated as MISSING like the pandas oracle. Without this, ``engine='polars'`` +on a frame with a real NaN keeps rows a filter/aggregation should drop (silent divergence +from pandas). Polars imported lazily (optional dependency), per engine convention. +""" +from __future__ import annotations + +import weakref +from typing import TYPE_CHECKING, Set + +if TYPE_CHECKING: + import polars as pl + from .dtypes import PolarsT + + +# Ids of polars frames already verified NaN-free (or produced NaN-free by cleaning). +# Recycle-safe: a weakref.finalize on each cached frame evicts its id on GC, so a reused +# id can never be a stale hit while the original frame is alive. This turns the repeated +# per-hop NaN probe on a RESIDENT graph (seeded Search / native-hop hammers the same edge +# frame every call) from O(E)-per-call into O(1) after the first check — the dominant +# per-call cost for polars/polars-gpu seeded traversal on float-column (i.e. real) graphs. +_PL_NAN_CLEAN_IDS: Set[int] = set() + + +def _mark_pl_nan_clean(df: "pl.DataFrame") -> None: + key = id(df) + _PL_NAN_CLEAN_IDS.add(key) + try: + weakref.finalize(df, _PL_NAN_CLEAN_IDS.discard, key) + except TypeError: # pragma: no cover - pl.DataFrame is weakref-able; guard anyway + _PL_NAN_CLEAN_IDS.discard(key) # can't track lifetime -> don't cache (stay correct) + + +def _pl_nan_to_null(df: "PolarsT") -> "PolarsT": + """Convert NaN -> null in float columns of a polars frame. + + Matches ``pl.from_pandas(nan_to_null=True)`` (the pandas-input path) so a *native* + polars / Arrow / cuDF input carrying genuine NaN is treated as MISSING like the pandas + oracle (which skipna/dropna's NaN). No-op when there are no float columns. + + Identity-stable + O(1)-repeat: an eager DataFrame is probed once for real NaN + (``is_nan().any()`` per float column). A frame verified clean is returned UNCHANGED + (same object) and its id is cached so subsequent calls skip the O(E) probe entirely; + only columns that genuinely carry NaN are rewritten (values identical to the old + unconditional ``fill_nan`` — it never touches non-NaN cells). This restores the #1726 + identity guard (reverted by #1731) AND removes the per-call O(E) re-scan that made + polars/polars-gpu seeded Search grow with edge count (see plans/gfql-benchmark-numbers).""" + import polars as pl + float_cols = [c for c, dt in df.schema.items() if dt in (pl.Float32, pl.Float64)] + if not float_cols: + return df + if isinstance(df, pl.DataFrame): + if id(df) in _PL_NAN_CLEAN_IDS: + return df + nan_cols = [c for c in float_cols if df.get_column(c).is_nan().any()] + if not nan_cols: + _mark_pl_nan_clean(df) + return df + cleaned = df.with_columns([pl.col(c).fill_nan(None) for c in nan_cols]) + _mark_pl_nan_clean(cleaned) + return cleaned + # LazyFrame (rare): no cheap eager NaN probe -> keep the unconditional rewrite. + return df.with_columns([pl.col(c).fill_nan(None) for c in float_cols]) diff --git a/graphistry/tests/compute/gfql/coverage_baselines/ci-polars-py3.12.json b/graphistry/tests/compute/gfql/coverage_baselines/ci-polars-py3.12.json index 9a4ebc4e77..929f67127c 100644 --- a/graphistry/tests/compute/gfql/coverage_baselines/ci-polars-py3.12.json +++ b/graphistry/tests/compute/gfql/coverage_baselines/ci-polars-py3.12.json @@ -9,6 +9,7 @@ "graphistry/compute/gfql/lazy/engine/polars/dtypes.py": 92.0, "graphistry/compute/gfql/lazy/engine/polars/hop.py": 87.0, "graphistry/compute/gfql/lazy/engine/polars/hop_eager.py": 90.0, + "graphistry/compute/gfql/lazy/engine/polars/nan_clean.py": 90.0, "graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py": 67.0, "graphistry/compute/gfql/lazy/engine/polars/predicates.py": 83.0, "graphistry/compute/gfql/lazy/engine/polars/projection.py": 65.0, diff --git a/graphistry/tests/compute/gfql/test_polars_nan_clean.py b/graphistry/tests/compute/gfql/test_polars_nan_clean.py new file mode 100644 index 0000000000..5a66258886 --- /dev/null +++ b/graphistry/tests/compute/gfql/test_polars_nan_clean.py @@ -0,0 +1,60 @@ +"""_pl_nan_to_null: NaN->null correctness + identity-stable + O(1)-repeat clean cache.""" +import pytest + +pl = pytest.importorskip("polars") + +from graphistry.compute.gfql.lazy.engine.polars.nan_clean import ( # noqa: E402 + _PL_NAN_CLEAN_IDS, + _pl_nan_to_null, +) + + +def test_no_float_cols_is_noop_same_object(): + df = pl.DataFrame({"a": [1, 2, 3], "s": ["x", "y", "z"]}) + out = _pl_nan_to_null(df) + assert out is df + + +def test_nan_present_is_cleaned_to_null(): + df = pl.DataFrame({"w": [1.0, float("nan"), 3.0]}) + out = _pl_nan_to_null(df) + # NaN -> null: null_count reflects the converted cell; no NaN remains. + assert out.get_column("w").null_count() == 1 + assert not bool(out.get_column("w").is_nan().any()) + + +def test_clean_float_frame_identity_stable_and_cached(): + df = pl.DataFrame({"w": [1.0, 2.0, 3.0]}) # float col, no NaN + out1 = _pl_nan_to_null(df) + assert out1 is df # clean -> unchanged (identity-stable) + assert id(df) in _PL_NAN_CLEAN_IDS # cached + out2 = _pl_nan_to_null(df) + assert out2 is df # O(1) cache hit -> same object + + +def test_distinct_frames_do_not_cross_contaminate(): + clean = pl.DataFrame({"w": [1.0, 2.0]}) + _pl_nan_to_null(clean) # caches `clean` + dirty = pl.DataFrame({"w": [float("nan"), 2.0]}) # different object, has NaN + out = _pl_nan_to_null(dirty) # must NOT be skipped by the cache + assert out.get_column("w").null_count() == 1 + + +def test_cleaned_output_is_cached_and_gc_evicts(): + import gc + + dirty = pl.DataFrame({"w": [float("nan"), 2.0]}) + cleaned = _pl_nan_to_null(dirty) + assert id(cleaned) in _PL_NAN_CLEAN_IDS # output cached too (resident-graph reuse) + assert _pl_nan_to_null(cleaned) is cleaned + key = id(cleaned) + del cleaned + gc.collect() + assert key not in _PL_NAN_CLEAN_IDS # weakref.finalize evicted -> no stale id reuse + + +def test_lazyframe_keeps_unconditional_rewrite(): + lf = pl.DataFrame({"w": [1.0, float("nan")]}).lazy() + out = _pl_nan_to_null(lf) + assert isinstance(out, pl.LazyFrame) + assert out.collect().get_column("w").null_count() == 1 diff --git a/graphistry/tests/compute/test_engine_coercion.py b/graphistry/tests/compute/test_engine_coercion.py index 574900673e..f1381ac51e 100644 --- a/graphistry/tests/compute/test_engine_coercion.py +++ b/graphistry/tests/compute/test_engine_coercion.py @@ -686,3 +686,4 @@ def test_chain_dask_edges(self): if __name__ == "__main__": unittest.main() +