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

Introspection query normalization via apollo-rs #43

Closed
o0Ignition0o opened this issue Oct 18, 2021 · 9 comments
Closed

Introspection query normalization via apollo-rs #43

o0Ignition0o opened this issue Oct 18, 2021 · 9 comments

Comments

@o0Ignition0o
Copy link
Contributor

We currently don't run any normalization, which means the router won't be able to hit it's caches (or NaiveIntrospection alltogether) for queries that have different indents.

@o0Ignition0o o0Ignition0o self-assigned this Oct 18, 2021
@Geal
Copy link
Contributor

Geal commented Oct 18, 2021

could we parse the query with apollo-rs and hash the result?

@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Oct 18, 2021

We might wanna double check with @lrlna, if I got their presentation right apollo rs keeps all of the tokens around, so I'm not sure this would suffice.

OTOH I cannot find any normalization step happening on queries in federation, (poke @abernix maybe you know where/if it happens ?)

I think we can start by hashing the parsed result, and we can then iterate on it and normalize the query before we parse it via apollo-rs!

@lrlna
Copy link
Member

lrlna commented Oct 18, 2021

@o0Ignition0o yes, the AST is lossless: i.e. we keep all the whitespace and other ignored tokens. Eventually, once we have semantic analysis in place, you should be able to store the schema object which would mark ignored tokens as insignificant, therefore two queries that only differ in whitespace would be identical. This is, however, a few months out.

On the other hand, if a query has changed, even if it's just whitespace, how do you currently find out if that's the only thing that has changed, therefore is syntactically identical to what's already in a cache? Don't you have to do a brand new parse anyways?

@o0Ignition0o
Copy link
Contributor Author

I suspect we would have to indeed :(

(I just found the conversation we had on proj-router about that, and it seems to be what stripIgnoredCharacters does)

If a couple of \n and whitespaces strip is good enough we might be able to do that, otherwise i'm not sure that's too useful for our caching usecase

@Geal
Copy link
Contributor

Geal commented Oct 18, 2021

On the other hand, if a query has changed, even if it's just whitespace, how do you currently find out if that's the only thing that has changed, therefore is syntactically identical to what's already in a cache? Don't you have to do a brand new parse anyways?

My (untested) assumption is that query planning and introspection could cost a lot more than the parsing and normalization step

@abernix
Copy link
Member

abernix commented Oct 18, 2021

For caches right now I believe we just use a hash (SHA-256, perhaps?) of the string of the raw operation sent by the client. (I also don't know that variations in ignored tokens — e.g., spaces, etc. — are particularly prevalent in the wild right now or something we need to optimize for.

@abernix
Copy link
Member

abernix commented Oct 18, 2021

(Of course, we can eventually and there are a variety of reasons to do so, particularly as the weight of what we cache increases!)

@abernix
Copy link
Member

abernix commented Oct 18, 2021

(Continuing my parentheticals, rather than just editing the previous post): I should also note that the current cache store for these things are all bound LRU caches.

@o0Ignition0o o0Ignition0o transferred this issue from another repository Nov 4, 2021
@abernix abernix changed the title Query normalization Query normalization via apollo-rs Feb 14, 2022
@abernix abernix changed the title Query normalization via apollo-rs Introspection query normalization via apollo-rs Mar 16, 2022
@o0Ignition0o o0Ignition0o removed their assignment Jul 25, 2022
@abernix
Copy link
Member

abernix commented Dec 4, 2023

Closing as I know our more-practical nera-term answer this is going be "do introspection in Rust", which is in-progress thanks to @SimonSapin in apollographql/apollo-rs#758.

@abernix abernix closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2023
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

No branches or pull requests

4 participants