Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dml function in for loops in with blocks. #8306

Merged
merged 1 commit into from
Feb 5, 2025
Merged

Conversation

dnwpark
Copy link
Contributor

@dnwpark dnwpark commented Feb 5, 2025

Given the schema:

type Bar {
    required property a -> int64;
};
function foo(x: int64) -> Bar {
    using ((insert Bar{ a := x }))
};

The query with temp := (for x in {1,2,3} union(foo(x))) select temp.a would produce:

{1, 1, 1, 2, 2, 2, 3, 3, 3}

A similar result would appear if the dml function contained update or delete.

This was caused by two related bugs:

  1. Sets can be compiled twice if they are DML and contained within a for loop within a with block.

For normal DML statements, CompilerContextLevel.dml_stmts is used to track such CTEs and ensure that they are not duplicated. However, for inlined function arguments, this CTE was not tracked and was therefore duplicated and joined multiple times.

The fix was to track the inlined argument CTEs using a newly added CompilerContextLevel.inline_dml_ctes. Unlike the iterator CTEs, these need to be tracked by PathId instead of the Set directly. If the CTE is used in a view, then it may create a copy of the Set with the same path.

  1. Update and Delete did not apply iterator path bond to the statement body.

This was not an issue with explicit DML, as the dml range could be relied on to merge in any iterators. However, this did not join in the new inlined argument CTE.

@dnwpark dnwpark added the to-backport-6.x PRs that *should* be backported to 6.x label Feb 5, 2025
@msullivan
Copy link
Member

Add a PR description

@dnwpark dnwpark merged commit ab3ef0c into master Feb 5, 2025
24 checks passed
@dnwpark dnwpark deleted the with-for-dml-function branch February 5, 2025 21:50
@msullivan msullivan added backported-6.x PRs that *have* been backported to 6.x and removed to-backport-6.x PRs that *should* be backported to 6.x labels Feb 5, 2025
msullivan pushed a commit that referenced this pull request Feb 5, 2025
Given the schema:
```
type Bar {
    required property a -> int64;
};
function foo(x: int64) -> Bar {
    using ((insert Bar{ a := x }))
};
```

The query `with temp := (for x in {1,2,3} union(foo(x))) select temp.a` would produce:
```
{1, 1, 1, 2, 2, 2, 3, 3, 3}
```

A similar result would appear if the dml function contained `update` or `delete`.

This was caused by two related bugs:

1. Sets can be compiled twice if they are DML and contained within a for loop within a with block.

For normal DML statements, `CompilerContextLevel.dml_stmts` is used to track such CTEs and ensure that they are not duplicated. However, for inlined function arguments, this CTE was not tracked and was therefore duplicated and joined multiple times.

The fix was to track the inlined argument CTEs using a newly added `CompilerContextLevel.inline_dml_ctes`. Unlike the iterator CTEs, these need to be tracked by `PathId` instead of the `Set` directly. If the CTE is used in a view, then it may create a copy of the `Set` with the same path.

2. Update and Delete did not apply iterator path bond to the statement body.

This was not an issue with explicit DML, as the dml range could be relied on to merge in any iterators. However, this did not join in the new inlined argument CTE.
deepbuzin pushed a commit that referenced this pull request Feb 18, 2025
Given the schema:
```
type Bar {
    required property a -> int64;
};
function foo(x: int64) -> Bar {
    using ((insert Bar{ a := x }))
};
```

The query `with temp := (for x in {1,2,3} union(foo(x))) select temp.a` would produce:
```
{1, 1, 1, 2, 2, 2, 3, 3, 3}
```

A similar result would appear if the dml function contained `update` or `delete`.

This was caused by two related bugs:

1. Sets can be compiled twice if they are DML and contained within a for loop within a with block.

For normal DML statements, `CompilerContextLevel.dml_stmts` is used to track such CTEs and ensure that they are not duplicated. However, for inlined function arguments, this CTE was not tracked and was therefore duplicated and joined multiple times.

The fix was to track the inlined argument CTEs using a newly added `CompilerContextLevel.inline_dml_ctes`. Unlike the iterator CTEs, these need to be tracked by `PathId` instead of the `Set` directly. If the CTE is used in a view, then it may create a copy of the `Set` with the same path.

2. Update and Delete did not apply iterator path bond to the statement body.

This was not an issue with explicit DML, as the dml range could be relied on to merge in any iterators. However, this did not join in the new inlined argument CTE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-6.x PRs that *have* been backported to 6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants