Conversation
445fa89 to
6af4096
Compare
myronmarston
left a comment
There was a problem hiding this comment.
My initial thoughts (I haven't done a full review):
- This seems useful and I like it in general!
- I think I prefer it being named
returnablerather thanfetchablebecausefetchableis named more in terms of how EG interacts with the datastore (whether it "fetches" that field from the datastore or not) where asreturnable, to me, is phrased in terms of the impact on the GraphQL schema: a field which is notreturnableis not available to returned in the response. That's purely subjective, though--thoughts? - Elasticsearch/OpenSearch have a plethora of options we could leverage here--see the "retrieve selected fields from a search" docs for details.
- Notably, it sounds like the
fieldsparameter consults the index mappings rather than_sourceand might allow the same sort of storage savings your looking for while allowing the field to be returnable. - Doc value fields similarly may support returning a field while avoiding its storage costs in
_source. - There's also
stored_fieldswhich I don't fully grok yet.
- Notably, it sounds like the
I suspect there's some deeper, more profound improvement we could make to EG leveraging these features, allowing for greater storage savings, query performance, or both, but I haven't dug deep enough to have an idea of what that would be yet.
If it interests you to dig into that and open a feature proposal I'd be happy to review. But in the meantime I think returnable as a feature is straightforward enough that we can move forward with this.
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Adds a new `fetchable: false` option for field definitions that allows fields to remain filterable, sortable, groupable, and aggregatable while being excluded from the GraphQL output type and from `_source` in the datastore mapping. Changes: - Field struct: new `:fetchable` member with `fetchable?` predicate (defaults true) - fields_sdl: filters out non-fetchable fields from output type SDL - Index#mappings: emits `_source.excludes` for non-fetchable field paths - TypeWithSubfields#non_fetchable_field_paths: collects paths using indexing_fields_by_name_in_index to avoid interface/union recursion cycles - to_filter_field: resets fetchable to nil so filter fields still appear in filter input types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
0c71cf2 to
8e9b12f
Compare
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/field.rb
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/field.rb
Outdated
Show resolved
Hide resolved
...schema_definition/lib/elastic_graph/schema_definition/schema_elements/type_with_subfields.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/index.rb
Outdated
Show resolved
Hide resolved
...schema_definition/lib/elastic_graph/schema_definition/schema_elements/type_with_subfields.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
One other suggestion: for new features like this, it helps give much greater confidence that it works end-to-end if we (1) update the test schema to use it and (2) update the GraphQL acceptance tests to exercise it. However, I don't want to bloat this PR with that, so here's what I'd recommend:
- Stack a PR on top of this one which sets
returnable: falseon at least one field of the widgets schema.- If you can, make that (+ factory updates, if needed) the only change in that PR. The diff on schema artifact updates is quite large and it's nice to not mix it with other meaningful changes.
- Stack a PR on top of that one which runs some GraphQL queries demonstrating that the field can be filtered on, sorted on, grouped on, aggregated, and highlighted, but not returned.
Edit: also, it would be good to cover returnable: false on both a leaf and an object field in these end-to-end tests.
8e9b12f to
bde522d
Compare
Summary
This PR adds a
returnable: falseoption to field definitions in the schema definition DSL.When a field is marked
returnable: false, ElasticGraph:_sourcevia_source.excludesThis supports fields that are useful for query behavior but should not be returned in GraphQL responses, while also reducing stored
_sourcesize for those fields.Implementation
The change is intentionally narrow:
Fieldnow exposes areturnable?predicate with a default oftrue_source.excludesfor non-returnable field pathsreturnableso they still appear in input typesBecause the field remains part of the schema metadata used for non-output GraphQL features, existing generation for filter inputs, sort enums, grouped-by types, aggregated-values types, and highlights continues to work.
Tests
Updated unit specs cover:
returnable: falsefields from GraphQL output types_source.excludesin generated datastore mappingsI was not able to run the RSpec files in this local environment because Bundler is missing required gems/native extensions.