Skip to content

Commit

Permalink
Fix issues with empty sets leaking out of optional scopes
Browse files Browse the repository at this point in the history
For the previous issues, see #5575 and #6526.

The issue here is that in queries like
```
select User filter .avatar ?= <Card>{} and .name = 'Bob'
```
we were returning an empty set, since we were deciding that the
optional wraper we grabbed `.avatar` in was also a reasonable source
for User itself. We have machinery to prevent that (introduced in
the mask of the statement that was a "tentative container" for the
relation, but in this case, that container statement was not being
used, so the masks had no effect.

Instead, set up the masks on the final main rvar before including it.

Fixes #6739.
  • Loading branch information
msullivan committed Jan 30, 2024
1 parent 457bfc8 commit 48cd14b
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 3 deletions.
15 changes: 12 additions & 3 deletions edb/pgsql/compiler/relctx.py
Original file line number Diff line number Diff line change
Expand Up @@ -792,9 +792,18 @@ def update_scope(
assert p.path_id is not None
ctx.path_scope[p.path_id] = stmt


def update_scope_masks(
ir_set: irast.Set, rvar: pgast.PathRangeVar, *,
ctx: context.CompilerContextLevel) -> None:

if not isinstance(rvar, pgast.RangeSubselect):
return
stmt = rvar.subquery

# Mark any paths under the scope tree as masked, so that they
# won't get picked up by pull_path_namespace.
for child_path in scope_tree.get_all_paths():
for child_path in ctx.scope_tree.get_all_paths():
pathctx.put_path_id_mask(stmt, child_path)

# If this is an optional scope node, we need to be certain that
Expand All @@ -807,11 +816,11 @@ def update_scope(
# with some DML linkprop cases (probably easy to fix) and a number
# of materialization cases (possibly hard to fix), so I'm going
# with a more conservative approach.
if scope_tree.is_optional(ir_set.path_id):
if ctx.scope_tree.is_optional(ir_set.path_id):
# Since compilation is done, anything visible to us *will* be
# up on the spine. Anything tucked away under a node must have
# been pulled up.
for anc in scope_tree.ancestors:
for anc in ctx.scope_tree.ancestors:
for direct_child in anc.path_children:
if not direct_child.optional:
pathctx.put_path_id_mask(stmt, direct_child.path_id)
Expand Down
1 change: 1 addition & 0 deletions edb/pgsql/compiler/relgen.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ def get_set_rvar(
null_query, (pgast.SelectStmt, pgast.NullRelation))
null_query.where_clause = pgast.BooleanConstant(val=False)

relctx.update_scope_masks(ir_set, rvars.main.rvar, ctx=subctx)
result_rvar = _include_rvars(rvars, scope_stmt=scope_stmt, ctx=subctx)
for aspect in rvars.main.aspects:
pathctx.put_path_rvar_if_not_exists(
Expand Down
19 changes: 19 additions & 0 deletions tests/test_edgeql_scope.py
Original file line number Diff line number Diff line change
Expand Up @@ -4112,3 +4112,22 @@ async def test_edgeql_scope_linkprop_assert_03(self):
}
],
)

async def test_edgeql_scope_filter_qeq_01(self):
await self.assert_query_result(
r'''
select User filter .avatar ?= <Card>{} and .name = 'Bob';
''',
[
{},
],
)

await self.assert_query_result(
r'''
select User filter .name = 'Bob' and .avatar ?= <Card>{}
''',
[
{},
],
)

0 comments on commit 48cd14b

Please sign in to comment.