Fix deserialization of continuations for queries that contain LIKE (#3207)

ScottDugas · web-flow · commit 1814f9843ba4 · 2025-03-06T09:57:43.000-05:00
Previously, it would parse the query pattern as the escape character, making continuations useless. i.e. `LIKE 'XYZ' ESCAPE 'XYZ'`, regardless of what the original query had for the escape character, if it had one. Because I was curious what happens if the query pattern was a single character, I added some additional tests, and ran them with the old code to see if it would break in different ways. It didn't, I think in part because: `LIKE 'Z' ESCAPE 'Z'` is the same as `LIKE 'Z'`. I don't know if that is correct behavior, but it is what we do. Even though these tests didn't expose other issues, I felt like they were worth keeping. This Resolves: #3099
diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/PatternForLikeValue.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/PatternForLikeValue.java
@@ -188,7 +188,7 @@ public PValue toValueProto(@Nonnull final PlanSerializationContext serialization
     public static PatternForLikeValue fromProto(@Nonnull final PlanSerializationContext serializationContext,
                                                 @Nonnull final PPatternForLikeValue patternForLikeValueProto) {
         return new PatternForLikeValue(Value.fromValueProto(serializationContext, Objects.requireNonNull(patternForLikeValueProto.getPatternChild())),
-                Value.fromValueProto(serializationContext, Objects.requireNonNull(patternForLikeValueProto.getPatternChild())));
+                Value.fromValueProto(serializationContext, Objects.requireNonNull(patternForLikeValueProto.getEscapeChild())));
     }
 
     @Nonnull
diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/LikeOperatorValueTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/LikeOperatorValueTest.java
@@ -22,7 +22,10 @@
 
 import com.apple.foundationdb.record.Bindings;
 import com.apple.foundationdb.record.EvaluationContext;
+import com.apple.foundationdb.record.PlanHashable;
+import com.apple.foundationdb.record.PlanSerializationContext;
 import com.apple.foundationdb.record.TestRecords7Proto;
+import com.apple.foundationdb.record.planprotos.PLikeOperatorValue;
 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.typing.Typed;
@@ -33,6 +36,7 @@
 import com.apple.foundationdb.record.query.plan.cascades.values.LikeOperatorValue;
 import com.apple.foundationdb.record.query.plan.cascades.values.Value;
 import com.apple.foundationdb.record.query.plan.plans.QueryResult;
+import com.apple.foundationdb.record.query.plan.serialization.DefaultPlanSerializationRegistry;
 import com.google.common.collect.ImmutableList;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.extension.ExtensionContext;
@@ -41,6 +45,7 @@
 import org.junit.jupiter.params.provider.ArgumentsProvider;
 import org.junit.jupiter.params.provider.ArgumentsSource;
 
+import javax.annotation.Nonnull;
 import java.util.Arrays;
 import java.util.Optional;
 import java.util.stream.Stream;
@@ -188,18 +193,38 @@ void testSemanticException(Value lhs, Value rhs, Value escapeChar) {
     }
 
     @ParameterizedTest
-    @SuppressWarnings({"rawtypes", "unchecked", "ConstantConditions"})
+    @SuppressWarnings({"ConstantConditions"})
     @ArgumentsSource(ValidInputArgumentsProvider.class)
     void testLike(String lhs, String rhs, final String escapeChar, Boolean result) {
+        final LikeOperatorValue value = createLikeOperatorValue(lhs, rhs, escapeChar);
+        Assertions.assertEquals(result, value.eval(null, evaluationContext));
+    }
+
+    @ParameterizedTest
+    @SuppressWarnings({"ConstantConditions"})
+    @ArgumentsSource(ValidInputArgumentsProvider.class)
+    void testLikeSerialization(String lhs, String rhs, final String escapeChar, Boolean result) {
+        final LikeOperatorValue value = createLikeOperatorValue(lhs, rhs, escapeChar);
+        final PLikeOperatorValue proto = value.toProto(
+                new PlanSerializationContext(new DefaultPlanSerializationRegistry(),
+                        PlanHashable.CURRENT_FOR_CONTINUATION));
+        final LikeOperatorValue deserialized = LikeOperatorValue.fromProto(new PlanSerializationContext(new DefaultPlanSerializationRegistry(),
+                PlanHashable.CURRENT_FOR_CONTINUATION), proto);
+        Assertions.assertEquals(result, deserialized.eval(null, evaluationContext));
+    }
+
+
+    @SuppressWarnings({"rawtypes", "unchecked", "ConstantConditions"})
+    @Nonnull
+    private static LikeOperatorValue createLikeOperatorValue(final String lhs, final String rhs, final String escapeChar) {
         BuiltInFunction like = new LikeOperatorValue.LikeFn();
         BuiltInFunction pattern = new PatternForLikeValue.PatternForLikeFn();
         Typed value = like.encapsulate(Arrays.asList(
-                    new LiteralValue<>(Type.primitiveType(Type.TypeCode.STRING), lhs),
-                    pattern.encapsulate(Arrays.asList(
-                            new LiteralValue<>(Type.primitiveType(Type.TypeCode.STRING), rhs),
-                            new LiteralValue<>(Type.primitiveType(Type.TypeCode.STRING), escapeChar)))));
-        Assertions.assertTrue(value instanceof LikeOperatorValue);
-        Object actualValue = ((LikeOperatorValue)value).eval(null, evaluationContext);
-        Assertions.assertEquals(result, actualValue);
+                new LiteralValue<>(Type.primitiveType(Type.TypeCode.STRING), lhs),
+                pattern.encapsulate(Arrays.asList(
+                        new LiteralValue<>(Type.primitiveType(Type.TypeCode.STRING), rhs),
+                        new LiteralValue<>(Type.primitiveType(Type.TypeCode.STRING), escapeChar)))));
+        Assertions.assertInstanceOf(LikeOperatorValue.class, value);
+        return (LikeOperatorValue) value;
     }
 }
diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/CustomYamlConstructor.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/CustomYamlConstructor.java
@@ -132,6 +132,11 @@ public static LinedObject cast(@Nonnull Object obj, @Nonnull Supplier<String> ms
             Assert.thatUnchecked(obj instanceof LinedObject, ErrorCode.INTERNAL_ERROR, msg);
             return (LinedObject) obj;
         }
+
+        @Override
+        public String toString() {
+            return object + "@line:" + lineNumber;
+        }
     }
 
     private static class ConstructIgnore extends AbstractConstruct {
diff --git a/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/block/Block.java b/yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/block/Block.java
@@ -53,7 +53,8 @@ public interface Block {
      */
     static Block parse(@Nonnull Object document, int blockNumber, @Nonnull YamlExecutionContext executionContext) {
         final var blockObject = Matchers.map(document, "block");
-        Assert.thatUnchecked(blockObject.size() == 1, "Illegal Format: A block is expected to be a map of size 1");
+        Assert.thatUnchecked(blockObject.size() == 1,
+                "Illegal Format: A block is expected to be a map of size 1 (block: " + blockNumber + ") keys: " + blockObject.keySet());
         final var entry = Matchers.firstEntry(blockObject, "block key-value");
         final var linedObject = CustomYamlConstructor.LinedObject.cast(entry.getKey(), () -> "Invalid block key-value pair: " + entry);
         final var lineNumber = linedObject.getLineNumber();
diff --git a/yaml-tests/src/test/java/YamlIntegrationTests.java b/yaml-tests/src/test/java/YamlIntegrationTests.java
@@ -211,8 +211,6 @@ public void updateDeleteReturning(YamlTest.Runner runner) throws Exception {
     }
 
     @TestTemplate
-    @ExcludeYamlTestConfig(value = YamlTestConfigFilters.DO_NOT_FORCE_CONTINUATIONS,
-            reason = "Like continuation failure (https://github.com/FoundationDB/fdb-record-layer/issues/3099)")
     void like(YamlTest.Runner runner) throws Exception {
         runner.runYamsql("like.yamsql");
     }
diff --git a/yaml-tests/src/test/resources/like.yamsql b/yaml-tests/src/test/resources/like.yamsql
@@ -17,9 +17,16 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+---
+options:
+  # Prior to #3099 being fixed, every query that returned any data failed if there are continuations
+  # Without testing any continuations there's not a lot of value here to running with multi-server,
+  # So this is just being disabled entirely for older versions.
+  supported_version: !current_version # https://github.com/FoundationDB/fdb-record-layer/issues/3099
 ---
 schema_template:
     create table A(a1 string, primary key(a1))
+    create table B(b1 integer, b2 string, primary key(b1))
 ---
 setup:
   steps:
@@ -39,11 +46,49 @@ setup:
         ('{abcdefghijk}'),
         ('^$'),
         ('\\||%');
+    - query: insert into B values
+        (1, 'Y'),
+        (2, 'Z'),
+        (3, 'A'),
+        (4, 'Z'),
+        (5, 'B');
 ---
-# TODO (Investigate `Missing binding for __const_CONSTANT` error with queries when using plan from cache)
 test_block:
+  # TODO (Investigate `Missing binding for __const_CONSTANT` error with queries when using plan from cache)
   preset: single_repetition_parallelized
   tests:
+    -
+      - query: select * from B WHERE B2 LIKE 'X'
+      - result: []
+    -
+      - query: select * from B WHERE B2 LIKE 'Y'
+      - result: [{1, 'Y'}]
+    -
+      # Issue #3099 found that the pattern was being put in the escape part of the LIKE on continuation
+      # which for all the other queries, would result in an error that the escape should be of length 1.
+      # I added these queries to see if that issue would expose differently if the query was only one character,
+      # and it kind of did, it ended up exposing issue: https://github.com/FoundationDB/fdb-record-layer/issues/3216
+      - query: select * from B WHERE B2 NOT LIKE 'Z'
+      - result: [{1, 'Y'}, {3, 'A'}, {5, 'B'}]
+    -
+      - query: select * from B WHERE B2 NOT LIKE 'Z' ESCAPE 'Z'
+      # This should error; see https://github.com/FoundationDB/fdb-record-layer/issues/3216
+      - result: [{1, 'Y'}, {3, 'A'}, {5, 'B'}]
+    -
+      - query: select * from B WHERE B2 NOT LIKE '\'
+      # This should error; see https://github.com/FoundationDB/fdb-record-layer/issues/3216
+      - result: [{1, 'Y'}, {2, 'Z'}, {3, 'A'}, {4, Z}, {5, 'B'}]
+    -
+      - query: select * from B WHERE B2 LIKE 'Z'
+      - result: [{2, 'Z'}, {4, 'Z'}]
+---
+test_block:
+  # TODO (Investigate `Missing binding for __const_CONSTANT` error with queries when using plan from cache)
+  preset: single_repetition_parallelized
+  tests:
+    -
+      - query: select * from A WHERE A1 LIKE 'A'
+      - result: []
     -
       - query: select * from A WHERE A1 LIKE 'abc'
       - result: []
@@ -152,5 +197,6 @@ test_block:
           {'学校'},
           {'مدرسة'},
           {'^$'},
-          {'\\||%'} ]
+          {'\\||%'}
+          ]
 ...

Original file line number	Diff line number	Diff line change
`@@ -188,7 +188,7 @@ public PValue toValueProto(@Nonnull final PlanSerializationContext serialization`
`188`	`188`	`public static PatternForLikeValue fromProto(@Nonnull final PlanSerializationContext serializationContext,`
`189`	`189`	`@Nonnull final PPatternForLikeValue patternForLikeValueProto) {`
`190`	`190`	`return new PatternForLikeValue(Value.fromValueProto(serializationContext, Objects.requireNonNull(patternForLikeValueProto.getPatternChild())),`
`191`		`- Value.fromValueProto(serializationContext, Objects.requireNonNull(patternForLikeValueProto.getPatternChild())));`
	`191`	`+ Value.fromValueProto(serializationContext, Objects.requireNonNull(patternForLikeValueProto.getEscapeChild())));`
`192`	`192`	`}`
`193`	`193`
`194`	`194`	`@Nonnull`
Original file line number	Diff line number	Diff line change
`@@ -132,6 +132,11 @@ public static LinedObject cast(@Nonnull Object obj, @Nonnull Supplier<String> ms`
`132`	`132`	`Assert.thatUnchecked(obj instanceof LinedObject, ErrorCode.INTERNAL_ERROR, msg);`
`133`	`133`	`return (LinedObject) obj;`
`134`	`134`	`}`
	`135`	`+`
	`136`	`+ @Override`
	`137`	`+ public String toString() {`
	`138`	`+ return object + "@line:" + lineNumber;`
	`139`	`+ }`
`135`	`140`	`}`
`136`	`141`
`137`	`142`	`private static class ConstructIgnore extends AbstractConstruct {`
Original file line number	Diff line number	Diff line change
`@@ -211,8 +211,6 @@ public void updateDeleteReturning(YamlTest.Runner runner) throws Exception {`
`211`	`211`	`}`
`212`	`212`
`213`	`213`	`@TestTemplate`
`214`		`- @ExcludeYamlTestConfig(value = YamlTestConfigFilters.DO_NOT_FORCE_CONTINUATIONS,`
`215`		`- reason = "Like continuation failure (https://github.com/FoundationDB/fdb-record-layer/issues/3099)")`
`216`	`214`	`void like(YamlTest.Runner runner) throws Exception {`
`217`	`215`	`runner.runYamsql("like.yamsql");`
`218`	`216`	`}`