Skip to content

Change "grouped field set" to be a map of ordered sets #1161

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

Open
wants to merge 2 commits into
base: benjie/incremental-common
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Apr 25, 2025

Extension to #1039
Currently:

type GroupedFieldSet = Map<ResponseName, Array<FieldSelection{ResponseName}>>

I propose that we defined "field set" to be an ordered set (rather than list) of fields that all share the same response name. This makes a lot of the spec clearer, e.g. it's much clearer why ExecuteField() should receive a "field set" (since they all share the same response name) rather than simply receiving "fields".

Essentially I propose:

// Constraint: every FieldSelection in a FieldSet must have the same ResponseName
type FieldSet{ResponseName} = OrderedSet<FieldSelection{ResponseName}>

type GroupedFieldSet = Map<ResponseName, FieldSet{ResponseName}>

This also makes the term "grouped field set" a lot clearer.

It also increases efficiency; consider this query:

query Q {
  animal {
    ...Cat
    ...Dog
  }
  animal {
    ...Cat
    ...Dog
  }
  animal {
    ...Cat
    ...Dog
  }
}
fragment Cat on Animal {
  lives
}
fragment Dog on Animal {
  wagsTail
}

Prior to this change:

  1. ExecuteRootSelectionSet uses CollectFields to produce this grouped field set:
    const rootGroupedFieldSet = {
      "animal": [
        FieldSelection`animal { ...Cat ...Dog }`,
        FieldSelection`animal { ...Cat ...Dog }`,       
        FieldSelection`animal { ...Cat ...Dog }`,
      ]
    }
  2. CompleteValue calls CollectSubfields to produce this grouped field set (assuming Cat):
    const catGroupedFieldSet = {
      "lives": [
        FieldSelection`lives`,
        FieldSelection`lives`,
        FieldSelection`lives`,
      ]
    }
    Note: visitedFragments doesn't prevent this because it doesn't work across sibling selections, only nested selections

If we were to have deeper nesting, this duplication would get much worse. Memory wise, this isn't a big deal, but looping-wise we're looping a lot more than we need to.

By explicitly making this a set, we remove this duplication. The phrase "if not already present" makes it clear that the first field selection "wins", which is important for ordering.

@benjie benjie force-pushed the benjie/incremental-common branch 2 times, most recently from 6e76340 to 3c6dfb3 Compare April 25, 2025 09:01
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.

1 participant