Stop supporting 1-based explode ordinals

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java

@@ -57,8 +57,8 @@
  * A table function expression that “explodes” a repeated field into a stream of its values.
  *
  * <p>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}.
+ * are 0-based.
  *
  * <p>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.
@@ -73,11 +73,6 @@
      */
     private final boolean withOrdinality;
 
-    /**
-     * Whether the ordinals produced are 0-based rather than 1-based.
-     */
-    private final boolean zeroBasedOrdinality;
-
     /**
      * The element type of the collection value.
      */
@@ -103,11 +98,9 @@
     private final Value resultValue;
 
     public ExplodeExpression(@Nonnull final Value collectionValue, final boolean withOrdinality,
-                             final boolean zeroBasedOrdinality, final boolean flowsRecordConstructorValue) {
-        Verify.verify(withOrdinality || !zeroBasedOrdinality, "cannot base ordinals that are not produced");
+                             final boolean flowsRecordConstructorValue) {
         this.collectionValue = collectionValue;
         this.withOrdinality = withOrdinality;
-        this.zeroBasedOrdinality = zeroBasedOrdinality;
         Verify.verify(collectionValue.getResultType().isArray());
         this.elementType = Objects.requireNonNull(((Type.Array)collectionValue.getResultType()).getElementType());
         this.flowsRecordConstructorValue = flowsRecordConstructorValue;
@@ -116,17 +109,12 @@
         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) {
         this(collectionValue, withOrdinality, false);
     }
 
     public ExplodeExpression(@Nonnull final Value collectionValue) {
-        this(collectionValue, false, false);
+        this(collectionValue, false);
     }
 
     /**
@@ -210,10 +198,6 @@
         return withOrdinality;
     }
 
-    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
@@ -245,7 +229,6 @@
         if (otherExpression instanceof final ExplodeExpression other) {
             return collectionValue.semanticEquals(other.getCollectionValue(), equivalencesMap) &&
                     isWithOrdinality() == other.isWithOrdinality() &&
-                    isZeroBasedOrdinality() == other.isZeroBasedOrdinality() &&
                     flowsRecordConstructorValue() == other.flowsRecordConstructorValue() &&
                     semanticEqualsForResults(otherExpression, equivalencesMap);
         }
@@ -254,15 +237,12 @@
 
     @Override
     public int computeHashCodeWithoutChildren() {
-        // 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.
+        // Note: The `if` blocks below are written to preserve the hashes produced by past versions of `ExplodeExpression`.
         final var hashedObjects = ImmutableList.builder().add(collectionValue);
         if (withOrdinality) {
-            hashedObjects.add(true);
-            if (zeroBasedOrdinality) {
-                hashedObjects.add(true);
-            }
+            hashedObjects.add(true);  // withOrdinality
+            // Add `true` to preserve the hashes from before PR #4704, when `zeroBasedOrdinality` was still a choice.
+            hashedObjects.add(true);  // zeroBasedOrdinality
         }
         if (flowsRecordConstructorValue) {
             hashedObjects.add("flowsRecordConstructorValue");
@@ -281,8 +261,7 @@
                 collectionValue.translateCorrelations(translationMap, shouldSimplifyValues);
         // this is ok since there are no new quantifiers
         if (translatedCollectionValue != collectionValue) {
-            return new ExplodeExpression(translatedCollectionValue, withOrdinality, zeroBasedOrdinality,
-                    flowsRecordConstructorValue);
+            return new ExplodeExpression(translatedCollectionValue, withOrdinality, flowsRecordConstructorValue);
         }
         return this;
     }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementExplodeRule.java

@@ -49,9 +49,7 @@
         final ExplodeExpression explodeExpression = call.get(root);
         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,
-                flowsRecordConstructorValue));
+        call.yieldPlan(new RecordQueryExplodePlan(collectionValue, isWithOrdinality, flowsRecordConstructorValue));
     }
 }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java

@@ -26,7 +26,8 @@
 import com.apple.foundationdb.record.ObjectPlanHash;
 import com.apple.foundationdb.record.PlanDeserializer;
 import com.apple.foundationdb.record.PlanHashable;
 import com.apple.foundationdb.record.PlanSerializationContext;
+import com.apple.foundationdb.record.RecordCoreException;
 import com.apple.foundationdb.record.RecordCursor;
 import com.apple.foundationdb.record.planprotos.PRecordQueryExplodePlan;
 import com.apple.foundationdb.record.planprotos.PRecordQueryPlan;
@@ -75,7 +76,7 @@
  *
  * <p>In the {@code WITH ORDINALITY} variant, {@code EXPLODE} also generates ordinals of the array elements. In this
  * 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()}.
+ * instead of the bare element. The ordinals are 0-based.
  *
  * <p>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
@@ -99,11 +100,6 @@
      */
     private final boolean withOrdinality;
 
-    /**
-     * Whether the ordinals produced are 0-based rather than 1-based.
-     */
-    private final boolean zeroBasedOrdinality;
-
     /**
      * The element type of the collection value.
      */
