Key the index-entry-to-record translation by SQL field name

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/IndexPredicate.java

@@ -173,8 +173,8 @@
         if (argumentValues.size() != 1 || !(argumentValues.get(0) instanceof FieldValue)) {
             return null;
         }
         final var fieldValue = (FieldValue)argumentValues.get(0);
-        final var fieldNames = fieldValue.getFieldPathNames();
+        final var fieldNames = fieldValue.getFieldPathStorageNames();
         if (fieldNames.isEmpty()) {
             return null;
         }
@@ -195,7 +195,7 @@
             if (!(pv instanceof FieldValue)) {
                 return null;
             }
-            final var partFieldNames = ((FieldValue)pv).getFieldPathNames();
+            final var partFieldNames = ((FieldValue)pv).getFieldPathStorageNames();
             if (partFieldNames.isEmpty()) {
                 return null;
             }
@@ -228,7 +228,7 @@
             }
             return IndexComparison.isSupported(valuePredicate.getComparison()) &&
                    valuePredicate.getValue() instanceof FieldValue &&
-                   ((FieldValue)valuePredicate.getValue()).getFieldPathNamesMaybe().stream().allMatch(Optional::isPresent);
+                   ((FieldValue)valuePredicate.getValue()).getFieldPathStorageNamesMaybe().stream().allMatch(Optional::isPresent);
         } else {
             return false;
         }
@@ -564,7 +564,7 @@
         @SuppressWarnings("java:S5803")
         ValuePredicate(@Nonnull final com.apple.foundationdb.record.query.plan.cascades.predicates.ValuePredicate predicate) {
             Verify.verify(predicate.getValue() instanceof FieldValue);
-            this.fieldPath = ImmutableList.copyOf(((FieldValue)predicate.getValue()).getFieldPathNames());
+            this.fieldPath = ImmutableList.copyOf(((FieldValue)predicate.getValue()).getFieldPathStorageNames());
             this.comparison = IndexComparison.fromComparison(predicate.getComparison());
         }
 

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

@@ -107,8 +107,8 @@
 
         final var parentBuilderForField = parentBuilderForFieldOptional.get();
 
         // TODO not sure what to do with the null standing requirement
-        final var maybeFieldName = fieldValue.getLastFieldName();
+        final var maybeFieldName = fieldValue.getFieldPath().getLastFieldStorageName();
         if (maybeFieldName.isEmpty()) {
             return false;
         }
