Fields will merge to AST visitor #1824

xuorig · 2018-08-30T15:42:20Z

Converts the FieldsWillMerge validator to a "pure AST" validator, without using the rewrite. Most of the logic is based on the algorithm in graphql-js (https://github.com/graphql/graphql-js/blob/master/src/validation/rules/OverlappingFieldsCanBeMerged.js)

The algorithm is explained through comments in the class.

make the tests pass
refactor / beautify logic
performance optimizations
Fix Fix overlapping fields validator edge case graphql/graphql-js#1497 but here

xuorig · 2018-08-30T20:31:33Z

Preliminary benchmarks:

Master:

Warming up --------------------------------------
validate - introspection
                        29.000  i/100ms
validate - abstract fragments
                        61.000  i/100ms
validate - abstract fragments 2
                        36.000  i/100ms
validate - big query     3.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        278.307  (±14.7%) i/s -      1.363k in   5.065806s
validate - abstract fragments
                        560.752  (±14.6%) i/s -      2.745k in   5.029741s
validate - abstract fragments 2
                        330.814  (±16.0%) i/s -      1.620k in   5.053972s
validate - big query     31.787  (± 9.4%) i/s -    159.000  in   5.054637s

This branch:

Warming up --------------------------------------
validate - introspection
                        35.000  i/100ms
validate - abstract fragments
                        76.000  i/100ms
validate - abstract fragments 2
                        38.000  i/100ms
validate - big query     3.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        337.562  (±12.1%) i/s -      1.680k in   5.069268s
validate - abstract fragments
                        759.022  (±10.4%) i/s -      3.800k in   5.073882s
validate - abstract fragments 2
                        421.319  (± 8.3%) i/s -      2.090k in   5.003639s
validate - big query     38.481  (±10.4%) i/s -    192.000  in   5.046004s

rmosolgo · 2018-08-30T21:15:47Z

(For those keeping score at home, about 25% faster 😎 )

xuorig · 2018-08-31T14:57:48Z

After some caching, hard to say if it's much better, but I think the benches we have don't necessarily require caches so that's probably why.

Warming up --------------------------------------
validate - introspection
                        26.000  i/100ms
validate - abstract fragments
                        71.000  i/100ms
validate - abstract fragments 2
                        39.000  i/100ms
validate - big query     4.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        384.955  (±13.2%) i/s -      1.898k in   5.054420s
validate - abstract fragments
                        778.436  (±14.1%) i/s -      3.834k in   5.086308s
validate - abstract fragments 2
                        415.047  (±14.2%) i/s -      2.028k in   5.013408s
validate - big query     46.101  (±13.0%) i/s -    228.000  in   5.068996s

rmosolgo · 2018-08-31T15:14:02Z

Yeah, I bet "big query" benefited the most, I think it has lots of fragments.

xuorig · 2018-08-31T20:16:56Z

And another bench mark, I think this should be ready for review!

Warming up --------------------------------------
validate - introspection
                        40.000  i/100ms
validate - abstract fragments
                        85.000  i/100ms
validate - abstract fragments 2
                        44.000  i/100ms
validate - big query     4.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        417.224  (± 8.6%) i/s -      2.080k in   5.030352s
validate - abstract fragments
                        835.254  (± 8.7%) i/s -      4.165k in   5.034208s
validate - abstract fragments 2
                        446.764  (±10.5%) i/s -      2.244k in   5.096434s
validate - big query     43.860  (±16.0%) i/s -    216.000  in   5.086208s

rmosolgo · 2018-08-31T20:25:21Z

wow! I will review carefully next week

xuorig · 2018-09-04T15:10:44Z

lib/graphql/static_validation/rules/fields_will_merge.rb

-              context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: node.ast_nodes.to_a)
-            end
+      def fields_and_fragments_from_selection(node, parent_type:)
+        @fields_and_fragments_from_node[node] ||= begin


Fields and Fragment Spreads are collected once per node, iterating over the list only one.

xuorig · 2018-09-04T18:06:15Z

Great context here from graphql-js: graphql/graphql-js#1497 (comment)

Sounds like graphql-js doesn't handle these edge cases, and the spec doesn't seem to either 🤔

xuorig · 2018-09-05T17:02:15Z

After trying a few things, I've just realized that:

        {
          pet {
            ... on Dog {
              ... on Pet {
                name
              }
            }
            ... on Cat {
              name(surname: true)
            }
          }
        }

Used to be valid (Not sure how internal representation used to handle that case), but is not anymore. The algo probably has to maintain lists of parents instead of only the immediate parent of fields, in this case, Pet and Cat are not mutually exclusive (conflict), but looking at the grand parent Dog we should know that they are in fact exclusive. (no conflict)

rmosolgo · 2018-09-05T18:01:33Z

The implementation of that was in InternalRepresentation::Scope, it kept track of which concrete types were allowed at a given point in the AST

rmosolgo · 2018-09-05T18:02:00Z

(I'm surprised there wasn't a test for it!)

xuorig · 2018-09-05T20:58:30Z

With new algorithm, minus caching for now. Not a terrible change performance wise it seems

Warming up --------------------------------------
validate - introspection
                        41.000  i/100ms
validate - abstract fragments
                        81.000  i/100ms
