From d46370d0ac1e74adf275d3a2a73b8ae10ce0142f Mon Sep 17 00:00:00 2001 From: Pranjal Gupta Date: Tue, 22 Sep 2026 18:13:23 +0100 Subject: [PATCH 1/3] Let an explode declare the shape of the value it flows An explode `WITH ORDINALITY` flows an anonymous `(element, ordinal)` struct, and the value standing for that struct is a single opaque `QueriedValue`. Nothing inside it is reachable, which is fine for evaluation -- the plan builds the struct from the protobuf descriptor its declared type names, not from the value -- but it leaves no way to relate the element to anything. `MaxMatchMap` descends into record constructors and into nothing else, so an index over an `UnnestedRecordType`, whose candidate unnests with ordinality, cannot be related to a query that unnests the same collection plainly. `ExplodeExpression` and `RecordQueryExplodePlan` now carry a `flowsRecordConstructorValue` flag. Set, the value is a `RecordConstructorValue` whose columns -- the element, and the ordinal for the `WITH ORDINALITY` variant -- are each reachable on their own; unset, it is the opaque value of the whole explode result type. What the flag decides differs between the two variants. With ordinality the struct is the result type either way, so only the shape of the value changes, and the plan produces the same rows whichever shape it flows. Plainly, the struct is the flag's doing: the result type is the element type without it and a one-column struct with it, and the plan produces that struct rather than the bare element. The shape has to be state the plan records rather than something inferred from `withOrdinality`, because the plan hashes the value it flows: a plan serialized by an earlier version flows the opaque value and was hashed as such, and must keep doing both when a newer version reads it back. So the flag is serialized as `PRecordQueryExplodePlan.flows_rcv`, written only when set, and absent means the opaque value -- which is precisely what a plan from before the field existed flows. Nothing sets the flag yet: every constructor that does not name a shape picks the opaque value, so no explode flows anything different and plan hashes and expected plans are untouched. `ImplementExplodeRule` takes the shape off the expression it implements rather than choosing one, so the plan inherits whatever the expression settled on. --- .../expressions/ExplodeExpression.java | 101 ++++++++-- .../properties/DerivationsProperty.java | 16 +- .../cascades/rules/ImplementExplodeRule.java | 3 +- .../plan/plans/RecordQueryExplodePlan.java | 70 +++++-- .../src/main/proto/record_query_plan.proto | 1 + .../query/plan/plans/ExplodePlanTest.java | 181 +++++++++++++++++- 6 files changed, 328 insertions(+), 44 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java index 5d4f9faaf69..4283a46b234 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java @@ -23,6 +23,7 @@ import com.apple.foundationdb.annotation.API; import com.apple.foundationdb.record.EvaluationContext; import com.apple.foundationdb.record.query.plan.cascades.AliasMap; +import com.apple.foundationdb.record.query.plan.cascades.Column; import com.apple.foundationdb.record.query.plan.cascades.ComparisonRange; import com.apple.foundationdb.record.query.plan.cascades.Compensation; import com.apple.foundationdb.record.query.plan.cascades.CorrelationIdentifier; @@ -35,6 +36,7 @@ import com.apple.foundationdb.record.query.plan.cascades.typing.Type; import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue; import com.apple.foundationdb.record.query.plan.cascades.values.QueriedValue; +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.PullUp; import com.apple.foundationdb.record.query.plan.cascades.values.translation.TranslationMap; @@ -57,6 +59,9 @@ *

In the {@code WITH ORDINALITY} variant, it also generates ordinals of the field values. In this case it * produces a struct with two anonymous fields—the element and the ordinal—instead of the bare element. The ordinals * are 1-based per the SQL standard, unless the expression is created {@linkplain #isZeroBasedOrdinality() 0-based}. + * + *

An explode that {@linkplain #flowsRecordConstructorValue() flows a record constructor} produces a struct in the + * plain variant as well, with the element as its only field. */ @API(API.Status.EXPERIMENTAL) public class ExplodeExpression extends AbstractRelationalExpressionWithoutChildren implements InternalPlannerGraphRewritable { @@ -85,15 +90,35 @@ public class ExplodeExpression extends AbstractRelationalExpressionWithoutChildr @Nonnull private final Type explodeResultType; + /** + * Whether the value this expression flows is a record constructor of the element—and the ordinal, for the + * {@code WITH ORDINALITY} variant—rather than one opaque value. + */ + private final boolean flowsRecordConstructorValue; + + /** + * The result value of the explode. + */ + @Nonnull + private final Value resultValue; + public ExplodeExpression(@Nonnull final Value collectionValue, final boolean withOrdinality, - final boolean zeroBasedOrdinality) { + final boolean zeroBasedOrdinality, final boolean flowsRecordConstructorValue) { Verify.verify(withOrdinality || !zeroBasedOrdinality, "cannot base ordinals that are not produced"); this.collectionValue = collectionValue; this.withOrdinality = withOrdinality; this.zeroBasedOrdinality = zeroBasedOrdinality; Verify.verify(collectionValue.getResultType().isArray()); this.elementType = Objects.requireNonNull(((Type.Array)collectionValue.getResultType()).getElementType()); - this.explodeResultType = explodeResultType(elementType, withOrdinality); + this.flowsRecordConstructorValue = flowsRecordConstructorValue; + this.explodeResultType = explodeResultType(elementType, withOrdinality, flowsRecordConstructorValue); + this.resultValue = explodeResultValue(elementType, withOrdinality, flowsRecordConstructorValue); + Verify.verify(explodeResultType.equals(resultValue.getResultType())); + } + + public ExplodeExpression(@Nonnull final Value collectionValue, final boolean withOrdinality, + final boolean zeroBasedOrdinality) { + this(collectionValue, withOrdinality, zeroBasedOrdinality, false); } public ExplodeExpression(@Nonnull final Value collectionValue, final boolean withOrdinality) { @@ -113,18 +138,28 @@ public Type getElementType() { } /** - * Returns the type of the explode result. For the {@code WITH ORDINALITY} variant, builds an anonymous-field - * struct result type holding the element and the ordinal. + * Returns the type of the explode result: the element type itself, or an anonymous-field struct holding the + * element and, for the {@code WITH ORDINALITY} variant, the ordinal. The {@code WITH ORDINALITY} variant needs the + * struct to carry the ordinal; the plain variant takes it only when the explode + * {@linkplain #flowsRecordConstructorValue() flows a record constructor}. + * + * @param elementType the element type of the collection being exploded + * @param withOrdinality whether ordinals are produced alongside the elements + * @param flowsRecordConstructorValue whether the explode flows a record constructor + * @return the result type of such an explode */ @Nonnull - public static Type explodeResultType(@Nonnull final Type elementType, boolean withOrdinality) { - if (withOrdinality) { - return Type.Record.fromFields(ImmutableList.of( - Type.Record.Field.of(elementType, Optional.empty()), - Type.Record.Field.of(Type.primitiveType(Type.TypeCode.INT, false), Optional.empty()))); - } else { + public static Type explodeResultType(@Nonnull final Type elementType, final boolean withOrdinality, + final boolean flowsRecordConstructorValue) { + if (!withOrdinality && !flowsRecordConstructorValue) { return elementType; } + final var fields = ImmutableList.builder(); + fields.add(Type.Record.Field.of(elementType, Optional.empty())); + if (withOrdinality) { + fields.add(Type.Record.Field.of(Type.primitiveType(Type.TypeCode.INT, false), Optional.empty())); + } + return Type.Record.fromFields(fields.build()); } /** @@ -135,10 +170,42 @@ public Type getExplodeResultType() { return explodeResultType; } + /** + * Returns the value an explode of {@code elementType} flows, of the + * {@linkplain #explodeResultType(Type, boolean, boolean) explode result type} either way: one opaque + * {@link QueriedValue}, or a {@link RecordConstructorValue} whose columns are the element and, for the + * {@code WITH ORDINALITY} variant, the ordinal. + * + * @param elementType the element type of the collection being exploded + * @param withOrdinality whether ordinals are produced alongside the elements + * @param flowsRecordConstructorValue whether the element and the ordinal are flowed as a record constructor + * @return the value flowed by such an explode + */ + @Nonnull + public static Value explodeResultValue(@Nonnull final Type elementType, final boolean withOrdinality, + final boolean flowsRecordConstructorValue) { + if (!flowsRecordConstructorValue) { + return new QueriedValue(explodeResultType(elementType, withOrdinality, false)); + } + // Note: the element must stay the first column. `MaxMatchMap` returns the first reachable candidate value + // that compares equal, and a `QueriedValue` has no identity beyond its class and result type, so an element + // whose type is also a non-nullable `INT` -- the ordinal's type -- would just as happily match the ordinal + // if the ordinal came first. + // + // The record is built nullable because `explodeResultType` declares it that way, and the callers verify + // that the two agree: that declared type is what the plan looks its protobuf descriptor up by at run time. + final var columns = ImmutableList.>builder(); + columns.add(Column.unnamedOf(new QueriedValue(elementType))); + if (withOrdinality) { + columns.add(Column.unnamedOf(new QueriedValue(Type.primitiveType(Type.TypeCode.INT, false)))); + } + return RecordConstructorValue.ofColumns(columns.build(), true); + } + @Nonnull @Override public Value getResultValue() { - return new QueriedValue(getExplodeResultType()); + return resultValue; } @Nonnull @@ -154,6 +221,15 @@ public boolean isZeroBasedOrdinality() { return zeroBasedOrdinality; } + /** + * Returns whether the value this expression flows is a record constructor of the element—and the ordinal, for the + * {@code WITH ORDINALITY} variant—rather than one opaque value. The plan implementing the expression takes this + * over. + */ + public boolean flowsRecordConstructorValue() { + return flowsRecordConstructorValue; + } + @Nonnull @Override public List getQuantifiers() { @@ -205,7 +281,8 @@ public ExplodeExpression translateCorrelations(@Nonnull final TranslationMap tra collectionValue.translateCorrelations(translationMap, shouldSimplifyValues); // this is ok since there are no new quantifiers if (translatedCollectionValue != collectionValue) { - return new ExplodeExpression(translatedCollectionValue, withOrdinality, zeroBasedOrdinality); + return new ExplodeExpression(translatedCollectionValue, withOrdinality, zeroBasedOrdinality, + flowsRecordConstructorValue); } return this; } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java index bb2d7701bcb..b6192c59a38 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java @@ -351,11 +351,17 @@ public Derivations visitExplodePlan(@Nonnull final RecordQueryExplodePlan explod // Use `FirstOrDefaultValue` as a representative for “some” element of the array. final Value first = new FirstOrDefaultValue(collectionValue, new ThrowsValue(elementType)); final Value representative; - if (explodePlan.isWithOrdinality()) { - final Type ordinalType = Type.primitiveType(Type.TypeCode.INT, false); - representative = RecordConstructorValue.ofColumns(List.of( - Column.of(Type.Record.Field.of(elementType, Optional.empty()), first), - Column.of(Type.Record.Field.of(ordinalType, Optional.empty()), new ThrowsValue(ordinalType)))); + if (explodePlan.isWithOrdinality() || explodePlan.flowsRecordConstructorValue()) { + // The plan flows the element wrapped in a struct, so the representative has to be wrapped the same way: + // accesses of the element are written against the wrapper. + final var columns = ImmutableList.>builder(); + columns.add(Column.of(Type.Record.Field.of(elementType, Optional.empty()), first)); + if (explodePlan.isWithOrdinality()) { + final Type ordinalType = Type.primitiveType(Type.TypeCode.INT, false); + columns.add(Column.of(Type.Record.Field.of(ordinalType, Optional.empty()), + new ThrowsValue(ordinalType))); + } + representative = RecordConstructorValue.ofColumns(columns.build()); } else { representative = first; } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java index eec952b1193..87bf87df14f 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java @@ -50,6 +50,7 @@ public void onMatch(@Nonnull final ImplementationCascadesRuleCall call) { final Value collectionValue = explodeExpression.getCollectionValue(); final boolean isWithOrdinality = explodeExpression.isWithOrdinality(); final boolean isZeroBasedOrdinality = explodeExpression.isZeroBasedOrdinality(); - call.yieldPlan(new RecordQueryExplodePlan(collectionValue, isWithOrdinality, isZeroBasedOrdinality)); + call.yieldPlan(new RecordQueryExplodePlan(collectionValue, isWithOrdinality, isZeroBasedOrdinality, + explodeExpression.flowsRecordConstructorValue())); } } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java index 8f4a09e8a2d..0b85a8b9512 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java @@ -42,11 +42,9 @@ import com.apple.foundationdb.record.query.plan.cascades.explain.NodeInfo; import com.apple.foundationdb.record.query.plan.cascades.explain.PlannerGraph; import com.apple.foundationdb.record.query.plan.cascades.expressions.AbstractRelationalExpressionWithoutChildren; -import com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression; import com.apple.foundationdb.record.query.plan.cascades.expressions.RelationalExpression; import com.apple.foundationdb.record.query.plan.cascades.typing.Type; import com.apple.foundationdb.record.query.plan.cascades.typing.TypeRepository; -import com.apple.foundationdb.record.query.plan.cascades.values.QueriedValue; 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; @@ -66,6 +64,9 @@ import java.util.Set; import java.util.stream.IntStream; +import static com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression.explodeResultType; +import static com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression.explodeResultValue; + /** * An {@code EXPLODE} query plan. * @@ -78,6 +79,11 @@ * case the plan produces a {@link DynamicMessage} struct with two anonymous fields (the element and the ordinal) * instead of the bare element. The ordinals are 1-based or 0-based depending on {@linkplain #isZeroBasedOrdinality()}. * + *

For the {@code WITH ORDINALITY} variant, the shape of the {@link Value} the plan declares as its result does not + * affect the data it produces. For the plain variant it does: the plan produces a struct with the element as its only + * anonymous field rather than the bare element, but only when the result value is a record constructor. See + * {@linkplain #flowsRecordConstructorValue()}. + * * @see RecordQueryFlatMapPlan */ @API(API.Status.INTERNAL) @@ -112,23 +118,38 @@ public class RecordQueryExplodePlan extends AbstractRelationalExpressionWithoutC @Nonnull private final Type explodeResultType; + /** + * Whether the value this plan flows is a record constructor of the element—and the ordinal, for the + * {@code WITH ORDINALITY} variant—rather than one opaque value. + */ + private final boolean flowsRecordConstructorValue; + + /** + * The value this plan flows, of the {@linkplain #explodeResultType explode result type} whichever shape it takes. + */ + @Nonnull + private final Value resultValue; + public RecordQueryExplodePlan(@Nonnull Value collectionValue, boolean withOrdinality, - boolean zeroBasedOrdinality) { + boolean zeroBasedOrdinality, boolean flowsRecordConstructorValue) { Verify.verify(withOrdinality || !zeroBasedOrdinality, "cannot base ordinals that are not produced"); this.collectionValue = collectionValue; this.withOrdinality = withOrdinality; this.zeroBasedOrdinality = zeroBasedOrdinality; Verify.verify(collectionValue.getResultType().isArray()); this.elementType = Objects.requireNonNull(((Type.Array)collectionValue.getResultType()).getElementType()); - this.explodeResultType = ExplodeExpression.explodeResultType(elementType, withOrdinality); + this.flowsRecordConstructorValue = flowsRecordConstructorValue; + this.explodeResultType = explodeResultType(elementType, withOrdinality, flowsRecordConstructorValue); + this.resultValue = explodeResultValue(elementType, withOrdinality, flowsRecordConstructorValue); + Verify.verify(explodeResultType.equals(resultValue.getResultType())); } public RecordQueryExplodePlan(@Nonnull Value collectionValue, boolean withOrdinality) { - this(collectionValue, withOrdinality, false); + this(collectionValue, withOrdinality, false, false); } public RecordQueryExplodePlan(@Nonnull Value collectionValue) { - this(collectionValue, false, false); + this(collectionValue, false); } @Nonnull @@ -144,6 +165,15 @@ public boolean isZeroBasedOrdinality() { return zeroBasedOrdinality; } + /** + * Returns whether the value this plan flows is a record constructor of the element—and the ordinal, for the + * {@code WITH ORDINALITY} variant—rather than one opaque value. For the plain variant this also decides whether the + * plan produces a struct holding the element rather than the bare element. + */ + public boolean flowsRecordConstructorValue() { + return flowsRecordConstructorValue; + } + @SuppressWarnings("resource") @Nonnull @Override @@ -154,33 +184,35 @@ public RecordCursor executePlan(@Nonnull final final Object result = collectionValue.eval(store, context); final List list = (result == null) ? List.of() : (List)result; - // Without ordinality, produce the bare elements. - if (!withOrdinality) { + // Without ordinality, and flowing the element itself, produce the bare elements. + if (!withOrdinality && !flowsRecordConstructorValue) { return RecordCursor.fromList(list, continuation) .map(QueryResult::ofComputed) .skipThenLimit(executeProperties.getSkip(), executeProperties.getReturnedRowLimit()); } - // In the WITH ORDINALITY case, produce a struct (element, ordinal) per list element. + // Otherwise produce a struct per list element: the element, and its ordinal in the WITH ORDINALITY case. final Type elementType = getElementType(); final var resultType = (Type.Record) getExplodeResultType(); final TypeRepository typeRepository = context.getTypeRepository(); final Descriptors.Descriptor descriptor = Objects.requireNonNull(typeRepository.getMessageDescriptor(resultType)); final Descriptors.FieldDescriptor elementField = descriptor.getFields().get(0); - final Descriptors.FieldDescriptor ordinalField = descriptor.getFields().get(1); + final Descriptors.FieldDescriptor ordinalField = withOrdinality ? descriptor.getFields().get(1) : null; final int firstOrdinal = zeroBasedOrdinality ? 0 : 1; - final ImmutableList indexedList = + final ImmutableList structs = IntStream.range(0, list.size()) .mapToObj(i -> { final DynamicMessage.Builder builder = DynamicMessage.newBuilder(descriptor); final Object element = Verify.verifyNotNull(list.get(i), "array elements must be non-null"); builder.setField(elementField, RecordConstructorValue.deepCopyIfNeeded(typeRepository, elementType, element)); - builder.setField(ordinalField, firstOrdinal + i); + if (ordinalField != null) { + builder.setField(ordinalField, firstOrdinal + i); + } return (Message)builder.build(); }) .collect(ImmutableList.toImmutableList()); - return RecordCursor.fromList(indexedList, continuation) + return RecordCursor.fromList(structs, continuation) .map(QueryResult::ofComputed) .skipThenLimit(executeProperties.getSkip(), executeProperties.getReturnedRowLimit()); } @@ -201,7 +233,8 @@ public RecordQueryExplodePlan translateCorrelations(@Nonnull final TranslationMa final Value translatedCollectionValue = collectionValue.translateCorrelations(translationMap, shouldSimplifyValues); if (translatedCollectionValue != collectionValue) { - return new RecordQueryExplodePlan(translatedCollectionValue, withOrdinality, zeroBasedOrdinality); + return new RecordQueryExplodePlan(translatedCollectionValue, withOrdinality, zeroBasedOrdinality, + flowsRecordConstructorValue); } return this; } @@ -267,7 +300,7 @@ public Type getExplodeResultType() { @Nonnull @Override public Value getResultValue() { - return new QueriedValue(getExplodeResultType()); + return resultValue; } @Nonnull @@ -368,6 +401,9 @@ public PRecordQueryExplodePlan toProto(@Nonnull final PlanSerializationContext s if (zeroBasedOrdinality) { builder.setZeroBasedOrdinality(true); } + if (flowsRecordConstructorValue) { + builder.setFlowsRcv(true); + } return builder.build(); } @@ -382,7 +418,9 @@ public static RecordQueryExplodePlan fromProto(@Nonnull final PlanSerializationC @Nonnull final PRecordQueryExplodePlan proto) { return new RecordQueryExplodePlan( Value.fromValueProto(serializationContext, Objects.requireNonNull(proto.getCollectionValue())), - proto.getWithOrdinality(), proto.getZeroBasedOrdinality()); + // defaults to false, which is what a plan serialized before these fields existed flows + proto.getWithOrdinality(), proto.getZeroBasedOrdinality(), + proto.getFlowsRcv()); } /** diff --git a/fdb-record-layer-core/src/main/proto/record_query_plan.proto b/fdb-record-layer-core/src/main/proto/record_query_plan.proto index b329bf76bd7..c12ff88160d 100644 --- a/fdb-record-layer-core/src/main/proto/record_query_plan.proto +++ b/fdb-record-layer-core/src/main/proto/record_query_plan.proto @@ -1884,6 +1884,7 @@ message PRecordQueryExplodePlan { optional PValue collection_value = 1; optional bool with_ordinality = 2; optional bool zero_based_ordinality = 3; + optional bool flows_rcv = 4; } // diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java index e6241c9af8c..28b1150a190 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java @@ -37,6 +37,8 @@ import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue; import com.apple.foundationdb.record.query.plan.cascades.values.LiteralValue; import com.apple.foundationdb.record.query.plan.cascades.values.QuantifiedObjectValue; +import com.apple.foundationdb.record.query.plan.cascades.values.QueriedValue; +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.apple.foundationdb.record.query.plan.serialization.DefaultPlanSerializationRegistry; @@ -270,7 +272,7 @@ void planHashIsStable() { Assertions.assertEquals(WITH_ORDINALITY_FOR_CONTINUATION_HASH, withOrdinality.planHash(PlanHashable.CURRENT_FOR_CONTINUATION)); - final var withZeroBasedOrdinality = new RecordQueryExplodePlan(collectionValue, true, true); + final var withZeroBasedOrdinality = new RecordQueryExplodePlan(collectionValue, true, true, false); Assertions.assertEquals(WITH_ZERO_BASED_ORDINALITY_LEGACY_HASH, withZeroBasedOrdinality.planHash(PlanHashable.CURRENT_LEGACY)); Assertions.assertEquals(WITH_ZERO_BASED_ORDINALITY_FOR_CONTINUATION_HASH, @@ -344,7 +346,7 @@ void dynamicTypesIncludeOrdinalityStruct() { @Nonnull @SuppressWarnings("DataFlowIssue") // explode transposes a constant array Value, it does not need a record store private static List ordinalsOf(@Nonnull final List elements, final boolean zeroBasedOrdinality) { - final var plan = new RecordQueryExplodePlan(LiteralValue.ofList(elements), true, zeroBasedOrdinality); + final var plan = new RecordQueryExplodePlan(LiteralValue.ofList(elements), true, zeroBasedOrdinality, false); final var resultType = plan.getExplodeResultType(); final var typeRepository = TypeRepository.newBuilder().addTypeIfNeeded(resultType).build(); final var descriptor = Objects.requireNonNull(typeRepository.getMessageDescriptor(resultType)); @@ -380,7 +382,7 @@ void explodeWithOrdinalityFlowsZeroBasedOrdinalsWhenAsked() { void zeroBasedOrdinalityRequiresOrdinality() { final var collectionValue = LiteralValue.ofList(List.of(1, 2, 3)); Assertions.assertThrows(VerifyException.class, - () -> new RecordQueryExplodePlan(collectionValue, false, true)); + () -> new RecordQueryExplodePlan(collectionValue, false, true, false)); Assertions.assertThrows(VerifyException.class, () -> new ExplodeExpression(collectionValue, false, true)); } @@ -396,7 +398,7 @@ void translateCorrelationsPreservesZeroBasedOrdinality() { final Value collectionValue = FieldValue.ofFieldName(qov, "arr"); final var translationMap = TranslationMap.ofAliases(sourceAlias, targetAlias); - final var plan = new RecordQueryExplodePlan(collectionValue, true, true); + final var plan = new RecordQueryExplodePlan(collectionValue, true, true, false); final var translatedPlan = plan.translateCorrelations(translationMap, true, List.of()); Assertions.assertNotSame(plan, translatedPlan); Assertions.assertTrue(translatedPlan.isWithOrdinality()); @@ -415,8 +417,9 @@ void ordinalBaseIsPartOfIdentity() { // The two variants flow different ordinals for the same array, so neither may stand in for the other, whether // as a plan or as an expression to be matched. - Assertions.assertNotEquals(new RecordQueryExplodePlan(collectionValue, true, false), - new RecordQueryExplodePlan(collectionValue, true, true)); + Assertions.assertNotEquals( + new RecordQueryExplodePlan(collectionValue, true, false, false), + new RecordQueryExplodePlan(collectionValue, true, true, false)); Assertions.assertFalse(new ExplodeExpression(collectionValue, true, false) .semanticEquals(new ExplodeExpression(collectionValue, true, true), AliasMap.emptyMap())); } @@ -426,8 +429,8 @@ void explainOutputDoesNotDistinguishTheOrdinalBase() { // The ordinal base does not show up in the explain output, so a plan that flows 0-based ordinals explains // exactly as one that flows 1-based ordinals, and no expected plan string changes on account of it. final var collectionValue = LiteralValue.ofList(List.of(1, 2, 3)); - final var oneBased = new RecordQueryExplodePlan(collectionValue, true, false); - final var zeroBased = new RecordQueryExplodePlan(collectionValue, true, true); + final var oneBased = new RecordQueryExplodePlan(collectionValue, true, false, false); + final var zeroBased = new RecordQueryExplodePlan(collectionValue, true, true, false); final String zeroBasedExplain = ExplainPlanVisitor.toStringForDebugging(zeroBased); Assertions.assertEquals(ExplainPlanVisitor.toStringForDebugging(oneBased), zeroBasedExplain); @@ -445,7 +448,7 @@ void protoRoundTripPreservesZeroBasedOrdinality() { final var qov = QuantifiedObjectValue.of(sourceAlias, recordType); final Value collectionValue = FieldValue.ofFieldName(qov, "arr"); - final RecordQueryExplodePlan zeroBased = new RecordQueryExplodePlan(collectionValue, true, true); + final RecordQueryExplodePlan zeroBased = new RecordQueryExplodePlan(collectionValue, true, true, false); final PRecordQueryExplodePlan zeroBasedProto = zeroBased.toProto(newSerializationContext()); Assertions.assertTrue(zeroBasedProto.getZeroBasedOrdinality()); final RecordQueryExplodePlan deserializedZeroBased = @@ -455,7 +458,7 @@ void protoRoundTripPreservesZeroBasedOrdinality() { // A plan flowing 1-based ordinals must not set the field at all, so that it serializes to exactly the bytes it // serialized to before the field existed. - final RecordQueryExplodePlan oneBased = new RecordQueryExplodePlan(collectionValue, true, false); + final RecordQueryExplodePlan oneBased = new RecordQueryExplodePlan(collectionValue, true, false, false); final PRecordQueryExplodePlan oneBasedProto = oneBased.toProto(newSerializationContext()); Assertions.assertFalse(oneBasedProto.hasZeroBasedOrdinality()); Assertions.assertFalse(RecordQueryExplodePlan.fromProto(newSerializationContext(), oneBasedProto) @@ -471,4 +474,162 @@ void protoRoundTripPreservesZeroBasedOrdinality() { Assertions.assertFalse(legacy.isZeroBasedOrdinality()); Assertions.assertEquals(oneBased, legacy); } + + @Test + void theFlagDecidesTheShapeOfTheFlowedValue() { + final var collectionValue = LiteralValue.ofList(List.of(1, 2, 3)); + final var elementValue = new RecordQueryExplodePlan(collectionValue, false).getResultValue(); + + // The shape existing plans get: one opaque value standing for the whole struct, with nothing inside it reachable. + final var opaque = new RecordQueryExplodePlan(collectionValue, true); + Assertions.assertFalse(opaque.flowsRecordConstructorValue()); + Assertions.assertInstanceOf(QueriedValue.class, opaque.getResultValue()); + + // As a record constructor, which is what makes the element reachable as its first column -- and so relatable to + // what a plain explode over the same collection flows. + final var recordConstructor = new RecordQueryExplodePlan(collectionValue, true, false, true); + final var columns = Assertions.assertInstanceOf(RecordConstructorValue.class, recordConstructor.getResultValue()) + .getColumns(); + Assertions.assertEquals(2, columns.size()); + Assertions.assertEquals(elementValue, columns.get(0).getValue()); + + // Either shape is of the type the plan looks its protobuf descriptor up by at run time. + Assertions.assertEquals(opaque.getExplodeResultType(), opaque.getResultValue().getResultType()); + Assertions.assertEquals(opaque.getExplodeResultType(), recordConstructor.getResultValue().getResultType()); + } + + @Test + void theShapeOfTheFlowedValueIsPartOfIdentity() { + final var collectionValue = LiteralValue.ofList(List.of(1, 2, 3)); + + // Two plans that flow the same data in different shapes are not interchangeable, since what is reachable in the + // value each flows differs, and neither may be substituted for the other under a continuation. + final var opaque = new RecordQueryExplodePlan(collectionValue, true); + final var recordConstructor = new RecordQueryExplodePlan(collectionValue, true, false, true); + Assertions.assertNotEquals(opaque, recordConstructor); + Assertions.assertNotEquals(opaque.planHash(PlanHashable.CURRENT_FOR_CONTINUATION), + recordConstructor.planHash(PlanHashable.CURRENT_FOR_CONTINUATION)); + + // The same holds of the plain variant, where the shape decides the type the plan flows as well. + final var plainElement = new RecordQueryExplodePlan(collectionValue, false); + final var plainRecordConstructor = new RecordQueryExplodePlan(collectionValue, false, false, true); + Assertions.assertNotEquals(plainElement, plainRecordConstructor); + Assertions.assertNotEquals(plainElement.planHash(PlanHashable.CURRENT_FOR_CONTINUATION), + plainRecordConstructor.planHash(PlanHashable.CURRENT_FOR_CONTINUATION)); + } + + @Test + void plainExplodeCanFlowARecordConstructorToo() { + final var collectionValue = LiteralValue.ofList(List.of(1, 2, 3)); + + // The shape existing plans get: the element itself, which is also the plan's result type. + final var element = new RecordQueryExplodePlan(collectionValue, false); + final var elementValue = element.getResultValue(); + Assertions.assertInstanceOf(QueriedValue.class, elementValue); + Assertions.assertEquals(element.getElementType(), element.getExplodeResultType()); + + // As a record constructor, the element is the only column, and the plan flows the struct holding it. + final var recordConstructor = new RecordQueryExplodePlan(collectionValue, false, false, true); + final var columns = Assertions.assertInstanceOf(RecordConstructorValue.class, recordConstructor.getResultValue()) + .getColumns(); + Assertions.assertEquals(1, columns.size()); + Assertions.assertEquals(elementValue, columns.get(0).getValue()); + Assertions.assertEquals( + Type.Record.fromFields(List.of(Type.Record.Field.of(element.getElementType(), Optional.empty()))), + recordConstructor.getExplodeResultType()); + Assertions.assertEquals(recordConstructor.getExplodeResultType(), + recordConstructor.getResultValue().getResultType()); + + // Unlike WITH ORDINALITY, where the struct is the result type either way, the shape decides the plain variant's + // result type. + Assertions.assertNotEquals(element.getExplodeResultType(), recordConstructor.getExplodeResultType()); + Assertions.assertEquals(new RecordQueryExplodePlan(collectionValue, true).getExplodeResultType(), + new RecordQueryExplodePlan(collectionValue, true, false, true).getExplodeResultType()); + + // An expression flows what the plan implementing it flows. + final var expression = new ExplodeExpression(collectionValue, false, false, true); + Assertions.assertTrue(expression.flowsRecordConstructorValue()); + Assertions.assertEquals(recordConstructor.getExplodeResultType(), expression.getExplodeResultType()); + Assertions.assertEquals(recordConstructor.getResultValue(), expression.getResultValue()); + } + + /** + * Runs a plain {@code EXPLODE} that flows a record constructor over the given elements, and returns the elements it + * produces, read out of the struct each datum is. + */ + @Nonnull + @SuppressWarnings("DataFlowIssue") // explode transposes a constant array Value, it does not need a record store + private static List structuredElementsOf(@Nonnull final List elements) { + final var plan = new RecordQueryExplodePlan(LiteralValue.ofList(elements), false, false, true); + final var resultType = plan.getExplodeResultType(); + final var typeRepository = TypeRepository.newBuilder().addTypeIfNeeded(resultType).build(); + final var descriptor = Objects.requireNonNull(typeRepository.getMessageDescriptor(resultType)); + Assertions.assertEquals(1, descriptor.getFields().size()); + final var elementField = descriptor.getFields().get(0); + final var cursor = plan.executePlan(null, EvaluationContext.forTypeRepository(typeRepository), null, + ExecuteProperties.newBuilder().build()); + final var flowedElements = ImmutableList.builder(); + for (int i = 0; i < elements.size(); i++) { + final var result = cursor.getNext(); + Assertions.assertTrue(result.hasNext()); + final var message = (Message)Objects.requireNonNull(result.get()).getDatum(); + flowedElements.add(message.getField(elementField)); + } + Assertions.assertFalse(cursor.getNext().hasNext()); + return flowedElements.build(); + } + + @Test + void plainExplodeFlowingARecordConstructorProducesStructs() { + // The plain variant produces the bare elements -- see explodeWithSkipAndLimitWorks -- unless it flows a record + // constructor, in which case each element comes wrapped in the struct that value declares. + Assertions.assertEquals(List.of(100, 200, 300), structuredElementsOf(List.of(100, 200, 300))); + } + + @Test + void protoRoundTripPreservesTheShapeOfTheFlowedValue() { + final var sourceAlias = CorrelationIdentifier.of("source"); + final var arrayType = new Type.Array(false, Type.primitiveType(Type.TypeCode.INT, false)); + final var recordType = Type.Record.fromFields(List.of( + Type.Record.Field.of(arrayType, Optional.of("arr")))); + final var qov = QuantifiedObjectValue.of(sourceAlias, recordType); + final Value collectionValue = FieldValue.ofFieldName(qov, "arr"); + + final var recordConstructor = new RecordQueryExplodePlan(collectionValue, true, false, true); + final var recordConstructorProto = recordConstructor.toProto(newSerializationContext()); + Assertions.assertTrue(recordConstructorProto.getFlowsRcv()); + final var deserializedRecordConstructor = + RecordQueryExplodePlan.fromProto(newSerializationContext(), recordConstructorProto); + Assertions.assertTrue(deserializedRecordConstructor.flowsRecordConstructorValue()); + Assertions.assertEquals(recordConstructor, deserializedRecordConstructor); + + // A plan flowing the opaque value must not set the field at all, so that it serializes to exactly the bytes it + // serialized to before the field existed. + final var opaque = new RecordQueryExplodePlan(collectionValue, true); + final var opaqueProto = opaque.toProto(newSerializationContext()); + Assertions.assertFalse(opaqueProto.hasFlowsRcv()); + + // ... and a plan serialized by such a version, which cannot have the field, deserializes to the opaque value. + final var legacyProto = PRecordQueryExplodePlan.newBuilder() + .setCollectionValue(collectionValue.toValueProto(newSerializationContext())) + .setWithOrdinality(true) + .build(); + final var legacy = RecordQueryExplodePlan.fromProto(newSerializationContext(), legacyProto); + Assertions.assertFalse(legacy.flowsRecordConstructorValue()); + Assertions.assertEquals(opaque, legacy); + + // The plain variant round-trips the same way, where the field decides whether a struct is flowed at all. + final var plainRecordConstructor = new RecordQueryExplodePlan(collectionValue, false, false, true); + final var plainRecordConstructorProto = plainRecordConstructor.toProto(newSerializationContext()); + Assertions.assertTrue(plainRecordConstructorProto.getFlowsRcv()); + Assertions.assertEquals(plainRecordConstructor, + RecordQueryExplodePlan.fromProto(newSerializationContext(), plainRecordConstructorProto)); + + final var plainElement = new RecordQueryExplodePlan(collectionValue, false); + Assertions.assertFalse(plainElement.toProto(newSerializationContext()).hasFlowsRcv()); + Assertions.assertEquals(plainElement, RecordQueryExplodePlan.fromProto(newSerializationContext(), + PRecordQueryExplodePlan.newBuilder() + .setCollectionValue(collectionValue.toValueProto(newSerializationContext())) + .build())); + } } From ccd91637c53b8bbd70bffdbfe344fe266649a44e Mon Sep 17 00:00:00 2001 From: Pranjal Gupta Date: Thu, 24 Sep 2026 16:31:26 +0100 Subject: [PATCH 2/3] update --- .../cascades/expressions/ExplodeExpression.java | 8 +------- .../cascades/properties/DerivationsProperty.java | 2 +- .../cascades/rules/ImplementExplodeRule.java | 3 ++- .../query/plan/plans/RecordQueryExplodePlan.java | 16 ++++++++-------- .../src/main/proto/record_query_plan.proto | 2 +- .../record/query/plan/plans/ExplodePlanTest.java | 8 ++++---- 6 files changed, 17 insertions(+), 22 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java index 4283a46b234..f456164a836 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java @@ -187,13 +187,6 @@ public static Value explodeResultValue(@Nonnull final Type elementType, final bo if (!flowsRecordConstructorValue) { return new QueriedValue(explodeResultType(elementType, withOrdinality, false)); } - // Note: the element must stay the first column. `MaxMatchMap` returns the first reachable candidate value - // that compares equal, and a `QueriedValue` has no identity beyond its class and result type, so an element - // whose type is also a non-nullable `INT` -- the ordinal's type -- would just as happily match the ordinal - // if the ordinal came first. - // - // The record is built nullable because `explodeResultType` declares it that way, and the callers verify - // that the two agree: that declared type is what the plan looks its protobuf descriptor up by at run time. final var columns = ImmutableList.>builder(); columns.add(Column.unnamedOf(new QueriedValue(elementType))); if (withOrdinality) { @@ -253,6 +246,7 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi return collectionValue.semanticEquals(other.getCollectionValue(), equivalencesMap) && isWithOrdinality() == other.isWithOrdinality() && isZeroBasedOrdinality() == other.isZeroBasedOrdinality() && + flowsRecordConstructorValue() == other.flowsRecordConstructorValue() && semanticEqualsForResults(otherExpression, equivalencesMap); } return false; diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java index b6192c59a38..2c822fb8699 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/properties/DerivationsProperty.java @@ -361,7 +361,7 @@ public Derivations visitExplodePlan(@Nonnull final RecordQueryExplodePlan explod columns.add(Column.of(Type.Record.Field.of(ordinalType, Optional.empty()), new ThrowsValue(ordinalType))); } - representative = RecordConstructorValue.ofColumns(columns.build()); + representative = RecordConstructorValue.ofColumns(columns.build(), true); } else { representative = first; } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java index 87bf87df14f..dbe74ff3567 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java @@ -50,7 +50,8 @@ public void onMatch(@Nonnull final ImplementationCascadesRuleCall call) { final Value collectionValue = explodeExpression.getCollectionValue(); final boolean isWithOrdinality = explodeExpression.isWithOrdinality(); final boolean isZeroBasedOrdinality = explodeExpression.isZeroBasedOrdinality(); + final boolean flowsRecordConstructorValue = explodeExpression.flowsRecordConstructorValue(); call.yieldPlan(new RecordQueryExplodePlan(collectionValue, isWithOrdinality, isZeroBasedOrdinality, - explodeExpression.flowsRecordConstructorValue())); + flowsRecordConstructorValue)); } } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java index 0b85a8b9512..a0dc2046a0d 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java @@ -42,6 +42,7 @@ import com.apple.foundationdb.record.query.plan.cascades.explain.NodeInfo; import com.apple.foundationdb.record.query.plan.cascades.explain.PlannerGraph; import com.apple.foundationdb.record.query.plan.cascades.expressions.AbstractRelationalExpressionWithoutChildren; +import com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression; import com.apple.foundationdb.record.query.plan.cascades.expressions.RelationalExpression; import com.apple.foundationdb.record.query.plan.cascades.typing.Type; import com.apple.foundationdb.record.query.plan.cascades.typing.TypeRepository; @@ -64,9 +65,6 @@ import java.util.Set; import java.util.stream.IntStream; -import static com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression.explodeResultType; -import static com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression.explodeResultValue; - /** * An {@code EXPLODE} query plan. * @@ -139,8 +137,8 @@ public RecordQueryExplodePlan(@Nonnull Value collectionValue, boolean withOrdina Verify.verify(collectionValue.getResultType().isArray()); this.elementType = Objects.requireNonNull(((Type.Array)collectionValue.getResultType()).getElementType()); this.flowsRecordConstructorValue = flowsRecordConstructorValue; - this.explodeResultType = explodeResultType(elementType, withOrdinality, flowsRecordConstructorValue); - this.resultValue = explodeResultValue(elementType, withOrdinality, flowsRecordConstructorValue); + this.explodeResultType = ExplodeExpression.explodeResultType(elementType, withOrdinality, flowsRecordConstructorValue); + this.resultValue = ExplodeExpression.explodeResultValue(elementType, withOrdinality, flowsRecordConstructorValue); Verify.verify(explodeResultType.equals(resultValue.getResultType())); } @@ -332,6 +330,7 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi return collectionValue.semanticEquals(otherExplodePlan.getCollectionValue(), equivalencesMap) && isWithOrdinality() == otherExplodePlan.isWithOrdinality() && isZeroBasedOrdinality() == otherExplodePlan.isZeroBasedOrdinality() && + flowsRecordConstructorValue() == otherExplodePlan.flowsRecordConstructorValue() && semanticEqualsForResults(otherExpression, equivalencesMap); } @@ -402,7 +401,7 @@ public PRecordQueryExplodePlan toProto(@Nonnull final PlanSerializationContext s builder.setZeroBasedOrdinality(true); } if (flowsRecordConstructorValue) { - builder.setFlowsRcv(true); + builder.setFlowsRecordConstructorValue(true); } return builder.build(); } @@ -418,9 +417,10 @@ public static RecordQueryExplodePlan fromProto(@Nonnull final PlanSerializationC @Nonnull final PRecordQueryExplodePlan proto) { return new RecordQueryExplodePlan( Value.fromValueProto(serializationContext, Objects.requireNonNull(proto.getCollectionValue())), - // defaults to false, which is what a plan serialized before these fields existed flows + // The following flags are optional and hence can be unset. This is intentional, to maintain + // compatibility with what a plan serialized before these fields existed flows. proto.getWithOrdinality(), proto.getZeroBasedOrdinality(), - proto.getFlowsRcv()); + proto.getFlowsRecordConstructorValue()); } /** diff --git a/fdb-record-layer-core/src/main/proto/record_query_plan.proto b/fdb-record-layer-core/src/main/proto/record_query_plan.proto index c12ff88160d..b56f67a7f16 100644 --- a/fdb-record-layer-core/src/main/proto/record_query_plan.proto +++ b/fdb-record-layer-core/src/main/proto/record_query_plan.proto @@ -1884,7 +1884,7 @@ message PRecordQueryExplodePlan { optional PValue collection_value = 1; optional bool with_ordinality = 2; optional bool zero_based_ordinality = 3; - optional bool flows_rcv = 4; + optional bool flows_record_constructor_value = 4; } // diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java index 28b1150a190..406c38614bf 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java @@ -597,7 +597,7 @@ void protoRoundTripPreservesTheShapeOfTheFlowedValue() { final var recordConstructor = new RecordQueryExplodePlan(collectionValue, true, false, true); final var recordConstructorProto = recordConstructor.toProto(newSerializationContext()); - Assertions.assertTrue(recordConstructorProto.getFlowsRcv()); + Assertions.assertTrue(recordConstructorProto.getFlowsRecordConstructorValue()); final var deserializedRecordConstructor = RecordQueryExplodePlan.fromProto(newSerializationContext(), recordConstructorProto); Assertions.assertTrue(deserializedRecordConstructor.flowsRecordConstructorValue()); @@ -607,7 +607,7 @@ void protoRoundTripPreservesTheShapeOfTheFlowedValue() { // serialized to before the field existed. final var opaque = new RecordQueryExplodePlan(collectionValue, true); final var opaqueProto = opaque.toProto(newSerializationContext()); - Assertions.assertFalse(opaqueProto.hasFlowsRcv()); + Assertions.assertFalse(opaqueProto.hasFlowsRecordConstructorValue()); // ... and a plan serialized by such a version, which cannot have the field, deserializes to the opaque value. final var legacyProto = PRecordQueryExplodePlan.newBuilder() @@ -621,12 +621,12 @@ void protoRoundTripPreservesTheShapeOfTheFlowedValue() { // The plain variant round-trips the same way, where the field decides whether a struct is flowed at all. final var plainRecordConstructor = new RecordQueryExplodePlan(collectionValue, false, false, true); final var plainRecordConstructorProto = plainRecordConstructor.toProto(newSerializationContext()); - Assertions.assertTrue(plainRecordConstructorProto.getFlowsRcv()); + Assertions.assertTrue(plainRecordConstructorProto.getFlowsRecordConstructorValue()); Assertions.assertEquals(plainRecordConstructor, RecordQueryExplodePlan.fromProto(newSerializationContext(), plainRecordConstructorProto)); final var plainElement = new RecordQueryExplodePlan(collectionValue, false); - Assertions.assertFalse(plainElement.toProto(newSerializationContext()).hasFlowsRcv()); + Assertions.assertFalse(plainElement.toProto(newSerializationContext()).hasFlowsRecordConstructorValue()); Assertions.assertEquals(plainElement, RecordQueryExplodePlan.fromProto(newSerializationContext(), PRecordQueryExplodePlan.newBuilder() .setCollectionValue(collectionValue.toValueProto(newSerializationContext())) From 91320b13525da7fcfb84c2174fc4604368aa6748 Mon Sep 17 00:00:00 2001 From: Pranjal Gupta Date: Fri, 25 Sep 2026 13:53:48 +0100 Subject: [PATCH 3/3] address comments --- .../expressions/ExplodeExpression.java | 20 +++++++++++------- .../query/plan/plans/ExplodePlanTest.java | 21 +++++++++++++++++-- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java index f456164a836..d389612444d 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java @@ -254,14 +254,20 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi @Override public int computeHashCodeWithoutChildren() { - // Note: This is written in a way that preserves pre-existing hashes for `withOrdinality=false` and for - // (`withOrdinality=true` and `zeroBasedOrdinality=false`) - if (!withOrdinality) { - return Objects.hash(collectionValue); + // Note: This is written in a way that preserves the hashes of every combination that could be expressed before + // an explode could flow a record constructor: that flag only ever appends to what was hashed before, so with + // the flag unset nothing is appended at all. + final var hashedObjects = ImmutableList.builder().add(collectionValue); + if (withOrdinality) { + hashedObjects.add(true); + if (zeroBasedOrdinality) { + hashedObjects.add(true); + } + } + if (flowsRecordConstructorValue) { + hashedObjects.add("flowsRecordConstructorValue"); } - return zeroBasedOrdinality - ? Objects.hash(collectionValue, true, true) - : Objects.hash(collectionValue, true); + return Objects.hash(hashedObjects.build().toArray()); } @Nonnull diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java index 406c38614bf..d86535ff086 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/ExplodePlanTest.java @@ -348,7 +348,7 @@ void dynamicTypesIncludeOrdinalityStruct() { private static List ordinalsOf(@Nonnull final List elements, final boolean zeroBasedOrdinality) { final var plan = new RecordQueryExplodePlan(LiteralValue.ofList(elements), true, zeroBasedOrdinality, false); final var resultType = plan.getExplodeResultType(); - final var typeRepository = TypeRepository.newBuilder().addTypeIfNeeded(resultType).build(); + final var typeRepository = TypeRepository.newBuilder().addAllTypes(plan.getDynamicTypes()).build(); final var descriptor = Objects.requireNonNull(typeRepository.getMessageDescriptor(resultType)); final var elementField = descriptor.getFields().get(0); final var ordinalField = descriptor.getFields().get(1); @@ -496,6 +496,9 @@ void theFlagDecidesTheShapeOfTheFlowedValue() { // Either shape is of the type the plan looks its protobuf descriptor up by at run time. Assertions.assertEquals(opaque.getExplodeResultType(), opaque.getResultValue().getResultType()); Assertions.assertEquals(opaque.getExplodeResultType(), recordConstructor.getResultValue().getResultType()); + + Assertions.assertTrue(opaque.getDynamicTypes().contains(opaque.getExplodeResultType())); + Assertions.assertTrue(recordConstructor.getDynamicTypes().contains(recordConstructor.getExplodeResultType())); } @Test @@ -518,6 +521,19 @@ void theShapeOfTheFlowedValueIsPartOfIdentity() { plainRecordConstructor.planHash(PlanHashable.CURRENT_FOR_CONTINUATION)); } + @Test + void theShapeOfTheFlowedValueEntersTheSemanticHash() { + final var collectionValue = LiteralValue.ofList(List.of(1, 2, 3)); + + Assertions.assertEquals(new ExplodeExpression(collectionValue, true, false, true).semanticHashCode(), + new ExplodeExpression(collectionValue, true, false, true).semanticHashCode()); + + Assertions.assertNotEquals(new ExplodeExpression(collectionValue, true).semanticHashCode(), + new ExplodeExpression(collectionValue, true, false, true).semanticHashCode()); + Assertions.assertNotEquals(new ExplodeExpression(collectionValue).semanticHashCode(), + new ExplodeExpression(collectionValue, false, false, true).semanticHashCode()); + } + @Test void plainExplodeCanFlowARecordConstructorToo() { final var collectionValue = LiteralValue.ofList(List.of(1, 2, 3)); @@ -545,6 +561,7 @@ void plainExplodeCanFlowARecordConstructorToo() { Assertions.assertNotEquals(element.getExplodeResultType(), recordConstructor.getExplodeResultType()); Assertions.assertEquals(new RecordQueryExplodePlan(collectionValue, true).getExplodeResultType(), new RecordQueryExplodePlan(collectionValue, true, false, true).getExplodeResultType()); + Assertions.assertTrue(recordConstructor.getDynamicTypes().contains(recordConstructor.getExplodeResultType())); // An expression flows what the plan implementing it flows. final var expression = new ExplodeExpression(collectionValue, false, false, true); @@ -562,7 +579,7 @@ void plainExplodeCanFlowARecordConstructorToo() { private static List structuredElementsOf(@Nonnull final List elements) { final var plan = new RecordQueryExplodePlan(LiteralValue.ofList(elements), false, false, true); final var resultType = plan.getExplodeResultType(); - final var typeRepository = TypeRepository.newBuilder().addTypeIfNeeded(resultType).build(); + final var typeRepository = TypeRepository.newBuilder().addAllTypes(plan.getDynamicTypes()).build(); final var descriptor = Objects.requireNonNull(typeRepository.getMessageDescriptor(resultType)); Assertions.assertEquals(1, descriptor.getFields().size()); final var elementField = descriptor.getFields().get(0);