Fix bug of scan aggregate index returning empty non-end continuation #3397

pengpeng-lu · 2025-06-13T03:22:22Z

Fixes issue: #3206

Instead of returning the innerContinuation byte array, serializes into a protobuf, with an isEnd flag.

There will be 2 steps:

still serializes to the old format, being able to deserialize from both old and new format;
serializes to the new format.

Add an ExecuteProperty for testing purpose, can remove it after we completely switch to TO_NEW mode.

pengpeng-lu · 2025-06-17T03:49:52Z

yaml-tests/src/test/resources/aggregate-index-tests-count.yamsql

@@ -108,8 +106,6 @@ test_block:
    -
      - query: select count(col1) from t2
      - explain: "ISCAN(MV5 <,>) | MAP (_ AS _0) | AGG (count(_._0.COL1) AS _0) | ON EMPTY NULL | MAP (coalesce_long(_._0._0, promote(0l AS LONG)) AS _0)"
-      # Cannot run with FORCE_CONTINUATIONS due to: https://github.com/FoundationDB/fdb-record-layer/issues/3206
-      - maxRows: 0


These 2 were also mis-categorized to issue 3206.

pengpeng-lu · 2025-06-17T05:30:00Z

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

@@ -72,12 +72,13 @@ public class ExecuteProperties {
    // how record scan limit reached is handled -- false: return early with continuation, true: throw exception
    private final boolean failOnScanLimitReached;
    private final boolean isDryRun;
+    private final boolean kvCursorContSerializeToNew;


will add comments in the next commit.

Hm. I'm still not sure this really belongs in the ExecuteProperties. The point of the ExecuteProperties configuration is that it's supposed to contain stuff that a user might want to configure differently for other identical invocations of some operation. For example, various limits are in here, which could be applied to different plans. This is different, because for a given plan, it's actually very important that the user correctly serialize/deserialize KV cursor continuations. Which doesn't seem to fit the pattern

alecgrieser

Sorry for the delay in getting to reviewing this!

alecgrieser · 2025-07-03T17:10:35Z

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

@@ -72,12 +72,13 @@ public class ExecuteProperties {
    // how record scan limit reached is handled -- false: return early with continuation, true: throw exception
    private final boolean failOnScanLimitReached;
    private final boolean isDryRun;
+    private final boolean kvCursorContSerializeToNew;


Hm. I'm still not sure this really belongs in the ExecuteProperties. The point of the ExecuteProperties configuration is that it's supposed to contain stuff that a user might want to configure differently for other identical invocations of some operation. For example, various limits are in here, which could be applied to different plans. This is different, because for a given plan, it's actually very important that the user correctly serialize/deserialize KV cursor continuations. Which doesn't seem to fit the pattern

alecgrieser · 2025-07-03T17:15:13Z

...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java

@@ -156,21 +164,79 @@ public boolean isEnd() {
        @Nonnull
        @Override
        public ByteString toByteString() {
-            if (lastKey == null) {
-                return ByteString.EMPTY;
+            System.out.println("Keyvaluecursor toByteString serialization mode:" + serializationMode.name());


Suggested change

System.out.println("Keyvaluecursor toByteString serialization mode:" + serializationMode.name());

alecgrieser · 2025-07-03T17:16:22Z

...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java

+            if (serializationMode == SerializationMode.TO_OLD) {
+                if (lastKey == null) {
+                    return null;
+                }
+                return Arrays.copyOfRange(lastKey, prefixLength, lastKey.length);
+            } else {
+                byte[] result = toProto().toByteArray();
+                System.out.println("result:" + result);
+                return toProto().toByteArray();
+            }


I suppose there are some performance implications, but it would be nice if this were able to be based on the toByteString() implementation

alecgrieser · 2025-07-04T13:34:31Z

...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java

    }

-    private static class Continuation implements RecordCursorContinuation {
+    public static class Continuation implements RecordCursorContinuation {


Does this need to be public just so that the RecordQueryIndexPlan can update its continuation appropriately?

alecgrieser · 2025-07-04T14:41:05Z

fdb-record-layer-core/src/main/proto/record_cursor.proto

+
+message KeyValueCursorContinuation {
+    optional bytes continuation = 1;
+    optional bool isEnd = 2;


We really shouldn't be returning any "end" continuations that are of type KeyValueCursorContinuation, so this seems a little suspect. To support something #1076, it could be useful to have the prefix length, though that actually has some portability problems, though that's not necessarily required. For the purposes of just solving this problem, I think the issue was just that we would sometimes get the empty string if the prefix size was the same as the whole key size. If we can confirm that wrapping it in a continuation and setting the continuation field to the empty string results in a non-empty continuation, that might actually be enough. If we did go with just a wrapped bytes, we should probably note why we went with that in a comment lest someone be confused why it's not just returning the underlying bytes.

alecgrieser · 2025-07-08T13:57:23Z

...-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlan.java

+            if (continuation instanceof KeyValueCursorBase.Continuation) {
+                continuationBytes = ((KeyValueCursorBase.Continuation) continuation).getInnerContinuationInBytes();


Are there cases where the continuation is not of this type? Should assert on the continuation type here instead of attempting to work around it?

alecgrieser · 2025-07-08T14:00:10Z

...re/src/test/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorTest.java

@@ -44,6 +44,8 @@
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;


It would probably be good to add a test here where there is a scan over a single key range. The expectation would be that the continuation from the (only) key should be empty in the old serialization mode, but it should be non-empty in the new serialization mode

alecgrieser · 2025-07-08T14:01:43Z

...m/apple/foundationdb/record/provider/foundationdb/indexes/MultidimensionalIndexTestBase.java

@@ -496,7 +496,7 @@ void basicReadWithNullsTest(final boolean useAsync, @Nonnull final String storag
    }

    void indexReadTest(final boolean useAsync, final long seed, final int numRecords, @Nonnull final String storage,
-                   final boolean storeHilbertValues, final boolean useNodeSlotIndex) throws Exception {
+                           final boolean storeHilbertValues, final boolean useNodeSlotIndex) throws Exception {


minor:

Suggested change

final boolean storeHilbertValues, final boolean useNodeSlotIndex) throws Exception {

final boolean storeHilbertValues, final boolean useNodeSlotIndex) throws Exception {

I think. The main goal here would be to align the finals from the two lines

alecgrieser · 2025-07-08T14:05:18Z

...ava/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlanWithOverScanTest.java

-                assertEquals(indexResult.getContinuation().toByteString(), overscanResult.getContinuation().toByteString(), "Continuation byte strings should match");
-                assertArrayEquals(indexResult.getContinuation().toBytes(), overscanResult.getContinuation().toBytes(), "Continuation byte arrays should match");


Does this assertion actually need to change? I would have thought that we'd expect the continuations to stay the same, even if both are wrapped in protobuf

alecgrieser · 2025-07-08T14:06:31Z

...al-core/src/main/java/com/apple/foundationdb/relational/recordlayer/RecordLayerIterator.java

@@ -83,6 +83,7 @@ public boolean hasNext() {

    private void fetchNextResult() {
        if (result != null) {
+            continuation = ContinuationImpl.fromRecordCursorContinuation(result.getContinuation());


Hm, I'm not sure this is quite right. It seems like there is perhaps a deficiency in the previous code, but perhaps it should set the continuation on below near where it sets the continuation if the noNextReason is SOURCE_EXHAUSTED. That is, instead of just setting it to END if the no next reason is SOURCE_EXHAUSTED, it should also set it to the copied over continuation if the no-next-reason is not SOURCE_EXHAUSTED

pengpeng-lu added 15 commits May 21, 2025 10:57

save

387a648

save

0fd847e

save

3ce0259

save

78bea95

save

aefa82c

save

3fe7ba0

save

1336975

clean

44a5cb9

save

cb40b88

Merge branch 'main' into keyvalue_cursor

d82d31c

add planner configuration

8d523c1

save

d015132

save

22a6e85

revert PlannerConfiguration change:

f30c3e3

add test

cc9567b

pengpeng-lu added the bug fix label Jun 17, 2025

pengpeng-lu changed the title ~~Keyvalue cursor~~ Fix bug of scan aggregate index returning empty non-end continuation Jun 17, 2025

pengpeng-lu commented Jun 17, 2025

View reviewed changes

pengpeng-lu added 2 commits June 16, 2025 20:53

style

09907fe

checkstyle

8343661

pengpeng-lu marked this pull request as ready for review June 17, 2025 05:29

pengpeng-lu requested a review from alecgrieser June 17, 2025 05:29

pengpeng-lu commented Jun 17, 2025

View reviewed changes

alecgrieser requested changes Jul 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix bug of scan aggregate index returning empty non-end continuation #3397

Fix bug of scan aggregate index returning empty non-end continuation #3397

Uh oh!

pengpeng-lu commented Jun 13, 2025 •

edited

Loading

Uh oh!

pengpeng-lu Jun 17, 2025

Uh oh!

pengpeng-lu Jun 17, 2025

Uh oh!

alecgrieser Jul 3, 2025

Uh oh!

alecgrieser left a comment

Uh oh!

alecgrieser Jul 3, 2025

Uh oh!

alecgrieser Jul 3, 2025

Uh oh!

alecgrieser Jul 3, 2025

Uh oh!

alecgrieser Jul 4, 2025

Uh oh!

alecgrieser Jul 4, 2025

Uh oh!

alecgrieser Jul 8, 2025

Uh oh!

alecgrieser Jul 8, 2025

Uh oh!

alecgrieser Jul 8, 2025

Uh oh!

alecgrieser Jul 8, 2025

Uh oh!

alecgrieser Jul 8, 2025

Uh oh!

Uh oh!

		if (continuation instanceof KeyValueCursorBase.Continuation) {
		continuationBytes = ((KeyValueCursorBase.Continuation) continuation).getInnerContinuationInBytes();

	final boolean storeHilbertValues, final boolean useNodeSlotIndex) throws Exception {
	final boolean storeHilbertValues, final boolean useNodeSlotIndex) throws Exception {

		assertEquals(indexResult.getContinuation().toByteString(), overscanResult.getContinuation().toByteString(), "Continuation byte strings should match");
		assertArrayEquals(indexResult.getContinuation().toBytes(), overscanResult.getContinuation().toBytes(), "Continuation byte arrays should match");

Fix bug of scan aggregate index returning empty non-end continuation #3397

Are you sure you want to change the base?

Fix bug of scan aggregate index returning empty non-end continuation #3397

Uh oh!

Conversation

pengpeng-lu commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alecgrieser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pengpeng-lu commented Jun 13, 2025 •

edited

Loading