diff --git a/CHANGELOG.md b/CHANGELOG.md index 66303ca23e..831b997da7 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 `#1658` seeded index re-engages on Polars/Polars-GPU for float-column graphs (restores identity-stable `_pl_nan_to_null`, regression from #1731)**: #1726 made `Engine._pl_nan_to_null` identity-stable — it probes `is_nan().any()` per float column and returns the *same* frame object when no NaN is present, because `fill_nan(None)` on a NaN-free column is a value-level no-op that still yields a fresh frame, and that identity churn defeats the #1658 CSR index's `source_ref is df` residency check (so Polars/Polars-GPU seeded hops silently fell back to the O(E) scan). PR #1731's "final hardening" (commit `c2138b0d`) accidentally reverted this to the unconditional `df.with_columns([fill_nan(None) ...])`, re-breaking index engagement on any graph carrying a Float32/Float64 column (SNB/LDBC, graph-bench); all-int/str graphs like Pokec were unaffected via the no-float-columns fast return. This restores the identity-stable probe (rebuilding only — and only the columns where — a NaN is genuinely present; the rare LazyFrame path keeps the unconditional rewrite). Value-identical to the old behavior; only object identity changes so the resident index survives a polars conversion. Regression guard `TestPlNanToNull` in `test_engine_coercion.py` (restored + kept as a regression fence). - **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/graphistry/Engine.py b/graphistry/Engine.py index a3d93c2939..6668b58b2b 100644 --- a/graphistry/Engine.py +++ b/graphistry/Engine.py @@ -214,11 +214,30 @@ def _pl_nan_to_null(df): 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.""" + No-op when there are no float columns. + + Identity-stable: returns the *same* frame object when no float column actually carries a + NaN. ``fill_nan(None)`` on a NaN-free column is a value-level no-op but still yields a + FRESH frame; that identity churn defeats frame-identity caches keyed on ``source_ref is + df`` (the #1658 CSR index) -- see #1726. So we probe for real NaN presence per float + column (``is_nan`` only applies to float dtypes, hence the schema gate) and rebuild only + when -- and only the columns where -- a NaN is genuinely present. Values are identical to + the old unconditional rewrite (``fill_nan(None)`` never touches non-NaN entries). + + NOTE: this identity guard was originally landed in #1726 (3809ecd9) and then accidentally + reverted by #1731 (c2138b0d, "final hardening"); this restores it. Regression bit polars/ + polars-gpu index engagement on any float-column graph (SNB/LDBC, graph-bench); Pokec was + unaffected because its nodes are all int/str (the no-float-columns fast return).""" 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): + nan_cols = [c for c in float_cols if df.get_column(c).is_nan().any()] + if not nan_cols: + return df + return df.with_columns([pl.col(c).fill_nan(None) for c in nan_cols]) + # LazyFrame (rare): no cheap eager NaN probe, keep the original unconditional rewrite. return df.with_columns([pl.col(c).fill_nan(None) for c in float_cols]) diff --git a/graphistry/tests/compute/test_engine_coercion.py b/graphistry/tests/compute/test_engine_coercion.py index 574900673e..fedd94d187 100644 --- a/graphistry/tests/compute/test_engine_coercion.py +++ b/graphistry/tests/compute/test_engine_coercion.py @@ -684,5 +684,73 @@ def test_chain_dask_edges(self): self.assertIsInstance(result._edges, pd.DataFrame) +class TestPlNanToNull(NoAuthTestCase): + """_pl_nan_to_null must be identity-stable when no NaN is present (#1726) while + preserving the NaN -> null conversion when a float column actually carries a NaN. + + Regression guard: #1726 (3809ecd9) added this; #1731 (c2138b0d) reverted it; restored here. + """ + + @unittest.skipUnless(HAS_POLARS, "polars not installed") + def test_float_col_without_nan_returns_same_object(self): + # A NaN-free float column must NOT trigger a rebuild (frame-identity churn would + # defeat the #1658 CSR index, which keys resident indexes on `source_ref is df`). + from graphistry.Engine import _pl_nan_to_null + inp = pl.DataFrame({"src": ["a", "b"], "w": [1.0, 2.0]}) + out = _pl_nan_to_null(inp) + self.assertIs(out, inp) + + @unittest.skipUnless(HAS_POLARS, "polars not installed") + def test_no_float_cols_returns_same_object(self): + from graphistry.Engine import _pl_nan_to_null + inp = pl.DataFrame({"src": ["a", "b"], "dst": ["b", "c"]}) + out = _pl_nan_to_null(inp) + self.assertIs(out, inp) + + @unittest.skipUnless(HAS_POLARS, "polars not installed") + def test_float_col_with_nan_still_converts_to_null(self): + # NaN-present path is unchanged: NaN -> null, other values untouched. + from graphistry.Engine import _pl_nan_to_null + inp = pl.DataFrame({"src": ["a", "b", "c"], "w": [1.0, float("nan"), 3.0]}) + out = _pl_nan_to_null(inp) + self.assertIsNot(out, inp) + w = out.get_column("w") + # The NaN became a genuine null (not still a NaN), non-NaN values preserved. + self.assertEqual(w.null_count(), 1) + self.assertFalse(bool(w.is_nan().any())) + self.assertEqual(w.to_list(), [1.0, None, 3.0]) + + @unittest.skipUnless(HAS_POLARS, "polars not installed") + def test_existing_nulls_without_nan_returns_same_object(self): + # A float column with real nulls but no NaN needs no rewrite (identity preserved). + from graphistry.Engine import _pl_nan_to_null + inp = pl.DataFrame({"src": ["a", "b"], "w": [1.0, None]}) + out = _pl_nan_to_null(inp) + self.assertIs(out, inp) + + @unittest.skipUnless(HAS_POLARS, "polars not installed") + def test_multi_float_col_rebuilds_only_nan_carrying_columns(self): + # With several float columns, only the ones that actually carry a NaN are converted; + # NaN-free float columns are left byte-for-byte as-is (values identical to the old + # unconditional rewrite, which is a no-op on non-NaN entries anyway). + from graphistry.Engine import _pl_nan_to_null + inp = pl.DataFrame({ + "id": [0, 1, 2], + "clean": [1.0, 2.0, 3.0], # float, no NaN -> untouched + "dirty": [1.0, float("nan"), 3.0], # float, has NaN -> converted + }) + out = _pl_nan_to_null(inp) + self.assertIsNot(out, inp) # a NaN was present -> new frame + # NaN-free column unchanged (still float, no nulls introduced). + self.assertEqual(out.get_column("clean").to_list(), [1.0, 2.0, 3.0]) + self.assertEqual(out.get_column("clean").null_count(), 0) + # NaN column converted to null, non-NaN values preserved. + self.assertEqual(out.get_column("dirty").to_list(), [1.0, None, 3.0]) + self.assertEqual(out.get_column("dirty").null_count(), 1) + # Column order + non-float columns preserved. + self.assertEqual(out.columns, ["id", "clean", "dirty"]) + self.assertEqual(out.get_column("id").to_list(), [0, 1, 2]) + + if __name__ == "__main__": unittest.main()