@@ -123,7 +123,7 @@
     private static Optional<IndexKeyValueToPartialRecord.Builder> getParentBuilderForFieldMaybe(@Nonnull IndexKeyValueToPartialRecord.Builder builder,
                                                                                                 @Nonnull final FieldValue fieldValue) {
         // TODO field names are for debugging purposes only, we should probably use field ordinals here instead.
-        for (final var maybeFieldName : fieldValue.getFieldPrefix().getOptionalFieldNames()) {
+        for (final var maybeFieldName : fieldValue.getFieldPrefix().getOptionalFieldStorageNames()) {
             if (maybeFieldName.isEmpty()) {
                 return Optional.empty();
             }
@@ -204,7 +204,7 @@
         for (final var prefixFieldName : fieldValue.getFieldPrefix().getOptionalFieldNames()) {
             node = node.withChild(prefixFieldName.orElseThrow());
         }
-        node.withChild(fieldValue.getLastFieldName().orElseThrow()).cover(extraction.getValue());
+        node.withChild(fieldValue.getFieldPath().getLastFieldName().orElseThrow()).cover(extraction.getValue());
     }
 
     /**

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/matching/structure/ValueMatchers.java

@@ -141,8 +141,8 @@
                         Extractor.of(FieldValue::getChild, name -> "child(" + name + ")"),
                         downstreamValue);
         final TypedMatcherWithExtractAndDownstream<FieldValue> downstreamFieldPathMatcher =
                 typedWithDownstream(FieldValue.class,
-                        Extractor.of(FieldValue::getFieldPathNames, name -> "fieldPathNames(" + name + ")"),
+                        Extractor.of(FieldValue::getFieldPathStorageNames, name -> "fieldPathNames(" + name + ")"),
                         downstreamFieldPath);
 
         return typedWithDownstream(FieldValue.class,
@@ -160,7 +160,7 @@
         final TypedMatcherWithExtractAndDownstream<FieldValue> downstreamFieldPathMatcher =
                 typedWithDownstream(FieldValue.class,
                         Extractor.of(fieldValue -> {
-                            final var fieldPathNames = fieldValue.getFieldPathNames();
+                            final var fieldPathNames = fieldValue.getFieldPathStorageNames();
                             return fieldPathNames.get(fieldPathNames.size() - 1);
                         }, name -> "fieldPathNames(" + name + ")"),
                         downstreamFieldNameMatcher);

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

@@ -180,8 +180,8 @@
                                     return Stream.empty();
                                 }
 
                                 final var fieldValue = (FieldValue)valuePredicate.getValue();
-                                if (fieldValue.getFieldPathNamesMaybe()
+                                if (fieldValue.getFieldPathStorageNamesMaybe()
                                         .stream()
                                         .anyMatch(Optional::isEmpty)) {
                                     return Stream.empty();

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

@@ -84,16 +84,16 @@
     @Nonnull
     private final FieldPath fieldPath;
 
     @Nonnull
-    private final Supplier<List<String>> fieldNamesSupplier;
+    private final Supplier<List<String>> fieldStorageNamesSupplier;
     @Nonnull
     private final Supplier<Type> resultTypeSupplier;
 
     private FieldValue(@Nonnull final Value childValue, @Nonnull final FieldPath fieldPath) {
         this.childValue = childValue;
         this.fieldPath = fieldPath;
-        fieldNamesSupplier = Suppliers.memoize(() ->
-                fieldPath.getOptionalFieldNames()
+        fieldStorageNamesSupplier = Suppliers.memoize(() ->
+                fieldPath.getOptionalFieldStorageNames()
                         .stream()
                         .map(maybe -> maybe.orElseThrow(() -> new RecordCoreException("field name should have been set")))
                         .collect(Collectors.toList()));
@@ -105,9 +105,12 @@
         return fieldPath;
     }
 
+    /**
+     * The field names as spelled in the record descriptor.
+     */
     @Nonnull
-    public List<String> getFieldPathNames() {
-        return fieldNamesSupplier.get();
+    public List<String> getFieldPathStorageNames() {
+        return fieldStorageNamesSupplier.get();
     }
 
     @Nonnull
@@ -120,21 +123,19 @@
         return fieldPath.getFieldOrdinals();
     }
 
+    /**
+     * The field names as spelled in the record descriptor.
+     */
     @Nonnull
-    public List<Optional<String>> getFieldPathNamesMaybe() {
-        return fieldPath.getOptionalFieldNames();
+    public List<Optional<String>> getFieldPathStorageNamesMaybe() {
+        return fieldPath.getOptionalFieldStorageNames();
     }
 
     @Nonnull
     public FieldPath getFieldPrefix() {
         return fieldPath.getFieldPrefix();
     }
 
-    @Nonnull
-    public Optional<String> getLastFieldName() {
-        return fieldPath.getLastFieldName();
-    }
-
     @Nonnull
     @Override
     public Type getResultType() {
@@ -379,6 +380,9 @@
         @Nonnull
         private final List<ResolvedAccessor> fieldAccessors;
 
+        @Nonnull
+        private final Supplier<List<Optional<String>>> fieldStorageNamesSupplier;
+
         @Nonnull
         private final Supplier<List<Optional<String>>> fieldNamesSupplier;
 
@@ -397,6 +401,7 @@
 
         public FieldPath(@Nonnull final List<ResolvedAccessor> fieldAccessors) {
             this.fieldAccessors = ImmutableList.copyOf(fieldAccessors);
+            this.fieldStorageNamesSupplier = Suppliers.memoize(() -> computeFieldStorageNames(fieldAccessors));
             this.fieldNamesSupplier = Suppliers.memoize(() -> computeFieldNames(fieldAccessors));
             this.fieldOrdinalsSupplier = Suppliers.memoize(() -> computeOrdinals(fieldAccessors));
             this.fieldTypesSupplier = Suppliers.memoize(() -> computeFieldTypes(fieldAccessors));
@@ -432,6 +437,17 @@
                     .collect(Collectors.joining());
         }
 
+        /**
+         * The field names as spelled in the record descriptor.
+         */
+        @Nonnull
+        public List<Optional<String>> getOptionalFieldStorageNames() {
+            return fieldStorageNamesSupplier.get();
+        }
+
+        /**
+         * The field names as spelled in SQL.
+         */
         @Nonnull
         public List<Optional<String>> getOptionalFieldNames() {
             return fieldNamesSupplier.get();
@@ -459,6 +475,18 @@
             return getFieldAccessors().get(size() - 1);
         }
 
+        /**
+         * The last field's name as spelled in the record descriptor.
+         */
+        @Nonnull
+        public Optional<String> getLastFieldStorageName() {
+            Preconditions.checkArgument(!isEmpty());
+            return getOptionalFieldStorageNames().get(size() - 1);
+        }
+
+        /**
+         * The last field's name as spelled in SQL.
+         */
         @Nonnull
         public Optional<String> getLastFieldName() {
             Preconditions.checkArgument(!isEmpty());
@@ -539,12 +567,19 @@
         }
 
         @Nonnull
-        private static List<Optional<String>> computeFieldNames(@Nonnull final List<ResolvedAccessor> fieldAccessors) {
+        private static List<Optional<String>> computeFieldStorageNames(@Nonnull final List<ResolvedAccessor> fieldAccessors) {
             return fieldAccessors.stream()
                     .map(accessor -> accessor.getField().getFieldStorageNameOptional())
                     .collect(ImmutableList.toImmutableList());
         }
 
+        @Nonnull
+        private static List<Optional<String>> computeFieldNames(@Nonnull final List<ResolvedAccessor> fieldAccessors) {
+            return fieldAccessors.stream()
+                    .map(accessor -> accessor.getField().getFieldNameOptional())
+                    .collect(ImmutableList.toImmutableList());
+        }
+
         @Nonnull
         private static ImmutableIntArray computeOrdinals(@Nonnull final List<ResolvedAccessor> fieldAccessors) {
             final var resultBuilder = ImmutableIntArray.builder();

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/IndexEntryTranslatorEquivalenceTest.java

@@ -36,6 +36,7 @@
 import com.apple.foundationdb.tuple.Tuple;
 import com.apple.foundationdb.tuple.TupleHelpers;
 import com.google.common.collect.ImmutableList;
+import com.google.protobuf.DescriptorProtos;
 import com.google.protobuf.Descriptors;
 import com.google.protobuf.Message;
 import org.junit.jupiter.api.Test;
@@ -151,6 +152,72 @@
                 .findFirst().orElseThrow().getValue().toString());
     }
 
+    /**
+     * A field with an escaped identifier is spelled {@code foo.tableA.A1} by the planner and {@code foo__2tableA__2A1}
+     * by the record descriptor. The value based translator looks its columns up by the former, so it must be filled
+     * with that spelling too, or every column reads {@code NULL} (issue #4758).
+     */
+    @Test
+    void bothTranslatorsDecodeEscapedIdentifiersAlike() throws Descriptors.DescriptorValidationException {
+        final var metaDataBuilder = RecordMetaData.newBuilder().setRecords(escapedRecordsDescriptor());
+        metaDataBuilder.getRecordType("Escaped").setPrimaryKey(field("rec_no"));
+        metaDataBuilder.addIndex("Escaped", new Index("escaped",
+                concat(field("foo__2tableA__2A1"), field("foo__2tableA__2A2"))));
+        final var metaData = metaDataBuilder.build();
+        final var recordType = metaData.getRecordType("Escaped");
+        final var descriptor = recordType.getDescriptor();
+        final var baseType = Type.Record.fromDescriptor(descriptor);
+        final var baseAlias = Quantifier.current();
+        final var baseObjectValue = QuantifiedObjectValue.of(baseAlias, baseType);
+
+        final var indexKeyValues = ImmutableList.<Value>of(
+                FieldValue.ofFieldNames(baseObjectValue, List.of("foo.tableA.A1")),
+                FieldValue.ofFieldNames(baseObjectValue, List.of("foo.tableA.A2")),
+                FieldValue.ofFieldNames(baseObjectValue, List.of("rec_no")));
+
+        final var translators = ScanWithFetchMatchCandidate.computeIndexEntryToLogicalRecord(List.of(recordType),
+                baseAlias, baseType, indexKeyValues, ImmutableList.of()).orElseThrow();
+        final var recordValue = translators.indexEntryToRecordValue();
+        assertNotNull(recordValue, "escaped identifiers should still yield a value based translator");
+
+        final var entry = new IndexEntry(metaData.getIndex("escaped"), Tuple.from(1L, 2L, 7L), TupleHelpers.EMPTY);
+        final var byCopiers = translators.indexKeyValueToPartialRecord().toRecord(descriptor, entry);
+        final var byValue = evaluate(recordValue, entry);
+
+        assertEquals(1L, byCopiers.getField(descriptor.findFieldByName("foo__2tableA__2A1")));
+        assertEquals(2L, byCopiers.getField(descriptor.findFieldByName("foo__2tableA__2A2")));
+        assertSameRecord(descriptor, byCopiers, byValue);
+    }
+
+    /**
+     * {@code Escaped { rec_no, foo__2tableA__2A1, foo__2tableA__2A2 }}, all {@code int64}, and its union.
+     */
+    @Nonnull
+    private static Descriptors.FileDescriptor escapedRecordsDescriptor() throws Descriptors.DescriptorValidationException {
+        final var escaped = DescriptorProtos.DescriptorProto.newBuilder().setName("Escaped");
+        int number = 1;
+        for (final var name : List.of("rec_no", "foo__2tableA__2A1", "foo__2tableA__2A2")) {
+            escaped.addField(DescriptorProtos.FieldDescriptorProto.newBuilder()
+                    .setName(name)
+                    .setNumber(number++)
+                    .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL)
+                    .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_INT64));
+        }
+        final var union = DescriptorProtos.DescriptorProto.newBuilder().setName("RecordTypeUnion")
+                .addField(DescriptorProtos.FieldDescriptorProto.newBuilder()
+                        .setName("_Escaped")
+                        .setNumber(1)
+                        .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL)
+                        .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE)
+                        .setTypeName(".Escaped"));
+        final var fileProto = DescriptorProtos.FileDescriptorProto.newBuilder()
+                .setName("escaped_records.proto")
+                .addMessageType(escaped)
+                .addMessageType(union)
+                .build();
+        return Descriptors.FileDescriptor.buildFrom(fileProto, new Descriptors.FileDescriptor[] {});
+    }
+
     @Nonnull
     private static Message evaluate(@Nonnull final Value recordValue, @Nonnull final IndexEntry entry) {
         final var typeRepository = TypeRepository.newBuilder().addTypeIfNeeded(recordValue.getResultType()).build();

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/OrderingTest.java

@@ -877,7 +877,7 @@ private static RecordConstructorValue select(@Nonnull final String... projection
         final var rcv = ValueTestHelpers.qov();
         final List<Column<? extends Value>> columns = Arrays.stream(projection)
                 .map(field -> FieldValue.ofFieldName(rcv, field))
-                .map(field -> Column.of(Type.Record.Field.of(Type.primitiveType(Type.TypeCode.STRING), Optional.of(field.getLastFieldName().orElseThrow() + "p")), field))
+                .map(field -> Column.of(Type.Record.Field.of(Type.primitiveType(Type.TypeCode.STRING), Optional.of(field.getFieldPath().getLastFieldName().orElseThrow() + "p")), field))
                 .collect(Collectors.toList());
         // create a bunch of aliases
         return RecordConstructorValue.ofColumns(columns);

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/view/FieldValueMatcher.java

@@ -47,7 +47,7 @@ public FieldValueMatcher(@Nonnull String fieldName) {
     @Override
     protected boolean matchesSafely(final Value element) {
         return element instanceof FieldValue &&
-               ((FieldValue)element).getFieldPathNames().equals(fieldPath);
+               ((FieldValue)element).getFieldPathStorageNames().equals(fieldPath);
     }
 
     @Override