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

docs: add noIndex:true docs for experimental query planning mode #5883

Closed
wants to merge 16 commits into from

Conversation

lrlna
Copy link
Member

@lrlna lrlna commented Aug 26, 2024

Draft of the docs for experimental query planning mode. @chandrikas, @shorgi let me know what you two think!

@lrlna lrlna requested a review from chandrikas August 26, 2024 14:59
@lrlna lrlna requested a review from a team as a code owner August 26, 2024 14:59
@lrlna lrlna self-assigned this Aug 26, 2024
Copy link
Contributor

@lrlna, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

unsupported features are detected, the router falls back to legacy with an
`info` log.
* `legacy`. Enables only the legacy query planner.
* `both`. Enables comparison between legacy and new native query planners.
Copy link
Contributor

Choose a reason for hiding this comment

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

The difference between both and both_best_effort is not clear to me? Why even have the both option around?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it that both won't fall back to legacy-only if unsupported features are detected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the both won't fall back. We only just internally introduced both_best_effort in the last week specifically for us to be able to run this canary testing for all deployments. I can remove both_best_effort (or I guess both) from this list if you think it's too confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove both. I don't see why anybody would care about falling back to legacy_only if the native planner is running in shadow mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok sounds good!

unsupported features are detected, the router falls back to legacy with an
`info` log.
* `legacy`. Enables only the legacy query planner.
* `both`. Enables comparison between legacy and new native query planners.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it that both won't fall back to legacy-only if unsupported features are detected?

@lrlna
Copy link
Member Author

lrlna commented Aug 27, 2024

Thank you both! All suggestions have been applied. If you think it's less confusing to have mention of only one of the both supported modes, I'll remove the mention of both.

@lrlna lrlna requested a review from chandrikas August 28, 2024 07:02
@lrlna lrlna enabled auto-merge (squash) August 28, 2024 07:02
@lrlna lrlna requested a review from a team August 28, 2024 07:06
@lrlna lrlna changed the base branch from dev to 1.53.0 August 28, 2024 08:27
@lrlna lrlna changed the base branch from 1.53.0 to dev August 28, 2024 08:29
dariuszkuc and others added 5 commits August 28, 2024 11:00
When adding `__typename` field for abstract types, we need to check for inline fragment type condition and not the parent selection set. If we have an inline fragment without type condition and it is pointing to and abstract type, then the required `__typename` field should have already been added to its parent field.

We continue adding `__typename` for fragments with a type condition as their type condition _may_ be different than the parent type.

```graphql
foo {  
  # if this is abstract type we will add __typename
  a
  ... @Skip(if: false) {  
    # if this fragment is on abstract type we don't need to add anything as
    # we already added the __typename to the parent selection
    b
  }
}
```
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Iryna Shestak <[email protected]>
@lrlna lrlna changed the base branch from dev to 1.53.0 August 28, 2024 09:00
@lrlna lrlna force-pushed the lrlna/both-qp-docs branch from e64c941 to 63fbf8a Compare August 28, 2024 09:03
@lrlna
Copy link
Member Author

lrlna commented Aug 28, 2024

i messed up the changing the base branch, and don't want to spend too much time on trying to fix it, so this PR has moved to #5904

@lrlna lrlna closed this Aug 28, 2024
auto-merge was automatically disabled August 28, 2024 09:09

Pull request was closed

@abernix abernix deleted the lrlna/both-qp-docs branch January 20, 2025 15:18
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.