Skip to content

Conversation

@plcplc
Copy link
Contributor

@plcplc plcplc commented Apr 3, 2024

What

This PR adds support for selecting nested fields that arise from composite objects.

The case for nested fields which are arrays will be treated in a followup PR.

How

  • Handling of field selection is now recursive and lives in its own function.
  • A dedicated case handling nested fields has been added. Each field at each layer of nesting adds another lateral join

The SQL generated is of the form

///   SELECT
///     coalesce(json_agg(row_to_json("%6_rows")), '[]') AS "rows"
///   FROM
///     (
///       SELECT
///         "%3_nested_fields_collect"."collected" AS "result"
///       FROM
///         <current table> AS "%0_<current table>"
///         LEFT OUTER JOIN LATERAL (
///           SELECT
///             ("%0_<current table>"."<composite column>").*
///         ) AS "%2_nested_field_bound" ON ('true')
///         LEFT OUTER JOIN LATERAL (
///           SELECT
///             row_to_json("%4_nested_fields") AS "collected"
///           FROM
///             (
///               SELECT
///                 "%2_nested_field_bound"."<nested column>" AS "<nested field alias>"
///             ) AS "%4_nested_fields"
///         ) AS "%3_nested_fields_collect" ON ('true')
///     ) AS "%6_rows"

@plcplc plcplc enabled auto-merge April 3, 2024 16:15
Copy link
Contributor

@soupi soupi left a comment

Choose a reason for hiding this comment

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

I'm still in the process of reviewing and nothing seems to jump out, but I would appreciate more comments/docstrings on a few places, there are a couple of very large functions that do not have documentation.

Comment on lines 531 to 533
///
/// - `row_to_json` takes a row and converts it to a json object.
/// - `coalesce(<thing>, <otherwise>)` returns `<thing>` if it is not null, and `<otherwise>` if it is null.
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is probably copy paste from the with_default version right? :)

Copy link
Contributor Author

@plcplc plcplc Apr 4, 2024

Choose a reason for hiding this comment

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

Right - I've pruned it more now. Thanks


/// Translate rows part of query to sql ast.
pub fn translate_rows_query(
struct JoinNestedFieldInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

alias: sql::ast::TableAlias,
}

fn transalate_nested_field_joins(joins: Vec<JoinNestedFieldInfo>) -> Vec<sql::ast::Join> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

join_nested_fields: &mut Vec<JoinNestedFieldInfo>,
join_relationship_fields: &mut Vec<relationships::JoinFieldInfo>,
) -> Result<sql::ast::ColumnReference, Error> {
match field {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would really appreciate some comments in-between some code chunks summarizing what they do. Rust can be quite verbose and it gives some direction to reading the code (and also easier to verify the intent matches the code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree! I've added some commentary explaining how translate_nested_field goes about its business.

state: &mut State,
// The table reference the column lives on
current_table: &TableNameAndReference,
// The column to extract nested fields from
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be ///? Does that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems rust does not support /// (or #[doc()] attributes) on function arguments. I've moved these to the main /// block under an # Aruguments heading.

I'm fine either way myself.

@plcplc plcplc requested a review from soupi April 4, 2024 10:05
@plcplc
Copy link
Contributor Author

plcplc commented Apr 4, 2024

@soupi: Thanks for the review. I've tried to improve the documentation :-)

Copy link
Contributor

@soupi soupi left a comment

Choose a reason for hiding this comment

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

I think this makes sense to me. Nice!

@plcplc plcplc added this pull request to the merge queue Apr 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 4, 2024
@plcplc plcplc added this pull request to the merge queue Apr 4, 2024
Merged via the queue into main with commit 9c811bb Apr 4, 2024
@plcplc plcplc deleted the plc/issues/PG-73 branch April 4, 2024 18:31
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