Repository navigation
Offer indexes on synthetic record types to the planner when a query reads all their stored constituents - #4840
Conversation
…eads all their stored constituents
| @Nonnull final RecordStoreState recordStoreState, | ||
| @Nonnull final Collection<String> queriedRecordTypeNames) { | ||
| final var indexes = Lists.<Index>newArrayList(); |
There was a problem hiding this comment.
Nit: Run the IntelliJ formatter over this.
| */ | ||
| @Nonnull | ||
| @VisibleForTesting | ||
| static List<Index> syntheticIndexesOf(@Nonnull final RecordMetaData metaData, |
There was a problem hiding this comment.
Rather readableSyntheticIndexesOf(), since we also apply the readableOf() filter to the result.
There was a problem hiding this comment.
But actually, I would vote to keep that readableOf() filtering out of this method and let the caller do the filtering. That will make the call site more harmonious with how the other indexes are collected, via indexList.addAll(readableOf(recordStoreState, metaData.getUniversalIndexes())).
| final var indexes = Lists.<Index>newArrayList(); | ||
| for (final var syntheticRecordType : metaData.getSyntheticRecordTypes().values()) { | ||
| final var storedConstituentNames = | ||
| syntheticRecordType.getConstituents().stream() |
There was a problem hiding this comment.
What about the constituents that are isOuterJoined(), are they intentionally included in this stream?
There was a problem hiding this comment.
Yes, I have included them for now. In the work, I am not considering JoinedRecordType with outer join.
| .map(constituent -> constituent.getRecordType().getName()) | ||
| .filter(metaData.getRecordTypes()::containsKey) | ||
| .collect(ImmutableSet.toImmutableSet()); | ||
| if (!storedConstituentNames.isEmpty() && queriedRecordTypeNames.containsAll(storedConstituentNames)) { |
There was a problem hiding this comment.
I don’t like the String-based “join” we’re doing here via containsAll(). It seems unnecessary to take the detour through the strings, since the caller can just as well pass the queriedRecordTypes directly as a list of RecordType. Then this becomes a more efficient containsAll() over objects.
| final var storedConstituentNames = | ||
| syntheticRecordType.getConstituents().stream() | ||
| .map(constituent -> constituent.getRecordType().getName()) | ||
| .filter(metaData.getRecordTypes()::containsKey) |
There was a problem hiding this comment.
There’s got to be a more direct and efficient way to check that it’s a stored record type instead of this string-based containsKey() check. How about !constitutent.getRecordType().isSynthetic()?
| @Nonnull final Collection<String> queriedRecordTypeNames) { | ||
| final var indexes = Lists.<Index>newArrayList(); | ||
| for (final var syntheticRecordType : metaData.getSyntheticRecordTypes().values()) { | ||
| final var storedConstituentNames = |
There was a problem hiding this comment.
It could be worth added a dedicated helper to get the stored constituent record types, instead of doing an ad-hoc stream for it in here. How about SyntheticRecordType.getStoredConstitutentRecordTypes()? With that, the helper here would become a simple for (syntheticRecordType) { if (queriesRecordTypes.containsAll(type.getStoredConstitutentRecordTypes())) } } loop.
| * @return the readable indexes on the synthetic record types the query reads all the stored constituents of | ||
| */ | ||
| @Nonnull | ||
| @VisibleForTesting |
There was a problem hiding this comment.
Maybe there’s a more natural place for this method where it “makes sense” and can then be official and public, so we’d not need the @VisibleForTesting mark. Maybe a method in RecordMetaData or SyntheticRecordType? Then we’d also not need to add a whole new MetaDataPlanContextSyntheticIndexTest test file but could just extend the existing test files.
There was a problem hiding this comment.
Shifted to RecordMetadata
| if (queriedRecordTypeNamesOptional.isEmpty()) { // ALL_TYPES | ||
| commonPrimaryKey = commonPrimaryKey(metaData.getRecordTypes().values()); | ||
| queriedRecordTypeNames = metaData.getRecordTypes().keySet(); | ||
| queriedRecordTypes = metaData.getRecordTypes().values(); |
There was a problem hiding this comment.
Consider adding ImmutableSet.copyOf() here to wrap this into an ImmutableSet as well (like in the else branch below). That should make the containsAll() we do on this set more efficient. Otherwise it’ll be a linear scan per element.
| final List<SyntheticRecordType<?>> result = new ArrayList<>(); | ||
| for (SyntheticRecordType<?> syntheticRecordType : syntheticRecordTypes.values()) { | ||
| final Set<RecordType> storedRecordTypes = syntheticRecordType.getStoredRecordTypes(); | ||
| if (!storedRecordTypes.isEmpty() && recordTypes.containsAll(storedRecordTypes)) { |
There was a problem hiding this comment.
Can storedRecordTypes.isEmpty() ever happen? Does the API technically allow defining e.g. a joined type with no constituents? (Could be worth a comment or even a verify.)
MetaDataPlanContextnever offered indexes on synthetic record types to the planner. BothforRecordQueryandforRootReferencenow also offer the readable indexes of every synthetic record type whose stored constituents are all read by the query, found with the newRecordMetaData.getSyntheticRecordTypesFromRecordTypes(Collection<RecordType>).SyntheticRecordType.getStoredRecordTypes()gives those constituents: all of a joined type's, only the parent of an unnested type. Outer-joined constituents are still required for now.This changes no plans yet: synthetic primary keys are
ListKeyExpressions, which index expansion does not support, so such indexes are skipped. It is groundwork for #4817. Tests are inRecordMetaDataBuilderTest.