Skip to content

Build a match candidate from an index defined on an unnested record type #4641

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.

Sign up for GitHub

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
g31pranjal wants to merge 11 commits into
base: apple/g31pranjal/unnested/explode-ordinality-matching
Choose a base branch
from
+1,631 −42
Open

Build a match candidate from an index defined on an unnested record type #4641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
883f137
changes
g31pranjal Oct 1, 2026
71e6b67
changes
g31pranjal Oct 1, 2026
21a68cc
more changes
g31pranjal Oct 1, 2026
71b29ee
Add range predicate tests for the index on the unnested record type
g31pranjal Oct 2, 2026
1f8f04a
more tests
g31pranjal Oct 6, 2026
8dba6e2
nit
g31pranjal Oct 7, 2026
4d26935
issue
g31pranjal Oct 7, 2026
b5b5d6b
more tests
g31pranjal Oct 7, 2026
7594200
Regenerate the unnested record type index expectations over the table…
g31pranjal Nov 4, 2026
86e0145
Indent the ternary continuations of the synthetic candidate helpers f…
g31pranjal Nov 7, 2026
d3d906f
Disable the grouped PartiQL query over the deep-element index, which …
g31pranjal Nov 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion ...java/com/apple/foundationdb/record/query/plan/cascades/KeyExpressionExpansionVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
import com.apple.foundationdb.record.query.plan.cascades.values.EmptyValue;
import com.apple.foundationdb.record.query.plan.cascades.values.ExplodeValue;
import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue;

Check notice on line 43 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/KeyExpressionExpansionVisitor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 90.6% (241/266 lines) | Changed lines: 76.0% (19/25 lines)
import com.apple.foundationdb.record.query.plan.cascades.values.RecordConstructorValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
import com.apple.foundationdb.record.util.ProtoUtils;
import com.google.common.base.Verify;
Expand Down Expand Up @@ -482,7 +483,37 @@
@Nonnull
@Override
public GraphExpansion visitExpression(@Nonnull final ListKeyExpression listKeyExpression) {
throw new UnsupportedOperationException("visitor method for this key expression is not implemented");
final ImmutableList.Builder<GraphExpansion> expandedPredicatesBuilder = ImmutableList.builder();
final VisitorState state = getCurrentState();
int currentOrdinal = state.getCurrentOrdinal();
for (KeyExpression child : listKeyExpression.getChildren()) {
final VisitorState childState = state.withCurrentOrdinal(currentOrdinal);
final GraphExpansion childExpansion = pop(child.expand(push(childState.forFunctionalExpansion())));
if (!childExpansion.getQuantifiers().isEmpty()) {
throw new UnsupportedOperationException("cannot expand a list whose child introduces quantifiers");
}
final var childValues = childExpansion.getResultColumns()
.stream()
.map(Column::getValue)
.collect(ImmutableList.toImmutableList());
// a list places its child in a nested tuple, so the value of the position is the record of the child's
// column values -- a record of one for a single-column child, as that tuple is nested all the same
final var value = childState.registerValue(RecordConstructorValue.ofUnnamed(childValues));
// the child's position contributes what a scalar field at that position would
final var isSargable = childState.isKey() && !childState.isInternalExpansion();
if (childState.isSelectStar()) {
expandedPredicatesBuilder.add(isSargable
? GraphExpansion.ofPlaceholder(value.asPlaceholder(newParameterAlias()))
: GraphExpansion.empty());
} else {
expandedPredicatesBuilder.add(isSargable
? GraphExpansion.ofResultColumnAndPlaceholder(Column.unnamedOf(value),
value.asPlaceholder(newParameterAlias()))
: GraphExpansion.ofResultColumn(Column.unnamedOf(value)));
}
Comment on lines +504 to +513

@robert-brunel robert-brunel Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These four nested isSelectStar()/isSargable cases look cumbersome, and there is something very similar in visitExpression(FieldKeyExpression) already. Maybe you can extract some shared helper and use it in the two places to make it nicer.

currentOrdinal++;
}
return GraphExpansion.ofOthers(expandedPredicatesBuilder.build());
}