validate - abstract fragments 2
                        44.000  i/100ms
validate - big query     3.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        408.896  (± 4.4%) i/s -      2.050k in   5.023692s
validate - abstract fragments
                        796.939  (± 7.7%) i/s -      3.969k in   5.014707s
validate - abstract fragments 2
                        448.745  (± 7.6%) i/s -      2.244k in   5.035781s
validate - big query     44.255  (± 4.5%) i/s -    222.000  in   5.022948s

rmosolgo · 2018-09-05T21:41:35Z

what a relief, still the same huge improvement over master!

rmosolgo · 2018-09-13T14:51:25Z

shoot, i really screwed up 1.9-dev, let me straighten it out and try to fix this branch.

rmosolgo · 2018-09-13T20:59:55Z

Wow, I spent some time reading the code, thanks again for your amazing work here! 🎊

An interesting take-away for me is how iterative this implementation is. I mean, the comparison really boils down to looping over two sets and comparing each member to each other member. I wouldn't have guessed that it would be so fast, but it sure is. I would have used a Hash, but as shown in any profiler output for GraphQL-Ruby, Kernel#hash is a big time-waster of ours! So maybe I should keep an eye out for applying this more often. Maybe the Ruby VM is faster than the data-structure based approach.

I have an outstanding question, but it doesn't block merging this PR. Let's say we have a query like this:

{
  a { x y } 
  b { x y } 
}

When we enter a, we'll deeply check it for conflicts, including checking its subselections ({x y}) against b's subselections ({x y}). Is that right?

Then, when we enter b, will we also compare its subselections against a's subselections? I see a cache for fragments in a case like this, I wonder if it's worth caching here, too.

Also, I see that @fields_and_fragments_from_node is unused (hopefully not due to my bad git behavior 😩 ) so I will remove it before merging.

xuorig commented Sep 4, 2018

View reviewed changes

xuorig changed the title ~~[WIP] Fields will merge to AST visitor~~ Fields will merge to AST visitor Sep 4, 2018

rmosolgo force-pushed the 1.9-dev branch from bd9961d to 5098895 Compare September 7, 2018 21:09

rmosolgo added this to the 1.9.0 milestone Sep 13, 2018

Marc-Andre Giroux added 13 commits September 13, 2018 15:07

get the simple case working

891bc6a

make subselections work

c291057

one test to fix

f563502

all green

13ba137

linter

6238d03

run rufo

8c12dcf

align things

3060ced

fix loop problem

851ec2f

Use struct to carry field info

bef746b

add comment

a0587bb

compared fragment cache

d56d134

add fragment name and selection cache

af57fb8

more optimizations, fix fragment name issue

95b945c

Marc-Andre Giroux added 7 commits September 13, 2018 15:09

fix test

00792cd

more edge cases

48922ba

failing test for yet another case

980f90e

track all parents

1cdd8d1

exclusivity logic, removed caching for now

94b621e

lets not have infinite loops

81a9de2

unused vars

48bb6ba

rmosolgo force-pushed the fields-will-merge-ast branch from 20d12cb to 48bb6ba Compare September 13, 2018 19:32

Remove Untitled

a28ec6c

Remove unused ivar; remove focus

345ecd4

rmosolgo merged commit cbcac61 into rmosolgo:1.9-dev Sep 13, 2018

rmosolgo mentioned this pull request Aug 6, 2020

Cannot use interface type for possible types of union types #3063

Closed

Thompson1985 approved these changes Oct 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fields will merge to AST visitor #1824

Fields will merge to AST visitor #1824

Uh oh!

xuorig commented Aug 30, 2018 •

edited

Loading

Uh oh!

xuorig commented Aug 30, 2018 •

edited

Loading

Uh oh!

rmosolgo commented Aug 30, 2018

Uh oh!

xuorig commented Aug 31, 2018

Uh oh!

rmosolgo commented Aug 31, 2018

Uh oh!

xuorig commented Aug 31, 2018

Uh oh!

rmosolgo commented Aug 31, 2018

Uh oh!

xuorig Sep 4, 2018

Uh oh!

xuorig commented Sep 4, 2018

Uh oh!

xuorig commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 5, 2018

Uh oh!

xuorig commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 13, 2018

Uh oh!

rmosolgo commented Sep 13, 2018

Uh oh!

Uh oh!

Fields will merge to AST visitor #1824

Fields will merge to AST visitor #1824

Uh oh!

Conversation

xuorig commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuorig commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmosolgo commented Aug 30, 2018

Uh oh!

xuorig commented Aug 31, 2018

Uh oh!

rmosolgo commented Aug 31, 2018

Uh oh!

xuorig commented Aug 31, 2018

Uh oh!

rmosolgo commented Aug 31, 2018

Uh oh!

xuorig Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

xuorig commented Sep 4, 2018

Uh oh!

xuorig commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 5, 2018

Uh oh!

xuorig commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 5, 2018

Uh oh!

rmosolgo commented Sep 13, 2018

Uh oh!

rmosolgo commented Sep 13, 2018

Uh oh!

Uh oh!

xuorig commented Aug 30, 2018 •

edited

Loading

xuorig commented Aug 30, 2018 •

edited

Loading