Fix bug of scan aggregate index returning empty non-end continuation #3397
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
pengpeng-lu changed the title 
pengpeng-lu commented Jun 17, 2025
| - 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 |
There was a problem hiding this comment.
These 2 were also mis-categorized to issue 3206.
pengpeng-lu commented Jun 17, 2025
fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/ExecuteProperties.java Outdated Show resolved Hide resolved
alecgrieser requested changes Jul 8, 2025
fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/ExecuteProperties.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Show resolved Hide resolved
...-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlan.java Outdated Show resolved Hide resolved
...re/src/test/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorTest.java Show resolved Hide resolved
...m/apple/foundationdb/record/provider/foundationdb/indexes/MultidimensionalIndexTestBase.java Outdated Show resolved Hide resolved
...ava/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlanWithOverScanTest.java Show resolved Hide resolved
...al-core/src/main/java/com/apple/foundationdb/relational/recordlayer/RecordLayerIterator.java Outdated Show resolved Hide resolved
alecgrieser requested changes Sep 9, 2025
...-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlan.java Show resolved Hide resolved
alecgrieser requested changes Sep 12, 2025
There was a problem hiding this comment.
We had a discussion offline, and I think we've converged on a strategy here.
As @pengpeng-lu points out, there are a few too many places where the key value cursor is created directly. We could theoretically limit the fix to just the two plan types, and have them manage wrapping and unwrapping the cursor, but then all users of the KeyValueCursor would have to worry about scans over single key ranges leading to an empty continuation on the first element.
For that reason, we're thinking that we have to go with an approach where the KeyValueCursor is the one that can accept either an old or new style continuation, and it just has to parse it to see if it fits. However, this is dangerous as some old continuations could be parsed as a new continuation. To avoid that, we want to insert a magic number into the new protobuf continuation, and then we will validate that the number exactly matches. Any continuation without the magic number must then be from an older instance even if it parses correctly.
If we take this approach, we can "just" put this into the KeyValueCursor, and then remove the changes to the plan serialization. Step one will put out a version that can read the new continuations. Then the next version, we make the new continuations by default. A future version can then remove the compatibility mode.
This is actually pretty close to the original role out plan, but we think that having the extra magic number check makes us less likely to misinterpret a continuation, which can lead to reading incorrect data.
alecgrieser added a commit to alecgrieser/fdb-record-layer that referenced this pull request Sep 15, 2025
This adds test cases to cover situations where we might have a byte array that could plausibly be either a `Tuple` or a `Protobuf` message. It enumerates through the `Tuple` types, and in each case, it either identifies why the `Tuple` code could not be a valid message, or it constructs a case that could. The upshot is that it's clear that parsing as Protobuf without an error is not enough to say that something was definitely a Protobuf, though the stars do kind of need to align for this to happen. Part of my hope here is that we can use this reference whenever this kind of situation comes up. For example, in our current thinking for FoundationDB#3397, we'll want to employ a strategy of guessing the origin of a byte string. If we want to test what happens if we start with a `Tuple` and then try to parse it as a Protobuf, this could be used to inform such a test case.
alecgrieser added a commit to alecgrieser/fdb-record-layer that referenced this pull request Sep 15, 2025
This adds test cases to cover situations where we might have a byte array that could plausibly be either a `Tuple` or a `Protobuf` message. It enumerates through the `Tuple` types, and in each case, it either identifies why the `Tuple` code could not be a valid message, or it constructs a case that could. The upshot is that it's clear that parsing as Protobuf without an error is not enough to say that something was definitely a Protobuf, though the stars do kind of need to align for this to happen. Part of my hope here is that we can use this reference whenever this kind of situation comes up. For example, in our current thinking for FoundationDB#3397, we'll want to employ a strategy of guessing the origin of a byte string. If we want to test what happens if we start with a `Tuple` and then try to parse it as a Protobuf, this could be used to inform such a test case.
alecgrieser added a commit to alecgrieser/fdb-record-layer that referenced this pull request Sep 15, 2025
This adds test cases to cover situations where we might have a byte array that could plausibly be either a `Tuple` or a `Protobuf` message. It enumerates through the `Tuple` types, and in each case, it either identifies why the `Tuple` code could not be a valid message, or it constructs a case that could. The upshot is that it's clear that parsing as Protobuf without an error is not enough to say that something was definitely a Protobuf, though the stars do kind of need to align for this to happen. Part of my hope here is that we can use this reference whenever this kind of situation comes up. For example, in our current thinking for FoundationDB#3397, we'll want to employ a strategy of guessing the origin of a byte string. If we want to test what happens if we start with a `Tuple` and then try to parse it as a Protobuf, this could be used to inform such a test case.
alecgrieser requested changes Sep 16, 2025
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
alecgrieser requested changes Sep 17, 2025
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlan.java Outdated Show resolved Hide resolved
...-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlan.java Outdated Show resolved Hide resolved
...r-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryScanPlan.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
yaml-tests/src/test/resources/disabled-planner-rewrites/aggregate-index-tests.yamsql Show resolved Hide resolved
...re/src/test/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorTest.java Show resolved Hide resolved
...re/src/test/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorTest.java Show resolved Hide resolved
alecgrieser requested changes Sep 18, 2025
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java Outdated Show resolved Hide resolved
...-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryIndexPlan.java Outdated Show resolved Hide resolved
alecgrieser requested changes Sep 19, 2025
There was a problem hiding this comment.
Okay, LGTM. I left one comment about how I still think it would be good to remove PrefixRemovingContinuation and just use KeyValueCursorBase.Continuation, but I'm also fine with how it is in the PR now if we want to hold off on that. The only remaining thing is that as far as I can tell, there's still one test case that was removed from disabled-planner-rewrites/aggregate-index-tests.yamsql, and it seems like we should add it back
yaml-tests/src/test/resources/disabled-planner-rewrites/aggregate-index-tests.yamsql Show resolved Hide resolved
alecgrieser approved these changes Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Fixes issue: #3206
Instead of returning the innerContinuation byte array, serializes into a protobuf, with an
isEndflag.There will be 2 steps:
This fixes #3206