improve overlapping fields performance #2495
Conversation
| GraphQLType graphQLType = inlineFragment.getTypeCondition() != null | ||
| ? TypeFromAST.getTypeFromAST(getValidationContext().getSchema(), inlineFragment.getTypeCondition()) | ||
| : parentType; | ||
| collectFields(fieldMap, inlineFragment.getSelectionSet(), graphQLType, visitedFragmentSpreads); |
There was a problem hiding this comment.
GraphQLType graphQLType = TypeFromAST.getTypeFromAST(getValidationContext().getSchema(),
fragment.getTypeCondition());
GraphQLType graphQLType = inlineFragment.getTypeCondition() != null
? TypeFromAST.getTypeFromAST(getValidationContext().getSchema(), inlineFragment.getTypeCondition())
: parentType;
The above repeated code deserves a method
There was a problem hiding this comment.
done ...
just for the record: this is the same code as before: I didn't touch this part for this PR.
On a general note: this is very old code. In general there is a huge overlap in logic between FieldCollector, OverlappingField validation and the normalized field factory.
| public FieldPair(Field field1, Field field2) { | ||
| this.field1 = field1; | ||
| this.field2 = field2; | ||
| return field != null ? field.equals(that.field) : that.field == null; |
There was a problem hiding this comment.
This .equals some suspect.
I mean you may only ever compared them into terms of AST Field equality - but the class itself has 3 fields
I just want to make sure this is intended
There was a problem hiding this comment.
Yes that is intended: the AST field is unique and actually determines the types, meaning we only need to compare the fields.
| // return changeNode(context, node.transform(builder -> builder.value(BigInteger.valueOf(intValueCounter.getAndIncrement())))); | ||
| // } | ||
| // | ||
|
|
There was a problem hiding this comment.
deleting commented out code .. not directly related to this PR
andimarek
added
the
performance
This is a rewrite of the overlapping field can be merged validation, which is quite slow for very large queries.
It is based on the article https://tech.xing.com/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 by @SimonAdameit. Thanks a lot @SimonAdameit for this!
For reference: it is basically the algorithm shown in Listing 10 adapted to the GraphQL Java codebase. I added some basic caching, but it is not equal to Listing 11 in the article.
Performance improvements (on my laptop):
350.048 ± 27.135 ms/op average time vs 35.927 ± 2.913 ms/op
for OverlappingFieldValidationBenchmark.