fix nested fields relationships #564
Conversation
soupi
changed the title
| env, | ||
| state, | ||
| fields, | ||
| &nested_field_table_reference, | ||
| nested_field_from, | ||
| &mut join_relationship_fields, |
There was a problem hiding this comment.
we handle and add joins here, instead in the parent.
| -- ,( | ||
| -- 3, | ||
| -- 'University of Nowhere', | ||
| -- null, | ||
| -- null, | ||
| -- ARRAY ['nothing',null], | ||
| -- NULL | ||
| -- ); |
There was a problem hiding this comment.
commenting these out because of bugs but keeping them so it's easier to work on the bug.
| sql.append_syntax("("); | ||
| column.to_sql(sql); | ||
| sql.append_syntax(")"); | ||
| if !columns.is_empty() { |
There was a problem hiding this comment.
This change confused me a lot until I realized it is in a From clause.
Mulling it over a bit I think it's an alright change. But I wonder if it's ever necessary to give a column signature to FROM UNNEST(..). Maybe we could do without it entirely?
There was a problem hiding this comment.
Will have a look at it in the future. I don't remember the exact case right now, but I think we use it for variables or something where we need to specify the names and types.
There was a problem hiding this comment.
And sorry about this, I'm working on a similar ticket and kind of got some changes mixed together.
| env, | ||
| state, | ||
| fields, | ||
| &nested_field_table_reference, | ||
| nested_field_from, | ||
| &mut join_relationship_fields, |
| "%5_nested_field_binding"."address_line_1" AS "address_line_1", | ||
| "%5_nested_field_binding"."address_line_2" AS "address_line_2" | ||
| "%5_make_person.result.address"."address_line_1" AS "address_line_1", | ||
| "%5_make_person.result.address"."address_line_2" AS "address_line_2" |
There was a problem hiding this comment.
Urgh, this is difficult to read.
Information-wise it's much better than the arbitrary name, but the use of . suggests column projection happening in-line and it's only when you look closely that you see it's actually a quoted identifier.
Maybe we should change this scheme to e.g. -> or [<field>] in a follow-up PR.
There was a problem hiding this comment.
I have experimented with a few schemes and thought this was the best. I feel that once you understand what's going on, it becomes fine. We can revisit this is the future :)
| let source = TableSource::NestedField { | ||
| collection_name: collection_name.clone(), | ||
| type_name: nested_field_type_name, | ||
| field_path: FieldPath(field_path), |
There was a problem hiding this comment.
It looks like this lets us build up multiple nested field lookups in one go. Are there examples of this in tests?
There was a problem hiding this comment.
The field_path and collection_name are only used for naming the alias, so even if this isn't perfect, it wouldn't effect the query.
There was a problem hiding this comment.
I think if they're only used as a convenience we should consider making that more obvious in the code. There is a lot of stuff that has happen, so the more we can direct the reader's attention the better.
There was a problem hiding this comment.
Yeah I understand. I did try to make this explicit in the type's comments:
https://github.com/hasura/ndc-postgres/pull/564/files#diff-d24352dcf20dd7c6e0b5e4f0c7dec878e49c311b976280b19008620bb936bb2eR80
But I guess there is a discoverability issue here.
What
Selecting nested field relationships had a bug where it would try to look up a composite type field column in the parent collection. For example:
I have a collection
institutionwith a fieldstaffthat is a composite type that contains the fieldfavorite_artist_id. Trying to have a relationship from this field to theArtisttable would attempt to look-up this field ininstitution, instead of instaff.This PR fixes this.
How
Before, we had a type called
TableNameAndReferencethat describe the metadata name of a table and held a (relationship alias) reference to it.Sourceinstead of aName, which can be a collection name or a nested field name. This is done so it is clearer whether we are tracking a collection or a type.Collectioneverywhere, we lookup /anything/ that can have fields by replacinglookup_collectionwithlookup_fields_info.joinsaccumulator for the parent table, we create another on the fly, and translate the joins using the table created for the nested field instead of the parent table.We also:
v3-engine, so we add a newsqlfile and update the configurations accordingly.