Schema support for inherited indexes#1067
Conversation
Allow concrete types to inherit index definitions from parent abstract types (unions/interfaces). Types can now be indexed without defining their own index by inheriting from an indexed parent, enabling mixed-type indices where multiple types share a single datastore index. Key changes: - Add own_or_inherited_index_def() to resolve inherited indices from parent types - Add inherits_index?() predicate for types in mixed-type indices - Add recursively_resolve_supertypes() to traverse type hierarchy - Add requires_typename_for_mixed_index runtime metadata field - Inject __typename into data_params for mixed-type indices - Add comprehensive test coverage including transitive inheritance Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When an abstract type (union/interface) has an index, concrete subtypes inherit that index. The event envelope type enum should include these concrete types (which will actually be indexed) rather than just types with their own direct index definition. Changes: - Use own_or_inherited_index_def instead of root_document_type? to determine which types belong in the JSON schema type enum - Rename variable from root_document_type_names to indexed_type_names for clarity - Add test coverage for event envelope type enum with transitive indexing, including a mixed scenario where some subtypes inherit an index and others have their own index
myronmarston
left a comment
There was a problem hiding this comment.
I'm not done with my review (notably, I haven't reviewed the specs yet) but I wanted to submit what I have so far.
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
...ticgraph-schema_artifacts/lib/elastic_graph/schema_artifacts/runtime_metadata/object_type.rb
Outdated
Show resolved
Hide resolved
...icgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/object_type.rb
Show resolved
Hide resolved
...ticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/union_type.rb
Outdated
Show resolved
Hide resolved
...cgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/implements_interfaces.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
...raph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/interface_type.rb
Outdated
Show resolved
Hide resolved
…chema-definition-artifacts-infrastructure
This commit addresses all review feedback on PR block#1067 and makes several improvements to the index inheritance infrastructure: Semantic method separation: - root_document_type? now means "is this type indexed?" (own or inherited) - directly_queryable? now means "does this type have Query fields?" - Updated all callers to use the appropriate method - Query field generation uses directly_queryable? - Apollo @key directive uses root_document_type? (entities need indexing, not Query fields) Method renaming for clarity: - own_or_inherited_index_def → index_def - inherits_index? → needs_typename? (later inlined) - Removed unnecessary aliases Set-based supertypes: - recursively_resolve_supertypes now returns Set instead of Array - Automatic deduplication, semantically correct - Union memberships tracked as Sets, no need for .to_a conversion Consolidated recursively_resolve_supertypes: - Single implementation in ImplementsInterfaces mixin - Handles both union memberships and interface ancestors - Removed overrides from ObjectType and InterfaceType - UnionType override remains (returns Set[], has no supertypes) __typename handling improvements: - Added indexing_fields_by_name_in_index override in ObjectType - __typename automatically included for types in mixed-type indices - Flows naturally into data_params without manual addition - Removed requires_typename_for_mixed_index runtime metadata field - Inlined needs_typename? logic (only one caller) Code cleanup: - Removed dead root_document_type? methods from enum/scalar/type_with_subfields - Removed Velocity from Attribute union in test schema (mixed indexed/non-indexed) - Simplified EntityTypeExtension overrides All changes maintain 100% test coverage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Consolidated tests from index_inheritance_spec.rb into appropriate existing spec files for better test organization: - Moved concrete type inheritance tests to index_definition_names_spec.rb - Moved transitive interface inheritance test to index_definition_names_spec.rb - Moved __typename data_params tests to update_targets_spec.rb - Removed redundant test from json_schema_spec.rb - Deleted index_inheritance_spec.rb Tests are now co-located with the features they test at the right abstraction layers. Maintained 100% line and branch coverage. Also fixed Set union operator in implements_interfaces.rb to use idiomatic | instead of +.
elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb
Show resolved
Hide resolved
elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/type_spec.rb
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/enum_type.rb
Show resolved
Hide resolved
...ticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/union_type.rb
Show resolved
Hide resolved
...elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb
Outdated
Show resolved
Hide resolved
myronmarston
left a comment
There was a problem hiding this comment.
This is really close!
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_subtypes.rb
Outdated
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/enum_type.rb
Show resolved
Hide resolved
...icgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/object_type.rb
Outdated
Show resolved
Hide resolved
...elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb
Outdated
Show resolved
Hide resolved
Modified existing apollo test to put the index on NamedEntity interface (previously concrete types had their own indices). IndexedType1 and IndexedType2 now inherit the index from NamedEntity. Intentionally define subtypes before their indexed supertype to verify type references resolve correctly regardless of definition order. This would fail if @key directives were applied during type initialization (when supertypes may not be defined yet).
Renamed InterfaceA/InterfaceB to Animal/Dog/Poodle for clearer conceptual hierarchy. Added test validating that multiple indexed interfaces in an inheritance chain (Poodle → Dog [indexed] → Animal [indexed]) correctly raises a validation error.
- Standardized on supertype/subtype instead of parent/child throughout - Renamed indexed_parents to indexed_supertypes in has_indices.rb - Simplified object_type.rb indexing_fields_by_name_in_index with early returns - Improved EntityTypeExtension documentation explaining _Entity overrides - Updated directly_queryable? doc to match existing pattern: "directly queryable via a type-specific field on the root Query type" - Simplified self_update_target to use root_document_type? (clearer semantics) - Removed duplicate negative test in update_targets_spec (covered elsewhere)
myronmarston
left a comment
There was a problem hiding this comment.
This is fantastic! Appreciate the back-and-forth that helped us arrive at a better design!
...icgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/object_type.rb
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb
Outdated
Show resolved
Hide resolved
myronmarston
left a comment
There was a problem hiding this comment.
@marcdaniels-toast I was about to merge but I noticed that we lost some of the earlier fixes you had done, such as in 40ed8d1. I believe the changes in that commit (the added json_schema_spec.rb spec + the results.rb fix that makes that spec pass) are necessary for inherited indexes to work properly--otherwise, at ingestion time, we'll run into the error I reported in #1029.
Was that reversion intentional? (If so, can you share why?)
By the way, this PR is large enough (and we've gone through enough rounds of review) that if there is more to do (such as bring those changes back), I think I'd prefer to do it in a follow up PR so that we can merge that one and I have a smaller diff to review for the remaining changes...rather than trying to incorporate them into this already large PR.
|
@myronmarston I think we're okay. Notice how that commit corresponds to back when I had the previous (wrong) semantics for [ So back then, I had to change the filter for matching indexed types to use !type.own_or_inherited_index_def.nil? which is now more simply done with type.root_document_type? (Is the type indexed via direct or inherited?) And hence no need to change that select condition.
That spec test does look valuable though since it tests enum type correctness for a mix of inherited and own indexes. I confirmed locally that that test passes on this branch. I can make a new PR that just adds this one test if you're still okay with merging this as is. |
Makes sense, thanks!
Sounds good! |
Overview
First of likely three PRs implementing index inheritance (Issue #1029). Adds schema definition infrastructure to allow concrete types to inherit index definitions from parent abstract types (unions/interfaces), enabling mixed-type indices where multiple types share a single datastore index.
Problem
Currently every indexed type must define its own index. This PR enables patterns like:
Both
OnlineStoreandMobileStoredocuments live in the samestoresindex, requiring__typenamefor query-time type resolution.Key Changes
Schema Definition:
index_def()to resolve own or inherited indices from parent abstract typesroot_document_type?(): true when a type is indexed (own index or inherited)directly_queryable?()to indicate when a type is a root graphqlQuerytypeValidation:
Tests
index_definition_names_spec.rb(concrete + transitive)__typenamedata_params tests toupdate_targets_spec.rbabstract_types_spec.rbroot_document_type?()stubs from scalar/enum typesWhat's Not Included
This PR is infrastructure only:
These are deferred to PR2 (Indexer) and PR3 (Schema + GraphQL + Acceptance).
Related