/**
Expand Down
17 changes: 16 additions & 1 deletion ...main/java/com/apple/foundationdb/record/query/plan/cascades/ScalarTranslationVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@
import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
import com.apple.foundationdb.record.query.plan.cascades.values.EmptyValue;
import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue;
import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue;

Check notice on line 36 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ScalarTranslationVisitor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 86.4% (70/81 lines) | Changed lines: 90.0% (9/10 lines)
import com.apple.foundationdb.record.query.plan.cascades.values.RecordConstructorValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
import com.apple.foundationdb.record.util.ProtoUtils;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -206,7 +207,21 @@
@Nonnull
@Override
public Value visitExpression(@Nonnull final ListKeyExpression listKeyExpression) {
throw new UnsupportedOperationException("visitor method for this key expression is not implemented");
if (listKeyExpression.getColumnSize() > 1) {
throw new RecordCoreException("cannot expand ListKeyExpression in scalar expansion");
}
Comment on lines +210 to +212

@robert-brunel robert-brunel Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this restriction? (Could use some explanation in a comment.)


final ScalarVisitorState state = getCurrentState();
final KeyExpression child = Iterables.getOnlyElement(listKeyExpression.getChildren());

// The child occupies a single position whose value is the nested tuple of the child's own columns, so it is a
// record of their values -- a record of one for a single-column child, as that tuple is nested all the same.
final List<KeyExpression> positions = child.normalizeKeyForPositions();
final ImmutableList.Builder<Value> valuesBuilder = ImmutableList.builder();
for (final KeyExpression position : positions) {
valuesBuilder.add(pop(position.expand(push(state))));
}
return RecordConstructorValue.ofUnnamed(valuesBuilder.build());
}

@Nonnull
Expand Down
44 changes: 34 additions & 10 deletions ...n/java/com/apple/foundationdb/record/query/plan/cascades/ScanWithFetchMatchCandidate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,18 +149,21 @@
final var logicalKeyValuesBuilder = ImmutableList.<Value>builder();
for (int i = 0; i < indexKeyValues.size(); i++) {
final Value keyValue = indexKeyValues.get(i);

Check notice on line 152 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ScanWithFetchMatchCandidate.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 93.9% (92/98 lines) | Changed lines: 94.7% (18/19 lines)
final var extractFromIndexEntryPairOptional =
keyValue.extractFromIndexEntryMaybe(baseObjectValue, EvaluationContext.empty(), AliasMap.emptyMap(),
ImmutableSet.of(), IndexKeyValueToPartialRecord.TupleSource.KEY, ImmutableIntArray.of(i));
if (extractFromIndexEntryPairOptional.isPresent()) {
final var extractFromIndexEntryPair = extractFromIndexEntryPairOptional.get();
if (!addCoveringField(builder, extractFromIndexEntryPair.getKey(),
extractFromIndexEntryPair.getValue())) {
return Optional.empty();
for (final var datum : entryData(keyValue, ImmutableIntArray.of(i))) {
final var extractFromIndexEntryPairOptional =
datum.getLeft().extractFromIndexEntryMaybe(baseObjectValue, EvaluationContext.empty(),
AliasMap.emptyMap(), ImmutableSet.of(),
IndexKeyValueToPartialRecord.TupleSource.KEY, datum.getRight());
if (extractFromIndexEntryPairOptional.isPresent()) {
final var extractFromIndexEntryPair = extractFromIndexEntryPairOptional.get();
if (!addCoveringField(builder, extractFromIndexEntryPair.getKey(),
extractFromIndexEntryPair.getValue())) {
return Optional.empty();
}
recordCoveredField(covered, extractFromIndexEntryPair);
logicalKeyValuesBuilder.add(extractFromIndexEntryPair.getLeft());
}
recordCoveredField(covered, extractFromIndexEntryPair);
logicalKeyValuesBuilder.add(extractFromIndexEntryPair.getLeft());
}
}

Comment on lines 150 to 169

@robert-brunel robert-brunel Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that this logic is now getting quite cumbersome overall, with the two nested loops, the hidden recursion in entryData() that allocates several of these temporary List and ImmutableIntArray objects, then all that Optional/getLeft()/getRight()/getKey()/getValue() stuff. It’s getting hard to follow.The method could use some comments as well.

Could you try spending some time simplifying this further. There’s probably a better way to cut this than the entryData() helper. Essentially, the two nested loops are doing a recursive walk over indexKeyValues, and then the loop body is doing various add() calls, with the twist that there’s sometimes an early exit via return Optional.empty(). So it should be possible to bring all this into a simpler form roughly like this:

for (int i = 0; i < indexKeyvalues.size(); i++) {
  if (!addDatum(indexKeyValues.get(i), «path», …) {
    return Optional.empty();
  }
}

... and move the recursion, as well as the various addCoveringField()/recordCoveredField() calls, into addDatum(). The goal would be to get rid of the nested for { for { if { if { } } } here and remove the need for these cumbersome List<NonnullPair<...>> and Optional objects.

Expand Down Expand Up @@ -192,6 +195,27 @@
indexEntryToRecordValue(baseType, covered)));
}

/**
* Decomposes one column of the index entry into the data it holds that can be read individually.
*
* @param value the value registered for the column
* @param ordinalPath the path to the column
* @return the data that column holds, each datum paired with the path at which it sits in the entry's tuple
*/
@Nonnull
private static List<NonnullPair<Value, ImmutableIntArray>> entryData(@Nonnull final Value value,
@Nonnull final ImmutableIntArray ordinalPath) {
if (!(value instanceof RecordConstructorValue)) {

@robert-brunel robert-brunel Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you perhaps add a brief comment (or extend the javadoc) explaining why RecordConstructorValue matters here? It’s basically because such an RCV is now produced by the key expansion for a ListKeyExpression, right?

return ImmutableList.of(NonnullPair.of(value, ordinalPath));

Check warning on line 209 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ScanWithFetchMatchCandidate.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ScanWithFetchMatchCandidate.java#L209 

Use `java.util.List.of` instead
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?id=9DAB9F1B80D73CEDD9BACE3AFE886868&t=apple%2Fg31pranjal%2Funnested%2Fsyn-relational-integration%3AHEAD
}
final var columns = ((RecordConstructorValue)value).getColumns();
final var dataBuilder = ImmutableList.<NonnullPair<Value, ImmutableIntArray>>builder();
for (int i = 0; i < columns.size(); i++) {
dataBuilder.addAll(entryData(columns.get(i).getValue(), ImmutableIntArray.builder().addAll(ordinalPath).add(i).build()));
}
return dataBuilder.build();
}

