Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Resolves domain base types #402
Resolves domain base types #402
Changes from all commits
b8504d6
bbd9452
3076c7f
79edc5a
54f2349
ec2afb2
3933fcf
c1c9564
6dc1c10
055b919
665fa3f
ec1c3df
6549d91
e930a60
cbe8b99
7e4ee07
12b7aef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about extending
TypeCategory
andTypeDetails
with aDomain
variant and handling the resolution logic into_graphql_type
to keep it consistent with the existing type resolution stuff?The context is the source of truth for the database' state and I'd like to be able to continue using it confidently for things like casting user input via the SQL layer during transpilation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olirice Fair point I was trying to change as little as possible initially but that's been thrown out of the window at this point. It makes sense to include it as there is a decent amount more that can be done with that data. I'll get to it this weekend but if you don't mind just review this outline and correct me if I misunderstood something.
Change the base_type_map to domain instead and pull out all relevant, I'm thinking the oid, schema_oid, name, nullability, and possibly the element_oid. Might add the base_oid to that type to help with traversing the types later.
Change the types to include
Domain
in category type and adding it to the TypeCategory enum. Also create a Domain struct and add it to TypeDetails.Revert the replace functions in
load_sql_context
. Mapping the domain type in the type details function.Finally adding another match block to for
Domain
in the to_graphql_type function.As for the tests, you're happy with them as is so if they pass we're good right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I didn't understand what you're proposing here. Could you explain the goal at a higher level?
was that related to my comment "How about resolve_domains_as_base_type for clarity?"?
if so, I was only referring to the name of the comment directive. The name of the SQL CTE is okay as-is
yep
yep
yep
they looked good on my first pass. I'll re-review when you're ready but if any need to be adjusted I'm happy to do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, just changing the config key.
Type category tends to map to a type off of the context query. Enum to enums, Composite to composites and Table to tables. I was just going to do the same for domain types. I can add the
base_oid
under table_oid. That's all the info I need to get this done for now. I was just thinking ahead for all the other data you might need for domains, like is it nullable or is it an array, etc.