Skip to content

Commit

Permalink
Fix some obscure optional bugs in the presence of tuple projections (#…
Browse files Browse the repository at this point in the history
…5610)

When attaching a path in the scope tree, the detection of whether the
*target* node was optional was insufficient. It only checked the node
itself, but needs to check all the way to the factoring point.
I think that logic had forgotten about BRANCHes that weren't FENCEs.

This came up in a branch where I was experimenting with always
BRANCHing function arguments as a way to validate *sometimes*
branching function arguments to produce code that hits ORDER BY
indexes in complex pgvector cases.
  • Loading branch information
msullivan committed Jun 7, 2023
1 parent 8b7ac11 commit 805b7f2
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 2 deletions.
13 changes: 11 additions & 2 deletions edb/ir/scopetree.py
Original file line number Diff line number Diff line change
Expand Up @@ -484,10 +484,13 @@ def attach_subtree(self, node: ScopeTreeNode,

path_id = descendant.path_id.strip_namespace(dns)
visible, visible_finfo, vns = self.find_visible_ex(path_id)
desc_optional = (
descendant.is_optional_upto(node.parent) or self.optional)

if visible is not None:
desc_optional = (
descendant.is_optional_upto(node.parent)
or self.is_optional_upto(visible.parent)
)

if visible_finfo is not None and visible_finfo.factoring_fence:
# This node is already present in the surrounding
# scope and cannot be factored out, such as
Expand Down Expand Up @@ -530,6 +533,12 @@ def attach_subtree(self, node: ScopeTreeNode,
current = descendant
if factorable_nodes:
descendant.strip_path_namespace(dns)
desc_optional = (
descendant.is_optional_upto(node.parent)
# Check if there is an optional branch between here
# and the *highest* factoring point.
or self.is_optional_upto(factorable_nodes[-1][1])
)
if desc_optional:
descendant.mark_as_optional()

Expand Down
77 changes: 77 additions & 0 deletions tests/test_edgeql_coalesce.py
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,83 @@ async def test_edgeql_coalesce_dependent_20(self):
],
)

await self.assert_query_result(
r'''
WITH
I := (
SELECT Issue
FILTER Issue.status.name = 'Open'
)
# `I.time_estimate` is now a LCP
SELECT I.time_estimate ?= (I.time_estimate,).0;
''',
[
True,
],
)

await self.assert_query_result(
r'''
WITH
I := (
SELECT Issue
FILTER Issue.status.name = 'Open'
)
# `I.time_estimate` is now a LCP
SELECT (I.time_estimate,).0 ?= (I.time_estimate,).0;
''',
[
True,
],
)

await self.assert_query_result(
r'''
WITH
I := (
SELECT Issue
FILTER Issue.status.name = 'Open'
)
# `I.time_estimate` is now a LCP
SELECT ((I.time_estimate,).0,).0 ?= (I.time_estimate,).0;
''',
[
True,
],
)

await self.assert_query_result(
r'''
WITH
I := (
SELECT Issue
FILTER Issue.status.name = 'Open'
)
# `I.time_estimate` is now a LCP
SELECT
({I.time_estimate} = 0) ?=
(({I.time_estimate} = 0) = (I.time_estimate = 0));
''',
[
True,
],
)

await self.assert_query_result(
r'''
WITH
I := (
SELECT Issue
FILTER Issue.status.name = 'Open'
)
# `I.time_estimate` is now a LCP
SELECT {I.time_estimate} ?= (I.time_estimate,).0;
''',
[
True,
],
)

async def test_edgeql_coalesce_dependent_21(self):
await self.assert_query_result(
r'''
Expand Down

0 comments on commit 805b7f2

Please sign in to comment.