Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Add query planning typedefs #1013

Merged
merged 9 commits into from
Apr 26, 2021
Merged

Add query planning typedefs #1013

merged 9 commits into from
Apr 26, 2021

Conversation

chewselene
Copy link
Collaborator

Adds basic query planning type definitions including SimpleExecute.

Part of the refactor in #1008

Comment on lines 58 to 66
Given a scalar type dictionary and an AST node describing a type, return a GraphQLType
definition, which applies to that type. For example, if provided the parsed AST node for
`[Date]`, a GraphQLList instance will be returned, containing the type called
"Date" found in the scalar type dictionary. If a type called "Date" is not found in the scalar
type dictionary, then None will be returned.

Note: this is very similar to GraphQL's type_from_ast. However, instead of requiring a GraphQL
schema this function requires a dictionary of the scalar types. This simplifies deserialization
and allows for custom scalar types without constructing an entire schema.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

copying over this comment on #1008 from @obi1kenobi

Are we going to have issues with this approach if a provider wants to add some custom scalar types that the compiler doesn't natively support?

Not necessarily a deal-breaker, just want to make sure we consider the costs and benefits and explicitly choose the tradeoffs that are best here.

I think that yes, we will have an issue for custom scalar types not in the compiler. However, I'm not sure what the alternative is since we don't want deserialization to be dependent on the schema. Open to suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just document it for now as a limitation of the current approach, and revisit when we need more custom scalar types?

@chewselene chewselene requested a review from obi1kenobi April 26, 2021 14:10
Copy link
Contributor

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of nits.

chewselene and others added 4 commits April 26, 2021 13:04
Co-authored-by: Predrag Gruevski <[email protected]>
Co-authored-by: Predrag Gruevski <[email protected]>
Co-authored-by: Predrag Gruevski <[email protected]>
@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #1013 (442c8f4) into main (0cb05d4) will decrease coverage by 1.12%.
The diff coverage is 0.00%.

❗ Current head 442c8f4 differs from pull request most recent head c0b0ca7. Consider uploading reports for the commit c0b0ca7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1013      +/-   ##
==========================================
- Coverage   94.38%   93.25%   -1.13%     
==========================================
  Files         113      114       +1     
  Lines        9088     9198     +110     
==========================================
  Hits         8578     8578              
- Misses        510      620     +110     
Impacted Files Coverage Δ
graphql_compiler/query_planning/typedefs.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cb05d4...c0b0ca7. Read the comment docs.

Copy link
Contributor

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

LGTM!

@chewselene chewselene merged commit 93d2cf3 into main Apr 26, 2021
@chewselene chewselene deleted the query_planning_typedefs branch April 26, 2021 17:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants