Skip to content

Commit

Permalink
Do not corrupt cardinality inference results when doing group analysis (
Browse files Browse the repository at this point in the history
#5611)

Group aggregate use analysis makes calls to card inference... which can
side effect the IR and the schema! Add a flag to disable this.

The other option I considered was making a deep copy of the IR before
doing this analysis, which doesn't seem wonderful.
  • Loading branch information
msullivan committed Jun 7, 2023
1 parent f7a7b50 commit 8b7ac11
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 13 deletions.
2 changes: 2 additions & 0 deletions edb/edgeql/compiler/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ def __init__(
self.infctx = inference.make_ctx(ctx.env)._replace(
singletons=frozenset({target}),
ignore_computed_cards=True,
# Don't update the IR with the results!
make_updates=False,
)

def visit_Stmt(self, stmt: irast.Stmt) -> Any:
Expand Down
34 changes: 21 additions & 13 deletions edb/edgeql/compiler/inference/cardinality.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,10 @@ def _infer_pointer_cardinality(
required, card = ptr_card.to_schema_value()
env.schema = ptrcls.set_field_value(env.schema, 'cardinality', card)
env.schema = ptrcls.set_field_value(env.schema, 'required', required)
_update_cardinality_in_derived(ptrcls, env=ctx.env)
if ctx.make_updates:
_update_cardinality_in_derived(ptrcls, env=ctx.env)

if ptrref:
if ptrref and ctx.make_updates:
out_card, in_card = typeutils.cardinality_from_ptrcls(
env.schema, ptrcls)
assert in_card is not None
Expand Down Expand Up @@ -690,11 +691,12 @@ def __infer_func_call(
arg_cards = []

for arg, typemod in zip(ir.args, ir.params_typemods):
arg.cardinality = infer_cardinality(
arg.expr, scope_tree=scope_tree, ctx=ctx)

card = infer_cardinality(arg.expr, scope_tree=scope_tree, ctx=ctx)
if typemod is not qltypes.TypeModifier.OptionalType:
arg_cards.append(arg.cardinality)
arg_cards.append(card)

if ctx.make_updates:
arg.cardinality = card

arg_card = zip(*(_card_to_bounds(card) for card in arg_cards))
arg_lower, arg_upper = arg_card
Expand Down Expand Up @@ -725,16 +727,17 @@ def __infer_func_call(
all_singletons = True

for arg, typemod in zip(ir.args, ir.params_typemods):
arg.cardinality = infer_cardinality(
arg.expr, scope_tree=scope_tree, ctx=ctx)
card = infer_cardinality(arg.expr, scope_tree=scope_tree, ctx=ctx)
if typemod is not qltypes.TypeModifier.SetOfType:
non_aggregate_args.append(arg.expr)
non_aggregate_arg_cards.append(arg.cardinality)
non_aggregate_arg_cards.append(card)
if typemod is qltypes.TypeModifier.SingletonType:
singleton_args.append(arg.expr)
singleton_arg_cards.append(arg.cardinality)
singleton_arg_cards.append(card)
else:
all_singletons = False
if ctx.make_updates:
arg.cardinality = card

if non_aggregate_args:
_check_op_volatility(
Expand All @@ -761,9 +764,10 @@ def __infer_oper_call(
) -> qltypes.Cardinality:
cards = []
for arg in ir.args:
arg.cardinality = infer_cardinality(
arg.expr, scope_tree=scope_tree, ctx=ctx)
cards.append(arg.cardinality)
card = infer_cardinality(arg.expr, scope_tree=scope_tree, ctx=ctx)
cards.append(card)
if ctx.make_updates:
arg.cardinality = card

if str(ir.func_shortname) == 'std::UNION':
# UNION needs to "add up" cardinalities.
Expand Down Expand Up @@ -1100,6 +1104,8 @@ def _infer_matset_cardinality(
) -> None:
if not materialized_sets:
return
if not ctx.make_updates:
return

for mat_set in materialized_sets.values():
if (len(mat_set.uses) <= 1
Expand All @@ -1121,6 +1127,8 @@ def _infer_dml_check_cardinality(
scope_tree: irast.ScopeTreeNode,
ctx: inference_context.InfCtx,
) -> None:
if not ctx.make_updates:
return
pctx = ctx._replace(singletons=ctx.singletons | {ir.result.path_id})
for read_pol in ir.read_policies.values():
read_pol.cardinality = infer_cardinality(
Expand Down
5 changes: 5 additions & 0 deletions edb/edgeql/compiler/inference/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ class InfCtx(NamedTuple):
singletons: FrozenSet[irast.PathId]
distinct_iterator: Optional[irast.PathId]
ignore_computed_cards: bool
# Whether to make updates to the cardinality fields in the IR/schema.
# This is used in cases where we need to do a "hypothetical"
# inference, but don't want to affect real state.
make_updates: bool


def make_ctx(env: context.Environment) -> InfCtx:
Expand All @@ -77,4 +81,5 @@ def make_ctx(env: context.Environment) -> InfCtx:
singletons=frozenset(env.singletons),
distinct_iterator=None,
ignore_computed_cards=False,
make_updates=True,
)
2 changes: 2 additions & 0 deletions edb/edgeql/compiler/inference/multiplicity.py
Original file line number Diff line number Diff line change
Expand Up @@ -873,6 +873,8 @@ def infer_multiplicity(
scope_tree: irast.ScopeTreeNode,
ctx: inf_ctx.InfCtx,
) -> inf_ctx.MultiplicityInfo:
assert ctx.make_updates, (
"multiplicity inference hasn't implemented make_updates=False yet")

result = ctx.inferred_multiplicity.get(
(ir, scope_tree, ctx.distinct_iterator))
Expand Down

0 comments on commit 8b7ac11

Please sign in to comment.