Skip to content

Commit dee6b8b

Browse files
committed
refactor(api): drop cache()/reference from this PR (track as #211)
Remove the cache() CTE feature and its ReferenceRel plumbing to keep this PR focused. Per review, shared subplans should be implemented at both the builder and dataframe layers with subtrees carried in-band in the Plan (like extensions) rather than via a contextvar; tracked in #211. - frame.py: remove DataFrame.cache(), _CteContext, the _cte_context contextvar, and _materialize's subtree prepend (to_plan/to_substrait resolve directly); drop the now-unreachable hint() guard for common-less relations. - type_inference.py: remove the reference_subtrees contextvar and the ReferenceRel schema-inference case. - builders/plan.py: remove the reference() builder added in fb1c49b. - tests: remove the cache/reference and hint-after-cache tests.
1 parent fb1c49b commit dee6b8b

4 files changed

Lines changed: 3 additions & 192 deletions

File tree

src/substrait/builders/plan.py

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -389,36 +389,6 @@ def resolve(registry: ExtensionRegistry) -> stp.Plan:
389389
return resolve
390390

391391

392-
def reference(
393-
subtree_ordinal: int,
394-
names: Iterable[str],
395-
*extension_sources: PlanOrUnbound,
396-
) -> UnboundPlan:
397-
"""A plan whose root references a shared subtree by ordinal (a ReferenceRel).
398-
399-
``subtree_ordinal`` indexes into the plan's leading shared relations (the
400-
common subplans a ``ReferenceRel`` points at); ``names`` labels the output.
401-
``extension_sources`` are the already-resolved subtree plans whose extension
402-
declarations are propagated, so a builder merging this plan upward keeps the
403-
subtree's extensions.
404-
405-
Inferring the resulting ``ReferenceRel``'s schema needs the subtree list to
406-
be in scope via :data:`substrait.type_inference.reference_subtrees`; this
407-
builder is meant for shared-subplan / CTE construction (see
408-
``substrait.dataframe.DataFrame.cache``), which sets that up.
409-
"""
410-
411-
def resolve(registry: ExtensionRegistry) -> stp.Plan:
412-
rel = stalg.Rel(reference=stalg.ReferenceRel(subtree_ordinal=subtree_ordinal))
413-
return stp.Plan(
414-
version=default_version,
415-
relations=[stp.PlanRel(root=stalg.RelRoot(input=rel, names=list(names)))],
416-
**_merge_extensions(*extension_sources),
417-
)
418-
419-
return resolve
420-
421-
422392
def fetch(
423393
plan: PlanOrUnbound,
424394
offset: ExtendedExpressionOrUnbound,

src/substrait/dataframe/frame.py

Lines changed: 3 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,17 @@
2828

2929
from __future__ import annotations
3030

31-
import contextvars
3231
from itertools import combinations
3332
from typing import Any, Iterable, Optional, Union
3433

3534
import substrait.algebra_pb2 as stalg
36-
import substrait.plan_pb2 as stpl
3735
import substrait.type_pb2 as stp
3836

3937
from substrait.builders import plan as _plan
4038
from substrait.builders import type as _type
4139
from substrait.dataframe.expr import Expr, Measure, col, lit
4240
from substrait.extension_registry import ExtensionRegistry
43-
from substrait.type_inference import infer_plan_schema, reference_subtrees
41+
from substrait.type_inference import infer_plan_schema
4442

4543
# All 13 JoinRel.JoinType variants (SET_OP_UNSPECIFIED excluded). "single"
4644
# returns at most one right match per left row (runtime error on multiple);
@@ -126,34 +124,6 @@ def _split_measure(m: Union[Expr, Measure]):
126124
return _unbound(m), None
127125

128126

129-
class _CteContext:
130-
"""Collects shared subtrees while a plan with ``cache()`` is being built."""
131-
132-
__slots__ = ("subtrees", "names", "plans", "ordinal_by_token")
133-
134-
def __init__(self):
135-
self.subtrees: "list[stalg.Rel]" = [] # indexed by subtree_ordinal
136-
self.names: "list[list[str]]" = []
137-
self.plans: "list[stpl.Plan]" = [] # the resolved subtree plans (extensions)
138-
self.ordinal_by_token: dict = {}
139-
140-
141-
# Active while a DataFrame is being materialized; None otherwise.
142-
#
143-
# ``cache()`` needs one accumulator shared by every nested ``resolve()`` in a
144-
# single ``to_plan()`` -- to dedupe repeated uses of a cached frame by identity
145-
# and hand out ``subtree_ordinal``s. The builder contract is
146-
# ``UnboundPlan = Callable[[ExtensionRegistry], Plan]``, which has no slot to
147-
# thread that accumulator, and schema inference (which resolves ReferenceRels
148-
# against the subtree list) runs *inside* the builder layer. Rather than widen
149-
# that contract across every builder, ``cache()`` publishes the accumulator here
150-
# and ``type_inference.reference_subtrees`` exposes its ``.subtrees`` list to
151-
# inference; ``_materialize`` sets both for the duration of the build.
152-
_cte_context: contextvars.ContextVar = contextvars.ContextVar(
153-
"cte_context", default=None
154-
)
155-
156-
157127
_default_registry: Optional[ExtensionRegistry] = None
158128

159129

@@ -532,15 +502,6 @@ def resolve(registry: ExtensionRegistry):
532502
bound = inner(registry)
533503
rel = bound.relations[-1].root.input
534504
rel_inner = getattr(rel, rel.WhichOneof("rel_type"))
535-
# A few relations (ReferenceRel from .cache(), UpdateRel) carry no
536-
# RelCommon and so cannot hold a hint -- fail with a clear message
537-
# rather than an opaque AttributeError on `.common`.
538-
if "common" not in rel_inner.DESCRIPTOR.fields_by_name:
539-
raise TypeError(
540-
f"cannot attach a hint to a {rel_inner.DESCRIPTOR.name} "
541-
"(e.g. a cached/reference relation); apply .hint(...) before "
542-
".cache()"
543-
)
544505
common = rel_inner.common
545506
if row_count is not None:
546507
common.hint.stats.row_count = row_count
@@ -666,62 +627,13 @@ def write_named_table(
666627
)
667628
)
668629

669-
def cache(self) -> "DataFrame":
670-
"""Mark this DataFrame as a reusable common subplan (a CTE).
671-
672-
Every use of the returned frame in the same ``to_plan()`` emits the
673-
subplan once as a shared subtree and references it (``ReferenceRel``),
674-
instead of inlining a fresh copy each time.
675-
"""
676-
inner = self._plan
677-
token = object() # identity for this cached node
678-
679-
def resolve(registry: ExtensionRegistry) -> stpl.Plan:
680-
ctx = _cte_context.get(None)
681-
if ctx is None: # built without a context -> just inline
682-
return inner(registry)
683-
ordinal = ctx.ordinal_by_token.get(token)
684-
if ordinal is None:
685-
subplan = inner(registry)
686-
ordinal = len(ctx.subtrees)
687-
ctx.ordinal_by_token[token] = ordinal
688-
ctx.subtrees.append(subplan.relations[-1].root.input)
689-
ctx.names.append(list(subplan.relations[-1].root.names))
690-
ctx.plans.append(subplan)
691-
# Emit a ReferenceRel to the shared subtree, propagating the
692-
# subtree's extensions so builder merges carry them up.
693-
return _plan.reference(ordinal, ctx.names[ordinal], ctx.plans[ordinal])(
694-
registry
695-
)
696-
697-
return self._next(resolve)
698-
699-
def _materialize(self, registry: ExtensionRegistry) -> stpl.Plan:
700-
ctx = _CteContext()
701-
cte_token = _cte_context.set(ctx)
702-
ref_token = reference_subtrees.set(ctx.subtrees)
703-
try:
704-
plan = self._plan(registry)
705-
finally:
706-
_cte_context.reset(cte_token)
707-
reference_subtrees.reset(ref_token)
708-
if not ctx.subtrees:
709-
return plan
710-
# Prepend the shared subtrees; ReferenceRel ordinals index into them.
711-
subtree_rels = [stpl.PlanRel(rel=s) for s in ctx.subtrees]
712-
return stpl.Plan(
713-
version=_plan.default_version,
714-
relations=[*subtree_rels, plan.relations[-1]],
715-
**_plan._merge_extensions(plan, *ctx.plans),
716-
)
717-
718630
def to_plan(self):
719631
"""Materialize to a ``substrait.proto.Plan``."""
720-
return self._materialize(self._registry)
632+
return self._plan(self._registry)
721633

722634
# Kept for parity with the substrait.narwhals (Narwhals) wrapper's API.
723635
def to_substrait(self, registry: Optional[ExtensionRegistry] = None):
724-
return self._materialize(registry or self._registry)
636+
return self._plan(registry or self._registry)
725637

726638

727639
class GroupBy:

src/substrait/type_inference.py

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,6 @@
55
import substrait.plan_pb2 as stp
66
import substrait.type_pb2 as stt
77

8-
# The shared subtrees (Rels) a ReferenceRel's subtree_ordinal indexes into, set
9-
# for the duration of a plan build so a `reference` relation's schema resolves.
10-
reference_subtrees: contextvars.ContextVar = contextvars.ContextVar(
11-
"reference_subtrees", default=None
12-
)
13-
148
# Stack of enclosing-query schemas (NamedStruct) for correlated subqueries, so a
159
# field reference with an OuterReference root resolves against the right level.
1610
# Pushed by the subquery builders while resolving their inner plan.
@@ -535,12 +529,6 @@ def infer_rel_schema(rel: stalg.Rel) -> stt.Type.Struct:
535529
f"{rel.extension_multi.detail.type_url!r}"
536530
)
537531
(common, struct) = (rel.extension_multi.common, derived.struct)
538-
elif rel_type == "reference":
539-
subtrees = reference_subtrees.get()
540-
if subtrees is None:
541-
raise Exception("cannot infer a ReferenceRel's schema outside a plan build")
542-
# ReferenceRel has no common/emit; its schema is the subtree's schema.
543-
return infer_rel_schema(subtrees[rel.reference.subtree_ordinal])
544532
else:
545533
raise Exception(f"Unhandled rel_type {rel_type}")
546534

tests/dataframe/test_frame.py

Lines changed: 0 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -794,57 +794,6 @@ def test_unpivot_requires_on():
794794
_wide_df().unpivot([], index="region")
795795

796796

797-
# -- Phase (core-ext): References / CTEs -----------------------------------
798-
799-
800-
def _find_reference(rel):
801-
kind = rel.WhichOneof("rel_type")
802-
if kind == "reference":
803-
return rel.reference.subtree_ordinal
804-
for field in ("filter", "project", "fetch", "sort"):
805-
if kind == field:
806-
return _find_reference(getattr(rel, field).input)
807-
return None
808-
809-
810-
def test_cache_emits_shared_subtree_referenced_twice():
811-
base = sub.read_named_table("t", {"id": sub.i64, "v": sub.i64}).cache()
812-
plan = (
813-
base.filter(sub.col("id") < 10)
814-
.union(base.filter(sub.col("id") >= 10))
815-
.to_plan()
816-
)
817-
assert [r.WhichOneof("rel_type") for r in plan.relations] == ["rel", "root"]
818-
assert plan.relations[0].rel.HasField("read")
819-
set_inputs = plan.relations[-1].root.input.set.inputs
820-
assert [_find_reference(i) for i in set_inputs] == [0, 0]
821-
822-
823-
def test_no_cache_inlines_single_relation():
824-
a = sub.read_named_table("t", {"id": sub.i64})
825-
plan = a.filter(sub.col("id") > 0).union(a.filter(sub.col("id") < 0)).to_plan()
826-
# Without cache the source is inlined into each union input.
827-
assert len(plan.relations) == 1
828-
for i in plan.relations[-1].root.input.set.inputs:
829-
assert _find_reference(i) is None
830-
831-
832-
def test_cache_schema_inference_through_reference():
833-
# Filtering/selecting the cached frame requires inferring its schema
834-
# through the ReferenceRel.
835-
base = sub.read_named_table("t", {"id": sub.i64, "v": sub.i64}).cache()
836-
plan = base.filter(sub.col("v") > 0).select("id").to_plan()
837-
assert plan.relations[-1].root.input.project.HasField("common")
838-
assert list(plan.relations[-1].root.names) == ["id"]
839-
840-
841-
def test_cache_merges_subtree_extensions():
842-
base = sub.read_named_table("t", {"id": sub.i64}).filter(sub.col("id") > 5).cache()
843-
plan = base.union(base).to_plan()
844-
urns = {u.urn for u in plan.extension_urns}
845-
assert "extension:io.substrait:functions_comparison" in urns
846-
847-
848797
# -- Phase (core-ext): physical joins + exchange --------------------------
849798

850799

@@ -1209,14 +1158,6 @@ def test_mark_join_output_names_match_types():
12091158
assert ns.struct.types[-1].WhichOneof("kind") == "bool"
12101159

12111160

1212-
def test_hint_after_cache_raises_clear_error():
1213-
# Regression: a ReferenceRel (from .cache()) has no RelCommon, so hint() must
1214-
# fail with a clear message rather than an opaque AttributeError.
1215-
df = sub.read_named_table("t", {"a": sub.i64})
1216-
with pytest.raises(TypeError, match="cannot attach a hint"):
1217-
df.cache().hint(row_count=100).to_plan()
1218-
1219-
12201161
def test_default_registry_is_reused():
12211162
assert sub.default_registry() is sub.default_registry()
12221163

0 commit comments

Comments
 (0)