Skip to content

Commit

Permalink
Fix invalid joins when the order by relationship predicate is null
Browse files Browse the repository at this point in the history
  • Loading branch information
daniel-chambers committed Dec 3, 2024
1 parent e3f7a53 commit 1176e12
Show file tree
Hide file tree
Showing 11 changed files with 213 additions and 52 deletions.
41 changes: 21 additions & 20 deletions crates/query-engine/translation/src/translation/query/sorting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
root_table: root_and_current_tables.root_table.clone(),
current_table: join_table,
};

// generate a condition for this join.
let join_condition = relationships::translate_column_mapping(
env,
&root_and_current_tables.current_table,
&predicate_tables.current_table.reference,
sql::helpers::empty_where(),
relationship,
)?;

select.where_ = match predicate {
None => sql::ast::Where(join_condition),
Some(predicate) => {
// generate a condition for the predicate.
let predicate_tables = RootAndCurrentTables {
root_table: root_and_current_tables.root_table.clone(),
current_table: join_table,
};
let predicate_expr = filtering::translate(env, state, &predicate_tables, predicate)?;

// generate a condition for this join.
let join_condition = relationships::translate_column_mapping(
env,
&root_and_current_tables.current_table,
&predicate_tables.current_table.reference,
sql::helpers::empty_where(),
relationship,
)?;

select.where_ = sql::ast::Where(sql::ast::Expression::And {
sql::ast::Where(sql::ast::Expression::And {
left: Box::new(join_condition),
right: Box::new(predicate_expr),
});

Ok(select)
})
}
}
};

Ok(select)
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,12 @@
{
"relationship": "TrackAlbum",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
},
{
"relationship": "AlbumArtist",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,7 @@
{
"relationship": "AlbumArtist",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@
{
"relationship": "ArtistAlbum",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
}
]
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,12 @@
{
"relationship": "CompanyCEO",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
},
{
"relationship": "PersonParent",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@
{
"relationship": "AlbumArtist",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
}
]
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
{
"version": "4",
"metadata": {
"tables": {
"Album": {
"schemaName": "public",
"tableName": "Album",
"columns": {
"AlbumId": {
"name": "AlbumId",
"type": {
"scalarType": "int4"
},
"nullable": "nullable",
"description": null
},
"ArtistId": {
"name": "ArtistId",
"type": {
"scalarType": "int4"
},
"nullable": "nullable",
"description": null
},
"Title": {
"name": "Title",
"type": {
"scalarType": "varchar"
},
"nullable": "nullable",
"description": null
}
},
"uniquenessConstraints": {},
"foreignRelations": {},
"description": null
},
"Artist": {
"schemaName": "public",
"tableName": "Artist",
"columns": {
"ArtistId": {
"name": "ArtistId",
"type": {
"scalarType": "int4"
},
"nullable": "nullable",
"description": null
},
"Name": {
"name": "Name",
"type": {
"scalarType": "varchar"
},
"nullable": "nullable",
"description": null
}
},
"uniquenessConstraints": {},
"foreignRelations": {},
"description": null
}
},
"scalarTypes": {
"int4": {
"typeName": "int4",
"schemaName": "pg_catalog",
"description": null,
"aggregateFunctions": {},
"comparisonOperators": {},
"typeRepresentation": null
},
"varchar": {
"typeName": "varchar",
"schemaName": "pg_catalog",
"description": null,
"aggregateFunctions": {},
"comparisonOperators": {},
"typeRepresentation": null
}
},
"compositeTypes": {},
"nativeQueries": {}
},
"mutationsVersion": null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{
"collection": "Album",
"query": {
"fields": {
"Name": {
"type": "column",
"column": "Title",
"arguments": {}
}
},
"limit": 5,
"offset": 3,
"order_by": {
"elements": [
{
"order_direction": "asc",
"target": {
"type": "column",
"name": "Name",
"path": [
{
"relationship": "AlbumArtist",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
}
]
}
}
]
}
},
"arguments": {},
"collection_relationships": {
"AlbumArtist": {
"column_mapping": {
"ArtistId": "ArtistId"
},
"relationship_type": "object",
"target_collection": "Artist",
"arguments": {}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@
{
"relationship": "ArtistAlbums",
"arguments": {},
"predicate": {
"type": "and",
"expressions": []
}
"predicate": null
}
]
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
---
source: crates/query-engine/translation/tests/tests.rs
expression: result
---
SELECT
coalesce(json_agg(row_to_json("%3_universe")), '[]') AS "universe"
FROM
(
SELECT
*
FROM
(
SELECT
coalesce(json_agg(row_to_json("%4_rows")), '[]') AS "rows"
FROM
(
SELECT
"%0_Album"."Title" AS "Name"
FROM
"public"."Album" AS "%0_Album"
LEFT OUTER JOIN LATERAL (
SELECT
"%1_ORDER_PART_Artist"."Name" AS "Name"
FROM
(
SELECT
"%1_ORDER_PART_Artist"."Name" AS "Name"
FROM
"public"."Artist" AS "%1_ORDER_PART_Artist"
WHERE
(
"%0_Album"."ArtistId" = "%1_ORDER_PART_Artist"."ArtistId"
)
) AS "%1_ORDER_PART_Artist"
) AS "%2_ORDER_FOR_Album" ON ('true')
ORDER BY
"%2_ORDER_FOR_Album"."Name" ASC
LIMIT
5 OFFSET 3
) AS "%4_rows"
) AS "%4_rows"
) AS "%3_universe";

{}
8 changes: 8 additions & 0 deletions crates/query-engine/translation/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,14 @@ async fn sorting_by_relationship_column() {
insta::assert_snapshot!(result);
}

#[tokio::test]
async fn sorting_by_relationship_column_with_true_predicate() {
let result = common::test_translation("sorting_by_relationship_column_with_true_predicate")
.await
.unwrap();
insta::assert_snapshot!(result);
}

#[tokio::test]
async fn sorting_by_nested_relationship_column() {
let result = common::test_translation("sorting_by_nested_relationship_column")
Expand Down

0 comments on commit 1176e12

Please sign in to comment.