Skip to content

fix(cache): scalar-UDF-only entries never bake a result cache (#81) - #112

Merged
paddymul merged 2 commits into
mainfrom
fix/81-scalar-udf-worthiness
Jun 18, 2026
Merged

paddymul merged 2 commits into
mainfrom
fix/81-scalar-udf-worthiness

Conversation

@paddymul

Copy link
Copy Markdown
Contributor

Fixes #81.

Problem

The two worthiness predicates #73 requires to stay in lockstep ("change one, change both") disagree for an entry whose only expensive op is a scalar UDF.

  • source_cache._is_worthy_expr (src/tallyman_xorq/source_cache.py:75-78) walks the live graph and tests name in _EXPENSIVE_OPS or "UDF" in name, where name = type(node).__name__. A make_pandas_udf scalar UDF node is classed after the user function (e.g. plusone), so the substring check is False — even though the node's MRO is [plusone, ScalarUDF, Value, Node, …].
  • result_cache.classify_build (src/tallyman_xorq/result_cache.py:78-84) greps the serialized YAML, where the same node is op: ScalarUDF (contains "UDF"), so it returns worthy=True.

Net effect: rewrite_for_build bakes no top-level result cache (it gates on _is_worthy_expr), but at read cache_worthy is True so _resolve_result_plan takes the expensive branch, finds no CachedNode, and falls back to worthy-recompute-fallback — a full UDF recompute over the whole DAG on every viewer / diff / from_catalog read. No crash; the result is silently never cached. Agg UDFs (carry an Aggregate) and vectorized UDFs (live class name contains "UDF") are unaffected; the gap is specific to scalar UDFs used without any accompanying expensive op.

Change

_is_worthy_expr detects UDFs by ancestry, not leaf-name substring: it checks whether any class in type(node).__mro__ has "UDF" in its name. That mirrors classify_build's YAML rule exactly (the serializer emits the base-class op name, ScalarUDF), so the two predicates are back in lockstep by construction. The _EXPENSIVE_OPS leaf-name check is unchanged — those ops serialize under their own leaf names.

Status — TDD split

Per the repo's test-vs-fix rule, the first commit is the two failing tests (watched red on CI); the fix follows in the next commit (CI green):

  • test_scalar_udf_is_worthy_in_lockstep_with_classify_build — the predicate-level regression.
  • test_scalar_udf_only_entry_bakes_result_cache — the net effect (entry must bake a snapshot cached_result_expr dereferences).

Context

Surfaced during review of #74. The root shared-tmp/predicate-duplication is documented in result_cache.py; this restores the invariant the gap violated.

🤖 Generated with Claude Code

paddymul and others added 2 commits June 18, 2026 08:22
…rst)

A make_pandas_udf scalar UDF used without any accompanying expensive op
serializes as `op: ScalarUDF` (classify_build → worthy=True) but its live
node is classed after the user function (`plusone`), so _is_worthy_expr's
`"UDF" in type(node).__name__` substring check misses it and returns False.
The two "change one, change both" worthiness predicates fall out of lockstep,
so rewrite_for_build bakes no result cache and every read recomputes the UDF.

Two failing tests pin the regression: the predicate-level lockstep, and the
net effect (the entry must bake a snapshot cached_result_expr dereferences).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_is_worthy_expr tested `"UDF" in type(node).__name__`, but a make_pandas_udf
scalar UDF node is classed after the user function (e.g. `plusone`), so the
leaf-name substring missed it and the live predicate returned False — while
classify_build saw `op: ScalarUDF` in the YAML and returned worthy=True. The
two "change one, change both" predicates fell out of lockstep, so
rewrite_for_build baked no result cache and every read recomputed the UDF.

Walk type(node).__mro__ for "UDF" instead: that matches the base op name the
serializer emits (ScalarUDF / AggUDF / …), restoring the lockstep by
construction. The _EXPENSIVE_OPS leaf-name check is unchanged.

Net effect: a scalar-UDF-only entry now bakes a CachedNode and reads
dereference a materialised single-backend snapshot. The test asserts the
lockstep and that net effect. (xorq's make_pandas_udf mints a fresh class per
reconstruction, so the snapshot's structural key drifts across processes — the
build bake is not reused cross-process and reads self-heal. That deeper
key-stability gap is tracked separately.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul

Copy link
Copy Markdown
Contributor Author

Follow-up found while building this fix — UDF snapshot keys are non-deterministic

Verified empirically: the MRO predicate fix here is correct and necessary, but it surfaces a separate, deeper bug that this PR does not fully resolve.

xorq.expr.udf.make_pandas_udf mints a fresh class on every call, so a scalar-UDF node's structural hash differs on each recipe reconstruction. Concretely, baked_snapshot_path returns three different keys across three calls for one UDF entry (an Aggregate entry returns the same key every time). Consequences:

  • The build bakes the snapshot under key K₀, but every read reconstructs the recipe → new UDF class → new key Kₙ → cache miss, so the read self-heals its own snapshot. Cross-process (MCP build → companion read) the build's bake is never reused.
  • baked_snapshot_path therefore returns a phantom path for UDF entries — the cache inspector (/api/result_cache) silently skips them and the delete endpoint 404s (both guard on .exists(), so no crash).

In-process the lru-memoised plan keeps one snapshot warm, so repeated reads in a live companion do hit — a real improvement over recompute-every-read. The cross-process reuse + phantom-path gap is rooted in xorq's UDF node hashing; a proper fix (e.g. keying the result cache on a stable recipe-source digest rather than xorq's structural key) is bigger than this lockstep fix and should be tracked separately.

This PR's test asserts only what is true post-fix: predicates agree, the build bakes a CachedNode, and a read dereferences a materialised single-backend snapshot.

@paddymul

Copy link
Copy Markdown
Contributor Author

Filed the cross-process / phantom-path follow-up described above as #117.

@paddymul
paddymul merged commit c548ef3 into main Jun 18, 2026
4 checks passed
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.

caching: scalar-UDF-only entries never bake a result cache — _is_worthy_expr misses function-named UDF nodes that classify_build sees in YAML

1 participant