Fix internal errors from version queries: Part 1 #3809

alecgrieser · 2025-12-17T19:58:37Z

This begins the work necessary to fix internal errors related to VersionValue queries. The ultimate goal is to remove VersionValue, but for now, what this does is it modifies the way the various operators that sit on top of record fetches and has them copy out the version into a field, called __ROW_VERSION. Once that's done, then in a future version (say, 4.9), we'll be able to modify the PlanGenerator so that it references that field. Note that we'll need to wait for that rather than do it immediately for backwards compatibility reasons. If we took the final version right away, then any plan that wants the version when sent to an older version that doesn't do this copying will be unable to process that FieldValue.

This is in furtherance of #3796.

This also fixes #3734. This is because the type repository updates that are done to make the new copied-to types present in the type repository also put in the enum fields that were previously missing (and causing #3734).

This begins the work necessary to fix internal errors related to `VersionValue` queries. The ultimate goal is to remove `VersionValue`, but for now, what this does is it modifies the way the various operators that sit on top of record fetches and has them copy out the version into a field, called `__ROW_VERSION`. Once that's done, then in a future version (say, 4.9), we'll be able to modify the `PlanGenerator` so that it references that field. Note that we'll need to wait for that rather than do it immediately for backwards compatibility reasons. If we took the final version right away, then any plan that wants the version when sent to an older version that doesn't do this copying will be unable to process that `FieldValue`. This is in furtherance of FoundationDB#3796.

...rc/main/java/com/apple/foundationdb/record/metadata/expressions/RecordTypeKeyExpression.java

.../java/com/apple/foundationdb/record/query/plan/cascades/WindowedIndexScanMatchCandidate.java

yaml-tests/src/test/resources/enum.yamsql

yaml-tests/src/test/resources/field-index-tests-proto.yamsql

...-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/LogicalOperator.java

normen662 · 2025-12-18T11:16:50Z

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

- return ImmutableSet.of(p.getResultType());
+ final Type resultType = p.getResultType();
+ if (!resultType.isPrimitive() && !resultType.isUuid()) {
+ return ImmutableSet.of(resultType);


Did this really return a type even if the type is not a record? I guess something using the set that is returned here would then filter it.

I'm not sure I follow. This returns the type only if it's a record or an enum. Primitive types (and UUIDs) get filtered out. This is necessary so that the type repository has the type when it goes to copy it. It also fixed #3734

This method only really needs to return types that are Type.Record. Before your change, it seems it added primitive types as well which made me think about what happens here.

Ah. I believe that before, because it was basing its choice on whether the type was a CreatesDynamicTypesValue, which was mostly RecordConstructorValue and AbstractArrayConstructorValue. So, it would have returned a primitive if that was somehow the return type of one of those (like, say, PromoteValue). That no longer works as more things require dynamic types. No one is actually using that marker interface any more, so it could be removed, I suppose.

...ain/java/com/apple/foundationdb/record/query/plan/cascades/AggregateIndexMatchCandidate.java

...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/IndexExpansionInfo.java

.../src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryLoadByKeysPlan.java

normen662 · 2025-12-18T11:34:16Z

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


 @Nonnull
- private static Map<String, Descriptors.FieldDescriptor> getFieldDescriptorMap(@Nonnull final Stream<RecordType> recordTypeStream) {
+ public Type.Record getPlannerType(@Nonnull Collection<String> recordTypeNames) {


Shouldn't we cache this?

It would be nice (or at least, it would be nice to cache the single-type variant returned by getPlannerType(String)). My initial version of this constructed these structures during RecordMetaData.build and then just retrieves it from a Map. However, the descriptor to Type.Record logic cannot handle cyclic type relationships (e.g., a type X with a field of type Y which has a field of type X), and there are (it turns out) a bunch of tests in our code base with that kind of structure.

We could make this a memoized thing, and then we'd avoid re-creating the same object multiple times. We weren't doing that before, though, in the old code path, so this is at least treading water.

However, the descriptor to Type.Record logic cannot handle cyclic type relationships (e.g., a type X with a field of type Y which has a field of type X), and there are (it turns out) a bunch of tests in our code base with that kind of structure.

How is that possible? Type is immutable so how can you create those cyclic types. Can you point to a test case that does this?

In some sense, it's the immutability of Type that is the problem. We have test cases like UnnestedRecordTypeTest.unnestDoubleNestedMapType that use test_records_double_nested.proto as the basis of their meta-data definition: https://github.com/FoundationDB/fdb-record-layer/blob/main/fdb-record-layer-core/src/test/proto/test_records_double_nested.proto

Note that the types in that file (like, say MiddleRecord, which has a field of type MiddleRecord) have a cyclic graph. I don't think you can construct a Type.Record that models this correctly, and if you try to, you get a StackOverflowError. FWIW, I think you would always have gotten a stack overflow error if you tried to use cascades to query this type, and trying to do that eagerly just overturned the bug

…CV construction was taking place

…ecord instead of a base Type

…ugh Cascades plans are not yet good

…t coverage of those plan execute methods

1. The normalize fields method was resetting any field numbers if they were already set. This can cause `deepCopy` problems as the message structures may not align if the original types had out of order field numbers 1. Update could get weird. It expects the old and new messages to have the same format, which means that either they both needed to reference pseudo-fields or they both needed to not. However, the way it was structured, we could wind up placing two types with the same name (one with and one without the pseudo-fields) into the type repository. This adjusts this to not re-use the type, but we may have to think about what that means for the result set meta-data

…sh in UpdateExpressions

github-actions · 2025-12-19T20:00:33Z

📊 Metrics Diff Analysis Report

Summary

New queries: 52
Dropped queries: 13
Plan changed + metrics changed: 21
Plan unchanged + metrics changed: 16

ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

New queries: Queries added in this PR
Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
Plan changed + metrics changed: The query plan has changed along with planner metrics.
Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

New Queries

Count of new queries by file:

yaml-tests/src/test/resources/pseudo-field-clash.metrics.yaml: 19
yaml-tests/src/test/resources/versions-tests.metrics.yaml: 33

Dropped Queries

The following queries with metrics were removed:

yaml-tests/src/test/resources/versions-tests.metrics.yaml:2: EXPLAIN select "__ROW_VERSION" as version, t1.col2 from t1 where col1 = 10;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:14: EXPLAIN select t1.* from t1 where col1 = 10;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:24: EXPLAIN select s.version, s.col2 from (select "__ROW_VERSION" as version, t1.col2 as col2 from t1 where col1 = 10) AS s;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:36: EXPLAIN select s."__ROW_VERSION", s.col2 from (select "__ROW_VERSION", t1.col2 from t1 where col1 = 10) AS s;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:48: EXPLAIN select "__ROW_VERSION" as version, t1.* from t1 where col1 = 20;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:59: EXPLAIN select "__ROW_VERSION" as version, (t1.*) from t1 where col1 = 20;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:71: EXPLAIN select "__ROW_VERSION", t1.* from t1 where col1 = 20;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:82: EXPLAIN select "__ROW_VERSION", (t1.*) from t1 where col1 = 20;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:93: EXPLAIN select "__ROW_VERSION", t1.id from t1 order by "__ROW_VERSION" ASC;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:105: EXPLAIN select t1."__ROW_VERSION", t1.id from t1 order by "__ROW_VERSION" DESC;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:117: EXPLAIN select t1."__ROW_VERSION", t1.id from t1 where col1 = 20 order by "__ROW_VERSION" ASC;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:129: EXPLAIN select "__ROW_VERSION", t1.id from t1 where col1 = 20 order by "__ROW_VERSION" DESC;
yaml-tests/src/test/resources/versions-tests.metrics.yaml:141: EXPLAIN select "__ROW_VERSION", col1, t1.id from t1 where col1 > 10 order by col1 asc, "__ROW_VERSION" asc;

The reviewer should double check that these queries were removed intentionally to avoid a loss of coverage.

Plan and Metrics Changed

These queries experienced both plan and metrics changes. This generally indicates that there was some planner change
that means the planning for this query may be substantially different. Some amount of query plan metrics change is expected,
but the reviewer should still validate that these changes are not excessive.

Total: 21 queries

Statistical Summary (Plan and Metrics Changed)

task_count:

Average change: -60.1
Median change: -31
Standard deviation: 41.0
Range: -112 to -6
Queries changed: 17
No regressions! 🎉

transform_count:

Average change: -9.2
Average regression: +2.0
Median change: -5
Median regression: +2
Standard deviation: 7.3
Standard deviation of regressions: 0.0
Range: -21 to +2
Range of regressions: +2 to +2
Queries changed: 17
Queries regressed: 1

transform_yield_count:

Average change: -4.4
Median change: -3
Standard deviation: 2.4
Range: -8 to -2
Queries changed: 14
No regressions! 🎉

insert_new_count:

Average change: -5.3
Median change: -3
Standard deviation: 4.7
Range: -14 to -1
Queries changed: 21
No regressions! 🎉

insert_reused_count:

Average change: +2.4
Average regression: +2.4
Median change: +2
Median regression: +2
Standard deviation: 1.6
Standard deviation of regressions: 1.6
Range: +1 to +5
Range of regressions: +1 to +5
Queries changed: 14
Queries regressed: 14

There were no queries with significant regressions detected.

Minor Changes (Plan and Metrics Changed)

In addition, there were 21 queries with minor changes.

Only Metrics Changed

These queries experienced only metrics changes without any plan changes. If these metrics have substantially changed,
then a planner change has been made which affects planner performance but does not correlate with any new outcomes,
which could indicate a regression.

Total: 16 queries

Statistical Summary (Only Metrics Changed)

task_count:

Average change: -71.2
Median change: -53
Standard deviation: 50.4
Range: -234 to -32
Queries changed: 16
No regressions! 🎉

transform_count:

Average change: -11.0
Median change: -9
Standard deviation: 6.4
Range: -30 to -6
Queries changed: 16
No regressions! 🎉

transform_yield_count:

Average change: -3.8
Median change: -3
Standard deviation: 2.7
Range: -12 to -1
Queries changed: 16
No regressions! 🎉

insert_new_count:

Average change: -7.2
Median change: -5
Standard deviation: 5.0
Range: -23 to -3
Queries changed: 16
No regressions! 🎉

insert_reused_count:

Average change: +2.3
Average regression: +2.3
Median change: +2
Median regression: +2
Standard deviation: 1.0
Standard deviation of regressions: 1.0
Range: +1 to +5
Range of regressions: +1 to +5
Queries changed: 16
Queries regressed: 16

Significant Regressions (Only Metrics Changed)

There was 1 outlier detected. Outlier queries have a significant regression in at least one field. Statistically, this represents either an increase of more than two standard deviations above the mean or a large absolute increase (e.g., 100).

yaml-tests/src/test/resources/valid-identifiers.metrics.yaml:720: EXPLAIN SELECT * FROM "___T6.__UNESCAPED"
- explain: ISCAN(T6$ENUM2 <,>)
- task_count: 405 -> 321 (-84)
- transform_count: 98 -> 81 (-17)
- transform_yield_count: 43 -> 37 (-6)
- insert_new_count: 42 -> 31 (-11)
- insert_reused_count: 5 -> 10 (+5)

Minor Changes (Only Metrics Changed)

In addition, there were 15 queries with minor changes.

alecgrieser · 2025-12-19T21:53:08Z

Adding DO NOT MERGE. We have some things regarding the way type names are handled that we may want to work out. Once that's agreed upon, we'll want to take this, possibly with some adjustments for the type naming

alecgrieser requested a review from normen662 December 17, 2025 19:58

alecgrieser added the bug fix Change that fixes a bug label Dec 17, 2025

alecgrieser added 2 commits December 17, 2025 20:05

address one teamscale issue

823bd08

address queries with record type key value differences

fffa033

normen662 requested changes Dec 18, 2025

View reviewed changes

alecgrieser added 6 commits December 18, 2025 14:18

add back optimization to avoid extra map if not needed

04bab1f

address record type key incompatibilities by patching deserialization

24fc32f

address comments from @normen662 regarding where the star expansion R…

73ddc24

…CV construction was taking place

match candidates now explicitly remember that they have a base Type.R…

8c309a4

…ecord instead of a base Type

add clarifying comment to code

d6ceef3

remove unnessary supported_version

1c64c90

normen662 approved these changes Dec 18, 2025

View reviewed changes

alecgrieser added 7 commits December 18, 2025 19:45

handle duplicate column names when expanding expressions

b8ed269

revert "fix" to struct data meta-data test, as it was not a real fix

4d816a1

add coverage for RecordTypeKeyComparison.expand (it is broken)

08f586a

update plan asserts to newer API ; mark tests as DualPlanner even tho…

dbb5b40

…ugh Cascades plans are not yet good

update two SortCursorTests to use the equivalent plans in order to ge…

0349fc4

…t coverage of those plan execute methods

actually maintain separate cursor and plan tests

76428bc

alecgrieser force-pushed the 03796-be-explicit-about-versions-prelim branch from 9824821 to 50f02fc Compare December 19, 2025 17:31

alecgrieser added 2 commits December 19, 2025 18:28

explicitly remove type name reference in place that was causing a cla…

d02fa9f

…sh in UpdateExpressions

Fix versions-tests.yamsql plan after modifications to update expression

09bdcd3

alecgrieser added the DO NOT MERGE do not merge label Dec 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix internal errors from version queries: Part 1 #3809

Fix internal errors from version queries: Part 1 #3809

Uh oh!

alecgrieser commented Dec 17, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normen662 Dec 18, 2025

alecgrieser Dec 18, 2025

normen662 Dec 18, 2025

alecgrieser Dec 18, 2025

Uh oh!

Uh oh!

Uh oh!

normen662 Dec 18, 2025

alecgrieser Dec 18, 2025

normen662 Dec 18, 2025

alecgrieser Dec 18, 2025

github-actions bot commented Dec 19, 2025

alecgrieser commented Dec 19, 2025

Labels

2 participants

Fix internal errors from version queries: Part 1 #3809

Are you sure you want to change the base?

Fix internal errors from version queries: Part 1 #3809

Uh oh!

Conversation

alecgrieser commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2025

📊 Metrics Diff Analysis Report

Summary

New Queries

Dropped Queries

Plan and Metrics Changed

Statistical Summary (Plan and Metrics Changed)

Minor Changes (Plan and Metrics Changed)

Only Metrics Changed

Statistical Summary (Only Metrics Changed)

Significant Regressions (Only Metrics Changed)

Minor Changes (Only Metrics Changed)

alecgrieser commented Dec 19, 2025

Labels

2 participants

alecgrieser commented Dec 17, 2025 •

edited

Loading