Skip to content
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

Sql optionals #528

Merged
merged 11 commits into from
Sep 4, 2019
Merged

Sql optionals #528

merged 11 commits into from
Sep 4, 2019

Conversation

bojanserafimov
Copy link
Collaborator

@bojanserafimov bojanserafimov commented Aug 28, 2019

Implemented:

  • tag in optional scope
  • filter in optional scope
  • mandatory traverse in optional scope

todo:

  • Remove the hack (abusing python features)
  • Rewrite ([FeedingEvent_3].uuid IS NOT NULL) = 0 as [FeedingEvent_3].uuid IS NULL

@bojanserafimov bojanserafimov marked this pull request as ready for review August 28, 2019 15:44
@bojanserafimov
Copy link
Collaborator Author

This is a working solution with 2 hacks. Before I address them, I want to first see where the @recurse logic fits in, to avoid going back and forth undoing my work.

If you think these hacks should be addressed before this PR merges, I'll come back to it after I know where the rest of the pieces in the SQL backend fit in.

@obi1kenobi
Copy link
Contributor

Assuming #531 should merge first, and then we work on removing the hacks here?

@obi1kenobi
Copy link
Contributor

Takeaways from in-person conversation:

  • Let's ignore the lowering of ([FeedingEvent_1].uuid IS NOT NULL) = 0 into [FeedingEvent_1].uuid IS NULL for now, and just open an "enhancement" issue describing what needs to be done.
  • Let's remove the came_from hack and use a dict mapping aliases to came_from values. We might need to pass additional info to to_sql() methods as a result.

After those two steps, this is ready to merge.

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! A few suggestions to improve the maintainability of the code.

graphql_compiler/compiler/emit_sql.py Show resolved Hide resolved
graphql_compiler/compiler/ir_lowering_sql/__init__.py Outdated Show resolved Hide resolved
graphql_compiler/compiler/ir_lowering_sql/__init__.py Outdated Show resolved Hide resolved
graphql_compiler/compiler/ir_lowering_sql/__init__.py Outdated Show resolved Hide resolved
graphql_compiler/compiler/ir_lowering_sql/__init__.py Outdated Show resolved Hide resolved
graphql_compiler/tests/test_compiler.py Outdated Show resolved Hide resolved
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.

None yet

2 participants