@@ -129,11 +125,9 @@
     private final Value resultValue;
 
     public RecordQueryExplodePlan(@Nonnull Value collectionValue, boolean withOrdinality,
-                                  boolean zeroBasedOrdinality, boolean flowsRecordConstructorValue) {
-        Verify.verify(withOrdinality || !zeroBasedOrdinality, "cannot base ordinals that are not produced");
+                                  boolean flowsRecordConstructorValue) {
         this.collectionValue = collectionValue;
         this.withOrdinality = withOrdinality;
-        this.zeroBasedOrdinality = zeroBasedOrdinality;
         Verify.verify(collectionValue.getResultType().isArray());
         this.elementType = Objects.requireNonNull(((Type.Array)collectionValue.getResultType()).getElementType());
         this.flowsRecordConstructorValue = flowsRecordConstructorValue;
@@ -143,7 +137,7 @@
     }
 
     public RecordQueryExplodePlan(@Nonnull Value collectionValue, boolean withOrdinality) {
-        this(collectionValue, withOrdinality, false, false);
+        this(collectionValue, withOrdinality, false);
     }
 
     public RecordQueryExplodePlan(@Nonnull Value collectionValue) {
@@ -159,10 +153,6 @@
         return withOrdinality;
     }
 
-    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
@@ -196,7 +186,6 @@
         final Descriptors.Descriptor descriptor = Objects.requireNonNull(typeRepository.getMessageDescriptor(resultType));
         final Descriptors.FieldDescriptor elementField = descriptor.getFields().get(0);
         final Descriptors.FieldDescriptor ordinalField = withOrdinality ? descriptor.getFields().get(1) : null;
-        final int firstOrdinal = zeroBasedOrdinality ? 0 : 1;
         final ImmutableList<Message> structs =
                 IntStream.range(0, list.size())
                 .mapToObj(i -> {
@@ -205,7 +194,7 @@
                     builder.setField(elementField,
                             RecordConstructorValue.deepCopyIfNeeded(typeRepository, elementType, element));
                     if (ordinalField != null) {
-                        builder.setField(ordinalField, firstOrdinal + i);
+                        builder.setField(ordinalField, i);
                     }
                     return (Message)builder.build();
                 })
@@ -231,7 +220,7 @@
         final Value translatedCollectionValue =
                 collectionValue.translateCorrelations(translationMap, shouldSimplifyValues);
         if (translatedCollectionValue != collectionValue) {
-            return new RecordQueryExplodePlan(translatedCollectionValue, withOrdinality, zeroBasedOrdinality,
+            return new RecordQueryExplodePlan(translatedCollectionValue, withOrdinality,
                     flowsRecordConstructorValue);
         }
         return this;
@@ -329,7 +318,6 @@
         final var otherExplodePlan = (RecordQueryExplodePlan)otherExpression;
         return collectionValue.semanticEquals(otherExplodePlan.getCollectionValue(), equivalencesMap) &&
                 isWithOrdinality() == otherExplodePlan.isWithOrdinality() &&
-                isZeroBasedOrdinality() == otherExplodePlan.isZeroBasedOrdinality() &&
                 flowsRecordConstructorValue() == otherExplodePlan.flowsRecordConstructorValue() &&
                 semanticEqualsForResults(otherExpression, equivalencesMap);
     }
@@ -347,8 +335,9 @@
 
     @Override
     public int computeHashCodeWithoutChildren() {
-        // Note: This is written in a way that preserves pre-existing hashes for `zeroBasedOrdinality=false`.
-        return zeroBasedOrdinality ? Objects.hash(getResultValue(), true) : Objects.hash(getResultValue());
+        // Note: A WITH ORDINALITY explode hashes the way one that was explicitly asked for 0-based ordinals hashed
+        // while the ordinal base was still a choice, so that plans hash as the version that emitted them hashed them.
+        return withOrdinality ? Objects.hash(getResultValue(), true) : Objects.hash(getResultValue());
     }
 
     @Override
@@ -369,8 +358,7 @@
                 if (!withOrdinality) {
                     return PlanHashable.objectsPlanHash(mode, BASE_HASH, result);
                 }
-                return zeroBasedOrdinality ? PlanHashable.objectsPlanHash(mode, BASE_HASH, result, true, true)
-                                           : PlanHashable.objectsPlanHash(mode, BASE_HASH, result, true);
+                return PlanHashable.objectsPlanHash(mode, BASE_HASH, result, true, true);
             }
             default -> throw new UnsupportedOperationException("Hash kind " + mode.getKind() + " is not supported");
         }
@@ -396,8 +384,7 @@
         builder.setCollectionValue(collectionValue.toValueProto(serializationContext));
         if (withOrdinality) {
             builder.setWithOrdinality(true);
-        }
-        if (zeroBasedOrdinality) {
+            // For compatibility with older versions that still read the `zero_based_ordinality` field:
             builder.setZeroBasedOrdinality(true);
         }
         if (flowsRecordConstructorValue) {
@@ -415,12 +402,18 @@
     @Nonnull
     public static RecordQueryExplodePlan fromProto(@Nonnull final PlanSerializationContext serializationContext,
                                                    @Nonnull final PRecordQueryExplodePlan proto) {
+        final boolean withOrdinality = proto.getWithOrdinality();
+        // We only support 0-based ordinals. Therefore:
+        // * Reject plans before version 4.15.1.0 that had no `zero_based_ordinality` field yet (and produced 1-based ordinals).
+        // * Reject plans from version ≥4.15.1.0 that do not set `zero_based_ordinality` explicitly to true.
+        if (withOrdinality && (!proto.hasZeroBasedOrdinality() || !proto.getZeroBasedOrdinality())) {
+            throw new RecordCoreException("explode plans flowing 1-based ordinals are no longer supported");
+        }
         return new RecordQueryExplodePlan(
                 Value.fromValueProto(serializationContext, Objects.requireNonNull(proto.getCollectionValue())),
-                // 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.getFlowsRecordConstructorValue());
+                // The record constructor flag is optional and hence can be unset. This is intentional, to maintain
+                // compatibility with what a plan serialized before that field existed flows.
+                withOrdinality, proto.getFlowsRecordConstructorValue());
     }
 
     /**