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 invalid joins when the order by relationship predicate is null #655

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

daniel-chambers
Copy link
Contributor

@daniel-chambers daniel-chambers commented Dec 3, 2024

What

If a relationship used in an order by specified a null predicate, the SQL generator would fail to add the join conditions when joining the related table into the query. This would result in an incorrect cross product between the table and the related table.

How

If the relationship predicate is null, we now correctly add the table's join conditions.

Existing tests that assumed all predicates would contain an always true expression have been changed to contain a null predicate and an extra test has been added to cover receiving an always true predicate (to make sure that still works).

Fixes ENG-1386

@daniel-chambers daniel-chambers self-assigned this Dec 3, 2024
@daniel-chambers daniel-chambers force-pushed the daniel/fix-order-with-null-predicate branch from 6d1c48d to 3f4f926 Compare December 3, 2024 04:56
@@ -776,31 +776,32 @@ fn select_for_path_element(
select.select_list = select_list;
select.from = Some(from_clause);

match predicate {
None => Ok(select),
let predicate_tables = RootAndCurrentTables {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so tl;dr, we were short circuiting too early on None.

@daniel-chambers daniel-chambers added this pull request to the merge queue Dec 3, 2024
Merged via the queue into main with commit 7a5a372 Dec 3, 2024
30 checks passed
@daniel-chambers daniel-chambers deleted the daniel/fix-order-with-null-predicate branch December 3, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants