Removed parallel execution requirement for merging fields#985
Removed parallel execution requirement for merging fields#985rivantsov wants to merge 2 commits intographql:mainfrom rivantsov:P24_MergeParallel
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| **Merging Selection Sets** | ||
|
|
||
| When more than one field of the same name is executed in parallel, their | ||
| When multiple fields with the same name or alias are executed, their |
There was a problem hiding this comment.
"multiple" is less precise; for example:
consisting of, having, or involving several or many individuals, parts, elements, relations, etc.; manifold.
being more than two but fewer than many in number or kind:
constituting or forming a large number; numerous:
In the above definitions, "multiple" can be construed to mean any number 3 or larger. I, personally, view "multiple" as meaning 2 or more (but very strange to be used with 2); however in a spec we must be precise and not chose ambiguous language.
Please revert this change.
There was a problem hiding this comment.
The main point here is removing 'in PARALLEL', I can rephrase the 'multiple' part no problem
There was a problem hiding this comment.
also, instead of bringing in the alias ... we should just refer to response name as we do in the field merging algorithm.
There was a problem hiding this comment.
Co-authored-by: Michael Staib <[email protected]>
|
Just a question. About proper English:
is it plural or singular?! fields are many, so it's plural. But IS executed? singular. I am confused :( |
|
I believe "is" is correct English here, because the noun is "field" (singular) not "fields" (plural). English is weird. |
|
yeah, that feels weird, and that's what pushed me to switch to 'multiple fields' in the first place; non-ambiguous PLURAL |
|
Regarding using 'response name'. There are 4 occurrences in the spec of 'response name', none of them defines it, only use it as already known. There is a term 'response key' explained and kind of defined here in 2.7 Field Alias: maybe we should think about switching to 'response key', here and in 4 other locations. |
|
@rivantsov I also was thinking on this. |
|
Just checked ... graphql-js as the reference implementation only uses function executeFieldsSerially(
exeContext: ExecutionContext,
parentType: GraphQLObjectType,
sourceValue: unknown,
path: Path | undefined,
fields: Map<string, ReadonlyArray<FieldNode>>,
): PromiseOrValue<ObjMap<unknown>> {
return promiseReduce(
fields.entries(),
(results, [responseName, fieldNodes]) => {
const fieldPath = addPath(path, responseName, parentType.name);
const result = executeField(
exeContext,
parentType,
sourceValue,
fieldNodes,
fieldPath,
);
if (result === undefined) {
return results;
}
if (isPromise(result)) {
return result.then((resolvedResult) => {
results[responseName] = resolvedResult;
return results;
});
}
results[responseName] = result;
return results;
},
Object.create(null),
);
}above the serial execution of graphql-js. The term responseKey is not used on graphql-js. Same goes for HotChocolate, which also only uses responseName. I will do another PR getting this fixed in the spec. Will also add a note. |
mjmahone
left a comment
There was a problem hiding this comment.
Needs to also encompass line 747. The original wording is not accurate enough, but we should make sure we're building up context.
|
Looking at the broader context, parallel here means fields that can be independently computed, as used above, in https://spec.graphql.org/draft/#sec-Executing-Selection-Sets. As in, they may be executed in parallel, as they have no serial dependency.
This could instead change to: Additionally, we could maybe add a note: But this whole section does not make sense except in the context of section 6.2 and 6.3, which define the difference between serial and parallel execution. I cannot easily come up with a better concept for what we're describing besides parallel field execution. |
|
I think there's some confusion here. The original text gives a (wrong) idea that merge/not merge may depend on parallel/serial execution mode chosen by the server - which it free to choose. I asked at the meeting, "The merge/no merge decision does NOT depend on execution order, right? " - just to state it clearly, before we discuss wordings. And we all agreed, YES. Parallel or serial - it is MERGED field in the output |
|
I understand you try to use 'if can be done in parallel' as criteria for 'should be merged', but I think this brings more confusion. I think parallel should be taken out completely. I do agree that 'parallel fields' in the next sentence should also be taken away |
|
I looked at other places where Field merge is discussed and I think there's more wordings to fix there. I am closing this PR to open a discussion first and then a PR. |
The main point here is to remove 'when executed in PARALLEL'; service is free to execute the request 'normally' - in any order it wants, parallel or not. But I think the field merge process should always apply.
As for the final text - suggestions are welcome