/**
* Records what the given extraction covers, descending a node per field of the path it fills, which may run into
* nested messages. Every name is present here, {@code addCoveringField} having already refused the extraction
Expand Down
93 changes: 83 additions & 10 deletions ...in/java/com/apple/foundationdb/record/query/plan/cascades/ValueIndexExpansionVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
import com.apple.foundationdb.record.RecordCoreException;
import com.apple.foundationdb.record.metadata.Index;
import com.apple.foundationdb.record.metadata.IndexTypes;
import com.apple.foundationdb.record.metadata.RecordType;

Check notice on line 26 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ValueIndexExpansionVisitor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 99.2% (119/120 lines) | Changed lines: 100.0% (35/35 lines)
import com.apple.foundationdb.record.metadata.SyntheticRecordType;
import com.apple.foundationdb.record.metadata.expressions.GroupingKeyExpression;
import com.apple.foundationdb.record.metadata.expressions.KeyExpression;
import com.apple.foundationdb.record.metadata.expressions.KeyWithValueExpression;
Expand All @@ -32,9 +33,12 @@
import com.apple.foundationdb.record.query.plan.cascades.predicates.Placeholder;
import com.apple.foundationdb.record.query.plan.cascades.predicates.PredicateWithValueAndRanges;
import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
import com.apple.foundationdb.record.query.plan.cascades.values.RecordConstructorValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
import com.apple.foundationdb.record.query.plan.cascades.values.translation.TranslationMap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;

import javax.annotation.Nonnull;
Expand All @@ -43,6 +47,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;

import static com.apple.foundationdb.record.metadata.Key.Expressions.concat;
Expand Down Expand Up @@ -72,6 +77,62 @@
this.queriedRecordTypes = ImmutableList.copyOf(queriedRecordTypes);
}

/**
* Returns the {@link SyntheticRecordType} an index is defined on, if it is defined on exactly one.
*/
@Nonnull
private static Optional<SyntheticRecordType<?>> syntheticRecordTypeMaybe(
@Nonnull final Collection<RecordType> indexedRecordTypes) {
if (indexedRecordTypes.size() != 1) {
return Optional.empty();
}
final RecordType indexedRecordType = Iterables.getOnlyElement(indexedRecordTypes);
return indexedRecordType instanceof final SyntheticRecordType<?> syntheticRecordType
? Optional.of(syntheticRecordType)
: Optional.empty();
}

/**
* Merges the expansion of an index key into the expansion of the synthetic type the index is defined on, if it is
* defined on one. An index on a stored type has no base expansion, and its key expansion is returned as it stands.
*
* <p>The key's values are written over {@code baseQuantifier}, which does not survive into the merged graph, so
* each is re-expressed over the record the base expansion assembles and simplified:
* {@code base.unnesting_0.reviewer} over {@code (parent AS parent, explode._0 AS unnesting_0, ...)} becomes
* {@code explode._0.reviewer}, the very value a query navigating the same unnesting flows.
*
* <p>Predicates and placeholders are translated separately because a placeholder is held in both lists and
* {@link GraphExpansion#seal()} pairs them up by value; translating one alone would leave a parameter whose
* predicate the select does not hold.
*
* @param baseExpansion the expansion assembling the synthetic type's records, or {@code null} if there is none
* @param keyExpansion the expansion of the index key, expressed over {@code baseQuantifier}
* @param baseQuantifier the quantifier {@code keyExpansion} is expressed over
* @return the key expansion merged into the base expansion, or the key expansion itself if there is no base
*/
@Nonnull
private static GraphExpansion mergeWithBaseIfNeeded(@Nullable final GraphExpansion baseExpansion,
@Nonnull final GraphExpansion keyExpansion,
@Nonnull final Quantifier.ForEach baseQuantifier) {
if (baseExpansion == null) {
return keyExpansion;
}
final var ontoBaseRecord = TranslationMap.regularBuilder()
.when(baseQuantifier.getAlias())
.then((sourceAlias, leafValue) -> RecordConstructorValue.ofColumns(baseExpansion.getResultColumns()))
.build();
return GraphExpansion.ofOthers(
baseExpansion.toBuilder().removeAllResultColumns().build(),
keyExpansion.toBuilder()
.removeAllPredicates()
.addAllPredicates(keyExpansion.getPredicates().stream()
.map(predicate -> predicate.translateCorrelations(ontoBaseRecord, true))
.collect(ImmutableList.toImmutableList()))
.replacePlaceholder(placeholder ->
placeholder.translateLeafPredicate(ontoBaseRecord, true))
.build());
}

@Nonnull
@Override
public MatchCandidate expand(@Nonnull final Set<String> availableRecordTypeNames,
Expand All @@ -87,12 +148,22 @@
// the instantiation of the type filter below to create a placeholder for the record type key parameter
// alias and reuse it here. Similar to what we currently do for primary scans.
//
final var baseQuantifier = Quantifier.forEach(ExpansionVisitor.createBaseRef(availableRecordTypeNames,
queriedRecordTypeNames, baseType, null, accessHint));
@Nullable final GraphExpansion baseExpansion =
syntheticRecordTypeMaybe(queriedRecordTypes).map(recordType -> recordType.expand(accessHint)).orElse(null);
final var allExpansionsBuilder = ImmutableList.<GraphExpansion>builder();

// add the value for the flow of records
allExpansionsBuilder.add(GraphExpansion.ofQuantifier(baseQuantifier));
final Quantifier.ForEach baseQuantifier;
final Value baseObjectValue;
if (baseExpansion == null) {
baseQuantifier = Quantifier.forEach(ExpansionVisitor.createBaseRef(availableRecordTypeNames,
queriedRecordTypeNames, baseType, null, accessHint));
baseObjectValue = baseQuantifier.getFlowedObjectValue();
// add the value for the flow of records
allExpansionsBuilder.add(GraphExpansion.ofQuantifier(baseQuantifier));
} else {
// the base expansion's own quantifiers reach the graph when the key expansion is merged into it
baseQuantifier = Quantifier.forEach(Reference.initialOf(baseExpansion.seal().buildSelect()));
baseObjectValue = RecordConstructorValue.ofColumns(baseExpansion.getResultColumns());
}

var rootExpression = index.getRootExpression();

Expand All @@ -105,8 +176,7 @@
}

final int keyValueSplitPoint;
if (rootExpression instanceof KeyWithValueExpression) {
final KeyWithValueExpression keyWithValueExpression = (KeyWithValueExpression)rootExpression;
if (rootExpression instanceof final KeyWithValueExpression keyWithValueExpression) {
keyValueSplitPoint = keyWithValueExpression.getSplitPoint();
rootExpression = keyWithValueExpression.getInnerKey();
} else {
Expand Down Expand Up @@ -190,15 +260,18 @@
}
}

final var completeExpansion = GraphExpansion.ofOthers(allExpansionsBuilder.build());
final var sealedExpansion = completeExpansion.seal();
final var sealedExpansion = mergeWithBaseIfNeeded(
baseExpansion,
GraphExpansion.ofOthers(allExpansionsBuilder.build()),
baseQuantifier)
.seal();
final var parameters =
sealedExpansion.getPlaceholders()
.stream()
.map(Placeholder::getParameterAlias)
.collect(ImmutableList.toImmutableList());
final var matchableSortExpression = new MatchableSortExpression(parameters, isReverse,
sealedExpansion.buildSelectWithResultValue(baseQuantifier.getFlowedObjectValue()));
sealedExpansion.buildSelectWithResultValue(baseObjectValue));
return new ValueIndexScanMatchCandidate(index,
queriedRecordTypes,
Traversal.withRoot(Reference.initialOf(matchableSortExpression)),
Expand Down
Loading
Loading