From 9a43f248035bf4df9ce06f33866deabe934281ae Mon Sep 17 00:00:00 2001 From: Robert Brunel Date: Wed, 6 May 2026 19:55:08 +0100 Subject: [PATCH 1/3] Add static imports for `FanType` in `KeyExpressionTest` --- .../record/metadata/KeyExpressionTest.java | 141 +++++++++--------- 1 file changed, 72 insertions(+), 69 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java index 1fe4c2dd5f..1dd3b67da3 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java @@ -73,6 +73,9 @@ import static com.apple.foundationdb.record.metadata.Key.Expressions.recordType; import static com.apple.foundationdb.record.metadata.Key.Expressions.value; import static com.apple.foundationdb.record.metadata.expressions.EmptyKeyExpression.EMPTY; +import static com.apple.foundationdb.record.metadata.expressions.KeyExpression.FanType.Concatenate; +import static com.apple.foundationdb.record.metadata.expressions.KeyExpression.FanType.FanOut; +import static com.apple.foundationdb.record.metadata.expressions.KeyExpression.FanType.None; import static com.apple.foundationdb.record.metadata.expressions.VersionKeyExpression.VERSION; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.instanceOf; @@ -304,7 +307,7 @@ void testFunctionEvalToWrongType() { @Test void testSubstrFunctionStaticFanout() { - final KeyExpression expression = function("substr", concat(field("repeat_me", FanType.FanOut), value(0), value(3))); + final KeyExpression expression = function("substr", concat(field("repeat_me", FanOut), value(0), value(3))); expression.validate(TestScalarFieldAccess.getDescriptor()); List results = evaluate(expression, plantsBoxesAndBowls); assertEquals(2, results.size(), "Wrong number of results"); @@ -314,7 +317,7 @@ void testSubstrFunctionStaticFanout() { @Test void testSubstrFunctionDynamicFanout() { final KeyExpression expression = function("substr", - field("substrings", FanType.FanOut).nest( + field("substrings", FanOut).nest( concatenateFields("content", "start", "end"))); expression.validate(SubStrings.getDescriptor()); List results = evaluate(expression, subString); @@ -329,7 +332,7 @@ void testSubstrFunctionDynamicFanout() { @Test void testConcatenateSingleRepeatedField() { - final KeyExpression expression = field("repeat_me", FanType.Concatenate); + final KeyExpression expression = field("repeat_me", Concatenate); expression.validate(TestScalarFieldAccess.getDescriptor()); assertFalse(expression.createsDuplicates()); assertEquals(Collections.singletonList(scalar(Arrays.asList("Boxes", "Bowls"))), @@ -343,7 +346,7 @@ void testConcatenateSingleRepeatedField() { @Test void testFieldThenConcatenateRepeated() { final KeyExpression expression = Key.Expressions.concat(field("field"), - field("repeat_me", FanType.Concatenate)); + field("repeat_me", Concatenate)); expression.validate(TestScalarFieldAccess.getDescriptor()); assertFalse(expression.createsDuplicates()); assertEquals(Collections.singletonList(Key.Evaluated.concatenate("Plants", Arrays.asList("Boxes", "Bowls"))), @@ -356,7 +359,7 @@ void testFieldThenConcatenateRepeated() { @Test void testFanSingleRepeatedField() { - final KeyExpression expression = field("repeat_me", FanType.FanOut); + final KeyExpression expression = field("repeat_me", FanOut); expression.validate(TestScalarFieldAccess.getDescriptor()); assertTrue(expression.createsDuplicates()); assertEquals(Arrays.asList(scalar("Boxes"), scalar("Bowls")), @@ -370,14 +373,14 @@ void testFanSingleRepeatedField() { @Test void testValidateFanRequiresRepeated() { assertThrows(KeyExpression.InvalidExpressionException.class, () -> { - field("field", FanType.FanOut).validate(TestScalarFieldAccess.getDescriptor()); + field("field", FanOut).validate(TestScalarFieldAccess.getDescriptor()); }); } @Test void testValidateConcatenateRequiresRepeated() { assertThrows(KeyExpression.InvalidExpressionException.class, () -> { - field("field", FanType.Concatenate).validate(TestScalarFieldAccess.getDescriptor()); + field("field", Concatenate).validate(TestScalarFieldAccess.getDescriptor()); }); } @@ -399,7 +402,7 @@ void testValidateMissingField() { void testScalarThenFanned() { final KeyExpression expression = concat( field("field"), - field("repeat_me", FanType.FanOut)); + field("repeat_me", FanOut)); expression.validate(TestScalarFieldAccess.getDescriptor()); assertTrue(expression.createsDuplicates()); assertEquals(Arrays.asList( @@ -415,7 +418,7 @@ void testScalarThenFanned() { @Test void testFannedThenScalar() { final KeyExpression expression = concat( - field("repeat_me", FanType.FanOut), + field("repeat_me", FanOut), field("field")); expression.validate(TestScalarFieldAccess.getDescriptor()); assertTrue(expression.createsDuplicates()); @@ -439,7 +442,7 @@ void testValidateThenFailsOnFirst() { @Test void testValidateThenFailsOnSecond() { assertThrows(KeyExpression.InvalidExpressionException.class, () -> { - concat(field("repeat_me", FanType.FanOut), field("field", FanType.FanOut)) + concat(field("repeat_me", FanOut), field("field", FanOut)) .validate(TestScalarFieldAccess.getDescriptor()); }); } @@ -463,7 +466,7 @@ void testNestedScalars() { @Test void testNestedRepeats() { final KeyExpression expression = - field("repeated_nesty", FanType.FanOut).nest("regular_old_field"); + field("repeated_nesty", FanOut).nest("regular_old_field"); expression.validate(NestedField.getDescriptor()); assertTrue(expression.createsDuplicates()); assertEquals(Arrays.asList( @@ -481,7 +484,7 @@ void testNestedRepeats() { @Test void testNestedThenRepeats() { final KeyExpression expression = - field("nesty").nest("repeated_field", FanType.FanOut); + field("nesty").nest("repeated_field", FanOut); expression.validate(NestedField.getDescriptor()); assertTrue(expression.createsDuplicates()); assertEquals(Arrays.asList( @@ -499,7 +502,7 @@ void testNestedThenRepeats() { @Test void testNestedThenRepeatsConcatenated() { final KeyExpression expression = - field("nesty").nest("repeated_field", FanType.Concatenate); + field("nesty").nest("repeated_field", Concatenate); expression.validate(NestedField.getDescriptor()); assertFalse(expression.createsDuplicates()); assertEquals(Collections.singletonList(scalar(Arrays.asList("lily", "rose"))), @@ -530,27 +533,27 @@ void testNestedThenConcatenatedFields() { @Test void testInvalidFanOnNested() { assertThrows(KeyExpression.InvalidExpressionException.class, () -> { - field("nesty").nest("regular_old_field", FanType.FanOut).validate(NestedField.getDescriptor()); + field("nesty").nest("regular_old_field", FanOut).validate(NestedField.getDescriptor()); }); } @Test void testInvalidFanOnParentNested() { assertThrows(KeyExpression.InvalidExpressionException.class, () -> { - field("repeated_nesty", FanType.Concatenate).nest("regular_old_field").validate(NestedField.getDescriptor()); + field("repeated_nesty", Concatenate).nest("regular_old_field").validate(NestedField.getDescriptor()); }); } @Test void testInvalidDoubleNested() { assertThrows(KeyExpression.InvalidExpressionException.class, () -> { - field("nesty").nest(field("nesty").nest("regular_old_field", FanType.FanOut)).validate(NestedField.getDescriptor()); + field("nesty").nest(field("nesty").nest("regular_old_field", FanOut)).validate(NestedField.getDescriptor()); }); } @Test void testValidDoubleNested() { - field("nesty").nest(field("nesty").nest("repeated_field", FanType.FanOut)).validate(NestedField.getDescriptor()); + field("nesty").nest(field("nesty").nest("repeated_field", FanOut)).validate(NestedField.getDescriptor()); } @Test @@ -562,7 +565,7 @@ void testValidDoubleNested2() { void testNestWithParentField() { final KeyExpression expression = concat( field("regular_old_field"), - field("repeated_nesty", FanType.FanOut).nest("regular_old_field")); + field("repeated_nesty", FanOut).nest("regular_old_field")); expression.validate(NestedField.getDescriptor()); assertTrue(expression.createsDuplicates()); assertEquals(Arrays.asList( @@ -582,9 +585,9 @@ void testNestWithParentField() { @Test void testNestWithParentField2() { final KeyExpression expression = - field("repeated_nesty", FanType.FanOut).nest( + field("repeated_nesty", FanOut).nest( field("regular_old_field"), - field("repeated_field", FanType.FanOut)); + field("repeated_field", FanOut)); expression.validate(NestedField.getDescriptor()); assertTrue(expression.createsDuplicates()); assertEquals(Arrays.asList( @@ -605,9 +608,9 @@ void testNestWithParentField2() { void testDoubleNested() { final KeyExpression expression = concat( field("id"), - field("order", FanType.FanOut).nest( + field("order", FanOut).nest( field("id"), - field("item", FanType.FanOut).nest( + field("item", FanOut).nest( field("id"), field("name") )), @@ -639,9 +642,9 @@ void testDoubleNested() { void testDoubleNestedWithExtraConcats() { final KeyExpression expressionWithConcats = concat( field("id"), - field("order", FanType.FanOut).nest( + field("order", FanOut).nest( concat(field("id"), - field("item", FanType.FanOut).nest(concat( + field("item", FanOut).nest(concat( field("id"), field("name")) ))), @@ -649,9 +652,9 @@ void testDoubleNestedWithExtraConcats() { field("last_name")); final KeyExpression expressionWithoutConcats = concat( field("id"), - field("order", FanType.FanOut).nest( + field("order", FanOut).nest( field("id"), - field("item", FanType.FanOut).nest( + field("item", FanOut).nest( field("id"), field("name") )), @@ -690,7 +693,7 @@ void testThenFlattens() { @Test void testList() { - final KeyExpression list = list(field("field"), field("repeat_me", FanType.Concatenate)); + final KeyExpression list = list(field("field"), field("repeat_me", Concatenate)); list.validate(TestScalarFieldAccess.getDescriptor()); assertEquals(Collections.singletonList(concatenate( scalar("Plants").values(), @@ -700,10 +703,10 @@ void testList() { @Test void testSerializeField() { - final FieldKeyExpression f1 = field("f1", FanType.FanOut, Key.Evaluated.NullStandin.NULL_UNIQUE); + final FieldKeyExpression f1 = field("f1", FanOut, Key.Evaluated.NullStandin.NULL_UNIQUE); final FieldKeyExpression f1Deserialized = new FieldKeyExpression(f1.toProto()); assertEquals("f1", f1Deserialized.getFieldName()); - assertEquals(FanType.FanOut, f1Deserialized.getFanType()); + assertEquals(FanOut, f1Deserialized.getFanType()); assertEquals(Key.Evaluated.NullStandin.NULL_UNIQUE, f1Deserialized.getNullStandin()); } @@ -725,17 +728,17 @@ void testSerializeList() { @Test void testSerializeNesting() { - final NestingKeyExpression nest = field("f1").nest(field("f2", FanType.FanOut).nest("f3")); + final NestingKeyExpression nest = field("f1").nest(field("f2", FanOut).nest("f3")); final NestingKeyExpression reserialized = new NestingKeyExpression(nest.toProto()); assertEquals("f1", reserialized.getParent().getFieldName()); final NestingKeyExpression child = (NestingKeyExpression) reserialized.getChild(); assertEquals("f2", child.getParent().getFieldName()); - assertEquals(FanType.FanOut, child.getParent().getFanType()); + assertEquals(FanOut, child.getParent().getFanType()); } @Test void testSplit() { - final SplitKeyExpression split = field("repeat_me", FanType.FanOut).split(3); + final SplitKeyExpression split = field("repeat_me", FanOut).split(3); split.validate(TestScalarFieldAccess.getDescriptor()); assertEquals(Arrays.asList( concatenate("one", "two", "three"), @@ -748,7 +751,7 @@ void testSplit() { @Test void testSplitBad() { assertThrows(RecordCoreException.class, () -> { - final SplitKeyExpression split = field("repeat_me", FanType.FanOut).split(4); + final SplitKeyExpression split = field("repeat_me", FanOut).split(4); split.validate(TestScalarFieldAccess.getDescriptor()); evaluate(split, numbers); }); @@ -757,7 +760,7 @@ void testSplitBad() { @Test void testSplitConcat() { final ThenKeyExpression splitConcat = concat(field("field"), - field("repeat_me", FanType.FanOut).split(3)); + field("repeat_me", FanOut).split(3)); splitConcat.validate(TestScalarFieldAccess.getDescriptor()); assertEquals(Arrays.asList( concatenate("numbers", "one", "two", "three"), @@ -767,7 +770,7 @@ void testSplitConcat() { } public static Stream getPrefixKeyComparisons() { - final KeyExpression nestedKeyWithValue = keyWithValue(field("a", FanType.FanOut).nest( + final KeyExpression nestedKeyWithValue = keyWithValue(field("a", FanOut).nest( concat(field("b"), field("c"), field("d"))), 2); return Stream.of( @@ -810,38 +813,38 @@ public static Stream getPrefixKeyComparisons() { Arguments.of(field("a"), field("a"), true), - Arguments.of(field("a", FanType.FanOut), - field("a", FanType.FanOut), + Arguments.of(field("a", FanOut), + field("a", FanOut), true), - Arguments.of(field("a", FanType.Concatenate), - field("a", FanType.Concatenate), + Arguments.of(field("a", Concatenate), + field("a", Concatenate), true), - Arguments.of(field("a", FanType.FanOut), - field("a", FanType.Concatenate), + Arguments.of(field("a", FanOut), + field("a", Concatenate), false), - Arguments.of(field("a", FanType.FanOut), - field("a", FanType.None), + Arguments.of(field("a", FanOut), + field("a", None), false), - Arguments.of(field("a", FanType.Concatenate), - field("a", FanType.FanOut), + Arguments.of(field("a", Concatenate), + field("a", FanOut), false), - Arguments.of(field("a", FanType.Concatenate), - field("a", FanType.None), + Arguments.of(field("a", Concatenate), + field("a", None), false), - Arguments.of(field("a", FanType.None), - field("a", FanType.Concatenate), + Arguments.of(field("a", None), + field("a", Concatenate), false), - Arguments.of(field("a", FanType.None), - field("a", FanType.FanOut), + Arguments.of(field("a", None), + field("a", FanOut), false), - Arguments.of(field("a", FanType.FanOut).nest("b"), - field("a", FanType.FanOut).nest(concat(field("b"), field("c"))), + Arguments.of(field("a", FanOut).nest("b"), + field("a", FanOut).nest(concat(field("b"), field("c"))), true), - Arguments.of(field("a", FanType.FanOut).nest("b"), - concat(field("a", FanType.FanOut).nest("b"), field("a", FanType.FanOut).nest("c")), + Arguments.of(field("a", FanOut).nest("b"), + concat(field("a", FanOut).nest("b"), field("a", FanOut).nest("c")), true), - Arguments.of(field("a", FanType.FanOut).nest(concat(field("b"), field("c"))), - concat(field("a", FanType.FanOut).nest("b"), field("a", FanType.FanOut).nest("c")), + Arguments.of(field("a", FanOut).nest(concat(field("b"), field("c"))), + concat(field("a", FanOut).nest("b"), field("a", FanOut).nest("c")), false), Arguments.of(concat(field("a"), VERSION), concat(field("a"), field("b")), @@ -872,22 +875,22 @@ public static Stream getPrefixKeyComparisons() { Arguments.of(concat(field("a"), field("b")), keyWithValue(concat(field("a"), field("b")), 1), false), - Arguments.of(field("a", FanType.FanOut).nest(field("b")), + Arguments.of(field("a", FanOut).nest(field("b")), nestedKeyWithValue, true), - Arguments.of(field("a", FanType.FanOut).nest(concat(field("b"), field("c"))), + Arguments.of(field("a", FanOut).nest(concat(field("b"), field("c"))), nestedKeyWithValue, true), - Arguments.of(field("a", FanType.FanOut).nest( + Arguments.of(field("a", FanOut).nest( concat(field("b"), field("c"), field("d"))), nestedKeyWithValue, false), - Arguments.of(concat(field("a", FanType.FanOut).nest( - field("b")), field("a", FanType.FanOut).nest("b")), + Arguments.of(concat(field("a", FanOut).nest( + field("b")), field("a", FanOut).nest("b")), nestedKeyWithValue, false), - Arguments.of(concat(field("a", FanType.FanOut).nest( - field("b")), field("a", FanType.FanOut).nest("c")), + Arguments.of(concat(field("a", FanOut).nest( + field("b")), field("a", FanOut).nest("c")), nestedKeyWithValue, false)); } @@ -909,8 +912,8 @@ static Stream testRecordTypePrefix() { Arguments.of(function("transpose", concat(field("foo"), recordType())), false), // record actually is first in result, but that's hidden behind the function implementation Arguments.of(list(recordType(), field("foo")), false), Arguments.of(list(field("foo"), recordType()), false), - Arguments.of(new SplitKeyExpression(concat(recordType(), field("foo", FanType.FanOut)), 2), false), // this maybe should be true? it's conservative for this to return false - Arguments.of(new SplitKeyExpression(concat(field("foo", FanType.FanOut), recordType()), 2), false), + Arguments.of(new SplitKeyExpression(concat(recordType(), field("foo", FanOut)), 2), false), // this maybe should be true? it's conservative for this to return false + Arguments.of(new SplitKeyExpression(concat(field("foo", FanOut), recordType()), 2), false), Arguments.of(recordType(), true), Arguments.of(VERSION, false), Arguments.of(field("foo").groupBy(recordType()), true), @@ -959,12 +962,12 @@ static Stream getLosslessNormalizationKeys() { Arguments.of(recordType(), true), Arguments.of(list(recordType(), field("foo")), true), Arguments.of(VERSION, true), - Arguments.of(new SplitKeyExpression(concat(field("foo", FanType.FanOut), recordType()), 2), false), + Arguments.of(new SplitKeyExpression(concat(field("foo", FanOut), recordType()), 2), false), Arguments.of(concat(field("foo"), field("bar")), true), Arguments.of(field("foo").groupBy(field("bar")), true), Arguments.of(field("parent").nest(field("foo"), field("bar")), true), - Arguments.of(field("parent").nest(field("child", FanType.FanOut).nest(field("foo"), field("bar"))), false), - Arguments.of(new GroupingKeyExpression(field("parent", FanType.FanOut).nest(field("foo"), field("bar")), 1), false) + Arguments.of(field("parent").nest(field("child", FanOut).nest(field("foo"), field("bar"))), false), + Arguments.of(new GroupingKeyExpression(field("parent", FanOut).nest(field("foo"), field("bar")), 1), false) ); } From e1c3f413677e619ca507d2a4c75a235f2996d8e6 Mon Sep 17 00:00:00 2001 From: Robert Brunel Date: Fri, 1 May 2026 18:15:12 +0100 Subject: [PATCH 2/3] Extend `KeyExpressionTest` tests for `nest()` to exercise combinations of null standins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the light of Issue #4129, this change adds systematic tests that document the current behavior of `field(…).nest(field(…))` key expressions for different combinations of fan types and null standins. --- .../record/metadata/KeyExpressionTest.java | 406 +++++++++++++++--- 1 file changed, 353 insertions(+), 53 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java index 1dd3b67da3..9f1b86393a 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java @@ -61,7 +61,9 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static com.apple.foundationdb.record.metadata.Key.Evaluated.NullStandin.NOT_NULL; import static com.apple.foundationdb.record.metadata.Key.Evaluated.NullStandin.NULL; +import static com.apple.foundationdb.record.metadata.Key.Evaluated.NullStandin.NULL_UNIQUE; import static com.apple.foundationdb.record.metadata.Key.Evaluated.concatenate; import static com.apple.foundationdb.record.metadata.Key.Evaluated.scalar; import static com.apple.foundationdb.record.metadata.Key.Expressions.concat; @@ -337,9 +339,11 @@ void testConcatenateSingleRepeatedField() { assertFalse(expression.createsDuplicates()); assertEquals(Collections.singletonList(scalar(Arrays.asList("Boxes", "Bowls"))), evaluate(expression, plantsBoxesAndBowls)); + // `repeat_me` has 0 repetitions: Concatenate yields the empty list. assertEquals(Collections.singletonList(scalar(Collections.emptyList())), evaluate(expression, emptyScalar)); - assertEquals(Collections.singletonList(scalar(Collections.emptyList())), + // Null record: Propagates the field’s null standin (by default NULL). + assertEquals(Collections.singletonList(scalar(NULL)), evaluate(expression, null)); } @@ -351,9 +355,11 @@ void testFieldThenConcatenateRepeated() { assertFalse(expression.createsDuplicates()); assertEquals(Collections.singletonList(Key.Evaluated.concatenate("Plants", Arrays.asList("Boxes", "Bowls"))), evaluate(expression, plantsBoxesAndBowls)); + // Both fields unset: The scalar `field` yields the NULL standin; concatenate yields the empty list. assertEquals(Collections.singletonList(Key.Evaluated.concatenate(NULL, Collections.emptyList())), evaluate(expression, emptyScalar)); - assertEquals(Collections.singletonList(Key.Evaluated.concatenate(NULL, Collections.emptyList())), + // Null record: Both parts propagate their null standin (by default NULL) and yield NULL. + assertEquals(Collections.singletonList(Key.Evaluated.concatenate(NULL, NULL)), evaluate(expression, null)); } @@ -447,76 +453,370 @@ void testValidateThenFailsOnSecond() { }); } - @Test - void testNestedScalars() { - final KeyExpression expression = field("nesty").nest("regular_old_field"); + @FunctionalInterface + private interface NestSingularSingularCase { + void verify(@Nonnull Key.Evaluated.NullStandin parentStandin, + @Nonnull Key.Evaluated.NullStandin childStandin, + @Nonnull List expected); + } + + /** + * Builds the key expression under test for {@link #testNestSingularSingular()}. + */ + @Nonnull + private KeyExpression buildNestSingularSingularExpr(@Nonnull Key.Evaluated.NullStandin parentStandin, + @Nonnull Key.Evaluated.NullStandin childStandin) { + final KeyExpression expression = + field("nesty", None, parentStandin) + .nest(field("regular_old_field", None, childStandin)); + + // Also run `validate()` and validate that the `None` fan type implies `!createsDuplicates()`. expression.validate(NestedField.getDescriptor()); assertFalse(expression.createsDuplicates()); - assertEquals(Collections.singletonList( - scalar("Mother")), - evaluate(expression, matryoshkaDolls)); - assertEquals(Collections.singletonList(Key.Evaluated.NULL), - evaluate(expression, emptyNested)); - assertEquals(Collections.singletonList(Key.Evaluated.NULL), - evaluate(expression, lonelyDoll)); - assertEquals(Collections.singletonList(Key.Evaluated.NULL), - evaluate(expression, null)); + + return expression; } + /** + * {@code field(…).nest(field(…))} key expression where both the parent and the child are singular fields. + */ @Test - void testNestedRepeats() { + void testNestSingularSingular() { + // Some shortcuts. + final var emptyStringEntry = ImmutableList.of(scalar("")); + final var nullEntry = ImmutableList.of(scalar(NULL)); + final var nullUniqueEntry = ImmutableList.of(scalar(NULL_UNIQUE)); + final var notNullEntry = ImmutableList.of(scalar(NOT_NULL)); + + // Case: Parent present, child present. + // + // The `matryoshkaDolls` message has `nesty.regular_old_field = "Mother"`. + // The standins don’t matter, since both parent and child are present. + final NestSingularSingularCase expr1 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestSingularSingularExpr(parentStandin, childStandin), matryoshkaDolls)); + final var motherEntry = ImmutableList.of(scalar("Mother")); + expr1.verify(NULL, NULL, motherEntry); + expr1.verify(NULL, NULL_UNIQUE, motherEntry); + expr1.verify(NULL, NOT_NULL, motherEntry); + expr1.verify(NULL_UNIQUE, NULL, motherEntry); + expr1.verify(NULL_UNIQUE, NULL_UNIQUE, motherEntry); + expr1.verify(NULL_UNIQUE, NOT_NULL, motherEntry); + expr1.verify(NOT_NULL, NULL, motherEntry); + expr1.verify(NOT_NULL, NULL_UNIQUE, motherEntry); + expr1.verify(NOT_NULL, NOT_NULL, motherEntry); + + // Case: Parent present (as an empty sub-message), child absent. + // + // The `lonelyDoll` message has `nesty` explicitly set to an empty sub-message; the child is unset. + // The parent standin is irrelevant. + // For the child, NOT_NULL substitutes the proto default "", whereas NULL/NULL_UNIQUE yield the standin. + final NestSingularSingularCase expr2 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestSingularSingularExpr(parentStandin, childStandin), lonelyDoll)); + expr2.verify(NULL, NULL, nullEntry); + expr2.verify(NULL, NULL_UNIQUE, nullUniqueEntry); + expr2.verify(NULL, NOT_NULL, emptyStringEntry); + expr2.verify(NULL_UNIQUE, NULL, nullEntry); + expr2.verify(NULL_UNIQUE, NULL_UNIQUE, nullUniqueEntry); + expr2.verify(NULL_UNIQUE, NOT_NULL, emptyStringEntry); + expr2.verify(NOT_NULL, NULL, nullEntry); + expr2.verify(NOT_NULL, NULL_UNIQUE, nullUniqueEntry); + expr2.verify(NOT_NULL, NOT_NULL, emptyStringEntry); + + // Case: Parent absent on a non-null record. + // + // The `emptyNested` message has `nesty` absent. + // For the parent, NOT_NULL substitutes an empty sub-message; the child then sees a set-but-empty message, + // so the child’s NOT_NULL substitutes the proto default "". + // NULL/NULL_UNIQUE on the parent funnel null to the child, which then takes its own null path; and for + // `None`, `getNullResult()` emits the standin verbatim, so the child NOT_NULL yields the NOT_NULL + // sentinel rather than "". + // TODO This seems to be a bug; the NOT_NULL standin is not supposed to be emitted as is. + final NestSingularSingularCase expr3 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestSingularSingularExpr(parentStandin, childStandin), emptyNested)); + expr3.verify(NULL, NULL, nullEntry); + expr3.verify(NULL, NULL_UNIQUE, nullUniqueEntry); + expr3.verify(NULL, NOT_NULL, notNullEntry); + expr3.verify(NULL_UNIQUE, NULL, nullEntry); + expr3.verify(NULL_UNIQUE, NULL_UNIQUE, nullUniqueEntry); + expr3.verify(NULL_UNIQUE, NOT_NULL, notNullEntry); + expr3.verify(NOT_NULL, NULL, nullEntry); + expr3.verify(NOT_NULL, NULL_UNIQUE, nullUniqueEntry); + expr3.verify(NOT_NULL, NOT_NULL, emptyStringEntry); + + // Case: Null record. + // + // The parent standin is irrelevant, as `FieldKeyExpression` short-circuits on `message == null`. + // The child sees null and takes its null path. + // TODO For NOT_NULL the code currently emits the standin as is, not "". + final NestSingularSingularCase expr4 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestSingularSingularExpr(parentStandin, childStandin), null)); + expr4.verify(NULL, NULL, nullEntry); + expr4.verify(NULL, NULL_UNIQUE, nullUniqueEntry); + expr4.verify(NULL, NOT_NULL, notNullEntry); + expr4.verify(NULL_UNIQUE, NULL, nullEntry); + expr4.verify(NULL_UNIQUE, NULL_UNIQUE, nullUniqueEntry); + expr4.verify(NULL_UNIQUE, NOT_NULL, notNullEntry); + expr4.verify(NOT_NULL, NULL, nullEntry); + expr4.verify(NOT_NULL, NULL_UNIQUE, nullUniqueEntry); + expr4.verify(NOT_NULL, NOT_NULL, notNullEntry); + } + + @FunctionalInterface + private interface NestRepeatedSingularCase { + void verify(@Nonnull Key.Evaluated.NullStandin parentStandin, + @Nonnull Key.Evaluated.NullStandin childStandin, + @Nonnull List expected); + } + + /** + * Builds the key expression under test for {@link #testNestRepeatedSingular()}. + */ + @Nonnull + private KeyExpression buildNestRepeatedSingularExpr(@Nonnull Key.Evaluated.NullStandin parentStandin, + @Nonnull Key.Evaluated.NullStandin childStandin) { final KeyExpression expression = - field("repeated_nesty", FanOut).nest("regular_old_field"); + field("repeated_nesty", FanOut, parentStandin) + .nest(field("regular_old_field", None, childStandin)); + + // Also run `validate()` and validate that only the `FanOut` fan type implies `createsDuplicates()`. expression.validate(NestedField.getDescriptor()); assertTrue(expression.createsDuplicates()); - assertEquals(Arrays.asList( - scalar("Daughter"), - scalar("Sister")), - evaluate(expression, matryoshkaDolls)); - assertEquals(Collections.emptyList(), - evaluate(expression, emptyNested)); - assertEquals(Arrays.asList(Key.Evaluated.NULL, Key.Evaluated.NULL), - evaluate(expression, lonelyDoll)); - assertEquals(Collections.emptyList(), - evaluate(expression, null)); + + return expression; } + /** + * {@code field(…).nest(field(…))} key expression where the parent is a repeated field and the child is singular. + * + *

Note that, because the parent is a repeated field evaluated with {@link FanType#FanOut}, its standin is never + * consulted: The repeated branch of {@link FieldKeyExpression} uses the proto-level repetition count directly, + * and on a null record {@code getNullResult()} for FanOut returns {@code []}. The parent standin there does + * not matter, but we exercise it nevertheless for completeness. + */ @Test - void testNestedThenRepeats() { + void testNestRepeatedSingular() { + // Some shortcuts. + final ImmutableList noEntry = ImmutableList.of(); + final var twoEmptyStringEntries = ImmutableList.of(scalar(""), scalar("")); + final var twoNullEntries = ImmutableList.of(scalar(NULL), scalar(NULL)); + final var twoNullUniqueEntries = ImmutableList.of(scalar(NULL_UNIQUE), scalar(NULL_UNIQUE)); + + // Case: Parent present (2 repetitions), child present in each. + // + // The `matryoshkaDolls` message has `repeated_nesty = [NestedField("Daughter"), NestedField("Sister")]`. + // Both standins don’t matter. + final NestRepeatedSingularCase expr1 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestRepeatedSingularExpr(parentStandin, childStandin), matryoshkaDolls)); + final var daughterSisterEntries = ImmutableList.of(scalar("Daughter"), scalar("Sister")); + expr1.verify(NULL, NULL, daughterSisterEntries); + expr1.verify(NULL, NULL_UNIQUE, daughterSisterEntries); + expr1.verify(NULL, NOT_NULL, daughterSisterEntries); + expr1.verify(NULL_UNIQUE, NULL, daughterSisterEntries); + expr1.verify(NULL_UNIQUE, NULL_UNIQUE, daughterSisterEntries); + expr1.verify(NULL_UNIQUE, NOT_NULL, daughterSisterEntries); + expr1.verify(NOT_NULL, NULL, daughterSisterEntries); + expr1.verify(NOT_NULL, NULL_UNIQUE, daughterSisterEntries); + expr1.verify(NOT_NULL, NOT_NULL, daughterSisterEntries); + + // Case: Parent present (2 repetitions), child absent in each. + // + // The `lonelyDoll` message has 2 empty `repeated_nesty` entries, each with `regular_old_field` unset. + // For the child, NOT_NULL substitutes ""; whereas NULL/NULL_UNIQUE yield the standin, once per sub-message. + final NestRepeatedSingularCase expr2 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestRepeatedSingularExpr(parentStandin, childStandin), lonelyDoll)); + expr2.verify(NULL, NULL, twoNullEntries); + expr2.verify(NULL, NULL_UNIQUE, twoNullUniqueEntries); + expr2.verify(NULL, NOT_NULL, twoEmptyStringEntries); + expr2.verify(NULL_UNIQUE, NULL, twoNullEntries); + expr2.verify(NULL_UNIQUE, NULL_UNIQUE, twoNullUniqueEntries); + expr2.verify(NULL_UNIQUE, NOT_NULL, twoEmptyStringEntries); + expr2.verify(NOT_NULL, NULL, twoNullEntries); + expr2.verify(NOT_NULL, NULL_UNIQUE, twoNullUniqueEntries); + expr2.verify(NOT_NULL, NOT_NULL, twoEmptyStringEntries); + + // Case: Parent has 0 repetitions. + // + // The `emptyNested` message has no `repeated_nesty` entries. + // `FanOut` on an empty repeated field yields no entries, so the child is never evaluated. + final NestRepeatedSingularCase expr3 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestRepeatedSingularExpr(parentStandin, childStandin), emptyNested)); + expr3.verify(NULL, NULL, noEntry); + expr3.verify(NULL, NULL_UNIQUE, noEntry); + expr3.verify(NULL, NOT_NULL, noEntry); + expr3.verify(NULL_UNIQUE, NULL, noEntry); + expr3.verify(NULL_UNIQUE, NULL_UNIQUE, noEntry); + expr3.verify(NULL_UNIQUE, NOT_NULL, noEntry); + expr3.verify(NOT_NULL, NULL, noEntry); + expr3.verify(NOT_NULL, NULL_UNIQUE, noEntry); + expr3.verify(NOT_NULL, NOT_NULL, noEntry); + + // Case: Null record. + // + // `FieldKeyExpression` short-circuits on `message == null`; for `FanOut`, `getNullResult()` yields []. + final NestRepeatedSingularCase expr4 = (parentStandin, childStandin, expected) -> + assertEquals(expected, evaluate(buildNestRepeatedSingularExpr(parentStandin, childStandin), null)); + expr4.verify(NULL, NULL, noEntry); + expr4.verify(NULL, NULL_UNIQUE, noEntry); + expr4.verify(NULL, NOT_NULL, noEntry); + expr4.verify(NULL_UNIQUE, NULL, noEntry); + expr4.verify(NULL_UNIQUE, NULL_UNIQUE, noEntry); + expr4.verify(NULL_UNIQUE, NOT_NULL, noEntry); + expr4.verify(NOT_NULL, NULL, noEntry); + expr4.verify(NOT_NULL, NULL_UNIQUE, noEntry); + expr4.verify(NOT_NULL, NOT_NULL, noEntry); + } + + @FunctionalInterface + private interface NestSingularRepeatedCase { + void verify(@Nonnull Key.Evaluated.NullStandin parentStandin, + @Nonnull Key.Evaluated.NullStandin childStandin, + @Nonnull FanType childFanType, + @Nonnull List expected); + } + + /** + * Builds the key expression under test for {@link #testNestSingularRepeated()}. + */ + @Nonnull + private KeyExpression buildNestSingularRepeatedExpr(@Nonnull Key.Evaluated.NullStandin parentStandin, + @Nonnull Key.Evaluated.NullStandin childStandin, + @Nonnull FanType childFanType) { final KeyExpression expression = - field("nesty").nest("repeated_field", FanOut); + field("nesty", None, parentStandin) + .nest(field("repeated_field", childFanType, childStandin)); + + // Also run `validate()` and validate that only the `FanOut` fan type implies `createsDuplicates()`. expression.validate(NestedField.getDescriptor()); - assertTrue(expression.createsDuplicates()); - assertEquals(Arrays.asList( - scalar("lily"), - scalar("rose")), - evaluate(expression, matryoshkaDolls)); - assertEquals(Collections.emptyList(), - evaluate(expression, emptyNested)); - assertEquals(Collections.emptyList(), - evaluate(expression, lonelyDoll)); - assertEquals(Collections.emptyList(), - evaluate(expression, null)); + assertEquals(childFanType == FanOut, expression.createsDuplicates()); + + return expression; } + /** + * {@code field(…).nest(field(…))} key expression where the parent is a singular field and the child is repeated. + */ @Test - void testNestedThenRepeatsConcatenated() { - final KeyExpression expression = - field("nesty").nest("repeated_field", Concatenate); - expression.validate(NestedField.getDescriptor()); - assertFalse(expression.createsDuplicates()); - assertEquals(Collections.singletonList(scalar(Arrays.asList("lily", "rose"))), - evaluate(expression, matryoshkaDolls)); - assertEquals(Collections.singletonList(scalar(Collections.emptyList())), - evaluate(expression, emptyNested)); - assertEquals(Collections.singletonList(scalar(Collections.emptyList())), - evaluate(expression, lonelyDoll)); - assertEquals(Collections.singletonList(scalar(Collections.emptyList())), - evaluate(expression, null)); + void testNestSingularRepeated() { + // Some shortcuts. + final var Concatenate = FanType.Concatenate; + final var FanOut = FanType.FanOut; + final var emptyList = Collections.emptyList(); + final var emptyListEntry = ImmutableList.of(scalar(emptyList)); + final ImmutableList noEntry = ImmutableList.of(); + + // Case: Parent present, child present. + // + // The `matryoshkaDolls` message has `nesty.repeated_field = [lily, rose]`. + // Both standins don’t matter, since both parent and child are present. + // Concatenate yields a single entry holding the `List`; FanOut yields the exploded array. + final NestSingularRepeatedCase expr1 = (parentStandin, childStandin, childFanType, expected) -> + assertEquals(expected, evaluate(buildNestSingularRepeatedExpr(parentStandin, childStandin, childFanType), matryoshkaDolls)); + final var concatenatedArray = ImmutableList.of(scalar(Arrays.asList("lily", "rose"))); + final var fannedOutArray = ImmutableList.of(scalar("lily"), scalar("rose")); + expr1.verify(NULL, NULL, Concatenate, concatenatedArray); + expr1.verify(NULL, NULL_UNIQUE, Concatenate, concatenatedArray); + expr1.verify(NULL, NOT_NULL, Concatenate, concatenatedArray); + expr1.verify(NULL_UNIQUE, NULL, Concatenate, concatenatedArray); + expr1.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, concatenatedArray); + expr1.verify(NULL_UNIQUE, NOT_NULL, Concatenate, concatenatedArray); + expr1.verify(NOT_NULL, NULL, Concatenate, concatenatedArray); + expr1.verify(NOT_NULL, NULL_UNIQUE, Concatenate, concatenatedArray); + expr1.verify(NOT_NULL, NOT_NULL, Concatenate, concatenatedArray); + expr1.verify(NULL, NULL, FanOut, fannedOutArray); + expr1.verify(NULL, NULL_UNIQUE, FanOut, fannedOutArray); + expr1.verify(NULL, NOT_NULL, FanOut, fannedOutArray); + expr1.verify(NULL_UNIQUE, NULL, FanOut, fannedOutArray); + expr1.verify(NULL_UNIQUE, NULL_UNIQUE, FanOut, fannedOutArray); + expr1.verify(NULL_UNIQUE, NOT_NULL, FanOut, fannedOutArray); + expr1.verify(NOT_NULL, NULL, FanOut, fannedOutArray); + expr1.verify(NOT_NULL, NULL_UNIQUE, FanOut, fannedOutArray); + expr1.verify(NOT_NULL, NOT_NULL, FanOut, fannedOutArray); + + // Case: Parent present, child absent (0 repetitions) + // + // The `lonelyDoll` message has `nesty` set but `repeated_field` has 0 repetitions. + // Both standins don’t matter. + // Concatenate yields a single entry holding the empty list; FanOut yields no entries. + final NestSingularRepeatedCase expr2 = (parentStandin, childStandin, childFanType, expected) -> + assertEquals(expected, evaluate(buildNestSingularRepeatedExpr(parentStandin, childStandin, childFanType), lonelyDoll)); + expr2.verify(NULL, NULL, Concatenate, emptyListEntry); + expr2.verify(NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr2.verify(NULL, NOT_NULL, Concatenate, emptyListEntry); + expr2.verify(NULL_UNIQUE, NULL, Concatenate, emptyListEntry); + expr2.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, emptyListEntry); + expr2.verify(NULL_UNIQUE, NOT_NULL, Concatenate, emptyListEntry); + expr2.verify(NOT_NULL, NULL, Concatenate, emptyListEntry); + expr2.verify(NOT_NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr2.verify(NOT_NULL, NOT_NULL, Concatenate, emptyListEntry); + expr2.verify(NULL, NULL, FanOut, noEntry); + expr2.verify(NULL, NULL_UNIQUE, FanOut, noEntry); + expr2.verify(NULL, NOT_NULL, FanOut, noEntry); + expr2.verify(NULL_UNIQUE, NULL, FanOut, noEntry); + expr2.verify(NULL_UNIQUE, NULL_UNIQUE, FanOut, noEntry); + expr2.verify(NULL_UNIQUE, NOT_NULL, FanOut, noEntry); + expr2.verify(NOT_NULL, NULL, FanOut, noEntry); + expr2.verify(NOT_NULL, NULL_UNIQUE, FanOut, noEntry); + expr2.verify(NOT_NULL, NOT_NULL, FanOut, noEntry); + + // Case: Parent absent (but on a non-null record) + // + // The `emptyNested` record has `nesty` absent. On the parent, the NOT_NULL standin effectively substitutes an + // empty sub-message, so the child sees 0 repetitions and goes through its repeated-field branch. For parent + // NULL/NULL_UNIQUE the parent funnels a `null` record to the child, which then takes its own null path; for + // both Concatenate and FanOut, `getNullResult()` does not consult the child standin either, so the child standin + // has no observable effect. FanOut yields no entries; Concatenate yields a single entry holding the empty list. + final NestSingularRepeatedCase expr3 = (parentStandin, childStandin, childFanType, expected) -> + assertEquals(expected, evaluate(buildNestSingularRepeatedExpr(parentStandin, childStandin, childFanType), emptyNested)); + expr3.verify(NULL, NULL, Concatenate, emptyListEntry); + expr3.verify(NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr3.verify(NULL, NOT_NULL, Concatenate, emptyListEntry); + expr3.verify(NULL_UNIQUE, NULL, Concatenate, emptyListEntry); + expr3.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, emptyListEntry); + expr3.verify(NULL_UNIQUE, NOT_NULL, Concatenate, emptyListEntry); + expr3.verify(NOT_NULL, NULL, Concatenate, emptyListEntry); + expr3.verify(NOT_NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr3.verify(NOT_NULL, NOT_NULL, Concatenate, emptyListEntry); + expr3.verify(NULL, NULL, FanOut, noEntry); + expr3.verify(NULL, NULL_UNIQUE, FanOut, noEntry); + expr3.verify(NULL, NOT_NULL, FanOut, noEntry); + expr3.verify(NULL_UNIQUE, NULL, FanOut, noEntry); + expr3.verify(NULL_UNIQUE, NULL_UNIQUE, FanOut, noEntry); + expr3.verify(NULL_UNIQUE, NOT_NULL, FanOut, noEntry); + expr3.verify(NOT_NULL, NULL, FanOut, noEntry); + expr3.verify(NOT_NULL, NULL_UNIQUE, FanOut, noEntry); + expr3.verify(NOT_NULL, NOT_NULL, FanOut, noEntry); + + // Case: Null record + // + // The parent standin doesn’t matter, as `FieldKeyExpression` short-circuits on `message == null` before + // consulting the parent standin. The child standin doesn’t matter either: for both Concatenate and FanOut, + // `getNullResult()` does not consult the standin. FanOut yields no entries; Concatenate yields a single entry + // holding the empty list. + final NestSingularRepeatedCase expr4 = (parentStandin, childStandin, childFanType, expected) -> + assertEquals(expected, + evaluate(buildNestSingularRepeatedExpr(parentStandin, childStandin, childFanType), null)); + expr4.verify(NULL, NULL, Concatenate, emptyListEntry); + expr4.verify(NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr4.verify(NULL, NOT_NULL, Concatenate, emptyListEntry); + expr4.verify(NULL_UNIQUE, NULL, Concatenate, emptyListEntry); + expr4.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, emptyListEntry); + expr4.verify(NULL_UNIQUE, NOT_NULL, Concatenate, emptyListEntry); + expr4.verify(NOT_NULL, NULL, Concatenate, emptyListEntry); + expr4.verify(NOT_NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr4.verify(NOT_NULL, NOT_NULL, Concatenate, emptyListEntry); + expr4.verify(NULL, NULL, FanOut, noEntry); + expr4.verify(NULL, NULL_UNIQUE, FanOut, noEntry); + expr4.verify(NULL, NOT_NULL, FanOut, noEntry); + expr4.verify(NULL_UNIQUE, NULL, FanOut, noEntry); + expr4.verify(NULL_UNIQUE, NULL_UNIQUE, FanOut, noEntry); + expr4.verify(NULL_UNIQUE, NOT_NULL, FanOut, noEntry); + expr4.verify(NOT_NULL, NULL, FanOut, noEntry); + expr4.verify(NOT_NULL, NULL_UNIQUE, FanOut, noEntry); + expr4.verify(NOT_NULL, NOT_NULL, FanOut, noEntry); } @Test - void testNestedThenConcatenatedFields() { + void testNestSingularThenConcatenatedFields() { final KeyExpression expression = field("nesty").nest(concatenateFields("regular_old_field", "regular_int_field")); expression.validate(NestedField.getDescriptor()); assertFalse(expression.createsDuplicates()); From d245c2486954ab6deb430d6500b93a515fac84ca Mon Sep 17 00:00:00 2001 From: Robert Brunel Date: Fri, 1 May 2026 18:15:12 +0100 Subject: [PATCH 3/3] Fix the NULL result of `field()` key expressions with fan-type `Concatenate` This change makes `FieldKeyExpression#getNullResult()` propagate the null stand-in for fan-type `Concatenate` instead of returning an empty list. This aligns `Concatenate` with the `None` branch, as the null stand-in is more appropriate behavior for this case. The empty list is only correct when the stand-in is NOT_NULL, since that matches the proto default for a repeated field. This change is desirable for alignment with SQL semantics; see the example scenario described under Issue #4129. However, note that it is a **breaking change** that affects persisted index entries. Deployed indexes will have persisted a `scalar(emptyList)` in the cases described here and should be rebuilt so they instead persist the `scalar(NULL)` going forward. We expect the impact to be limited, as `Concatenate` is not commonly used, and `FieldKeyExpression#getNullResult()` is reached only when the input message passed to `evaluateMessage()` is null. This, in particular, happens when a `nest()` expression descends into an absent parent. In other words, the Concatenate field has to sit inside a `nest()` expression under an optional parent and that parent submessage is absent. Testing: * Update affected unit tests in `KeyExpressionTest`. * No existing .yamsql tests are affected. Resolves #4129. --- .../expressions/FieldKeyExpression.java | 73 ++++++++++++++++--- .../expressions/NestingKeyExpression.java | 17 +++-- .../record/metadata/KeyExpressionTest.java | 36 ++++----- 3 files changed, 91 insertions(+), 35 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/FieldKeyExpression.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/FieldKeyExpression.java index ff4fbc1af1..cb1ee30566 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/FieldKeyExpression.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/FieldKeyExpression.java @@ -44,15 +44,54 @@ import java.util.List; /** - * Take keys from a record field. - * If fieldName is a repeated field, then FanType.Concatenate turns all the - * field values into a single Key.Evaluated. If FanType.FanOut, there is one (singleton) - * Key.Evaluated for each repeated value. If this is evaluated on the null record, then - * it will the same value as if it were evaluated on a record where the field is either unset (in the case of scalar - * fields) or empty (in the case of repeated fields). In particular, if FanType.None, then this returns - * a single Key.Evaluated containing null; if FanType.FanOut, then - * this returns no Key.Evaluateds; and if FanType.Concatenate, then this returns a single - * Key.Evaluated containing the empty list. + * A {@code field()} key expression. + * + *

The {@code field()} expression takes keys from a record field specified by name. Besides the field name, it + * carries two properties that control the behavior for {@code optional} or {@code repeated} fields: + *

    + *
  • The fan type ({@link com.apple.foundationdb.record.metadata.expressions.KeyExpression.FanType FanType}).
    • + *
  • The null standin ({@link Key.Evaluated.NullStandin NullStandin}).
    • + *
+ * + *

If the field is singular and {@code required}, the fan type is irrelevant and must be set to {@code None}. + * In this case, {@code field()} always yields a single {@link Key.Evaluated} holding the scalar value. + * + *

If the field is singular and marked as {@code optional}, the {@code NullStandin} property drives the behavior in + * the “absent” case where the field is not set on the processed message: + *

    + *
  • For {@code NULL} and {@code NULL_UNIQUE}, the {@code field()} expression yields a single {@code Key.Evaluated} + * carrying the standin, which represents an indexable null value in this case. + *
  • For {@code NOT_NULL}, it substitutes the default value of the Protobuf type, such as 0 or {@code ""}. + *
+ * + *

If the field is {@code repeated}, the fan type comes into play and must be set to either {@code FanOut} or + * {@code Concatenate}: + *

    + *
  • For {@code FanOut}, the {@code field()} expression yields one {@code Key.Evaluated} per element of the repeated + * field. + *
  • For {@code Concatenate}, it yields a single {@code Key.Evaluated} holding the entire repeated field as a + * {@link java.util.List List} object. + *
+ * Note that a {@code repeated} field cannot be “absent” in the sense of {@code optional}. If the {@code repeated} field + * has 0 repetitions, {@code FanOut} will yield nothing while {@code Concatenate} will yield a single key holding an + * empty list (since the empty list is the proto default for a repeated field). + * + *

A special case occurs when the processed message itself is {@code null}. In this case, the result depending on the + * fan type and the null standin is as follows: + *

    + *
  • For a singular field and fan type {@code None}, the {@code field()} expression yields a single key carrying the + * {@code NullStandin}. + *
  • For a repeated field and fan type {@code FanOut}, it yields no entries. + *
  • For a repeated field and fan type {@code Concatenate}, if the standin is {@code NOT_NULL}, it behaves “as if 0 + * repetitions” and yields a single entry holding the empty list. Otherwise, it behaves like {@code None} and emits + * a single key carrying the null standin. + *
+ * One common scenario where the message will be {@code null} is when the {@code field()} expression is the child + * of a {@link NestingKeyExpression nest()} key expression, and the parent field evaluates to null. + * For example, in the expression {@code field("array1").nest(field("values", Concatenate))}, if {@code array1} is an + * {@code optional} field that is absent, then the outer {@code field("array1")} will yield its null standin and, + * consequently, the inner {@code field("values", …)} expression will be evaluated on a {@code null} message and in turn + * yield its null standin. */ @API(API.Status.UNSTABLE) public class FieldKeyExpression extends BaseKeyExpression implements AtomKeyExpression, KeyExpressionWithoutChildren { @@ -174,13 +213,25 @@ public List evaluateMessage(@Nullable FDBReco } } + /** + * Evaluates the case where no value can be extracted for this field. This method is called from + * {@link #evaluateMessage} in two cases: + *
    + *
  • The input {@code message} is {@code null}. + *
  • For a singular (non-{@code repeated}), {@code optional} field when that field is absent on the message, and + * the {@link #nullStandin} is not {@code NOT_NULL} (i.e., the type-default substitution does not apply). + *
+ * See {@link FieldKeyExpression} for the semantics in this case. + */ private List getNullResult() { - // As opposed to default value, in order to get indexable NULL. switch (fanType) { case FanOut: return Collections.emptyList(); case Concatenate: - return Collections.singletonList(Key.Evaluated.scalar(Collections.emptyList())); + Key.Evaluated result = (nullStandin == Key.Evaluated.NullStandin.NOT_NULL) + ? Key.Evaluated.scalar(Collections.emptyList()) + : Key.Evaluated.scalar(nullStandin); + return Collections.singletonList(result); case None: return Collections.singletonList(Key.Evaluated.scalar(nullStandin)); default: diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/NestingKeyExpression.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/NestingKeyExpression.java index 65f3d3da18..3cf8d302f0 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/NestingKeyExpression.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/NestingKeyExpression.java @@ -38,12 +38,17 @@ /** * A key expression within a nested subrecord. - * If the parent field is repeated, then the parent field must have fan type FanType.FanOut. - * In that case, this will return the nested expression evaluated against every subrecord (possibly returning - * no Key.Evaluateds if the parent field is empty). If the parent field is not repeated and not set, - * then this will evaluate the nested expression on the null record. This should return the same - * result as if the field were set to the empty message. If this expression is evaluated on the null - * record, then it will evaluate the same as if the parent field is unset or empty (depending on the fan type). + * + *

If the parent field is {@code repeated}, then the parent {@code field()} expression must have fan type + * {@code FanType.FanOut}. In that case, this will return the nested expression evaluated against every subrecord + * (possibly returning no Key.Evaluated entries at all if the parent field is empty). + * + *

If the parent field is singular (not {@code repeated}) and {@code optional} and not set, then this will evaluate + * the nested expression on the null record. See {@link FieldKeyExpression} for the exact semantics in this + * scenario. + * + *

If this expression is evaluated on the null record, then it will evaluate the same as if the parent + * field is unset or empty (depending on the fan type). */ @API(API.Status.UNSTABLE) public class NestingKeyExpression extends BaseKeyExpression implements KeyExpressionWithChild, AtomKeyExpression { diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java index 9f1b86393a..923c1d0bb0 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/KeyExpressionTest.java @@ -528,7 +528,7 @@ void testNestSingularSingular() { // For the parent, NOT_NULL substitutes an empty sub-message; the child then sees a set-but-empty message, // so the child’s NOT_NULL substitutes the proto default "". // NULL/NULL_UNIQUE on the parent funnel null to the child, which then takes its own null path; and for - // `None`, `getNullResult()` emits the standin verbatim, so the child NOT_NULL yields the NOT_NULL + // `FanType.None`, `getNullResult()` emits the standin verbatim, so the child NOT_NULL yields the NOT_NULL // sentinel rather than "". // TODO This seems to be a bug; the NOT_NULL standin is not supposed to be emitted as is. final NestSingularSingularCase expr3 = (parentStandin, childStandin, expected) -> @@ -703,6 +703,8 @@ void testNestSingularRepeated() { final var emptyList = Collections.emptyList(); final var emptyListEntry = ImmutableList.of(scalar(emptyList)); final ImmutableList noEntry = ImmutableList.of(); + final var nullEntry = ImmutableList.of(scalar(NULL)); + final var nullUniqueEntry = ImmutableList.of(scalar(NULL_UNIQUE)); // Case: Parent present, child present. // @@ -761,17 +763,16 @@ void testNestSingularRepeated() { // Case: Parent absent (but on a non-null record) // // The `emptyNested` record has `nesty` absent. On the parent, the NOT_NULL standin effectively substitutes an - // empty sub-message, so the child sees 0 repetitions and goes through its repeated-field branch. For parent - // NULL/NULL_UNIQUE the parent funnels a `null` record to the child, which then takes its own null path; for - // both Concatenate and FanOut, `getNullResult()` does not consult the child standin either, so the child standin - // has no observable effect. FanOut yields no entries; Concatenate yields a single entry holding the empty list. + // empty sub-message, so the child sees 0 repetitions; whereas NULL/NULL_UNIQUE funnel a `null` record to the + // child, which then takes its own null path. For `FanOut` both code branches collapse to [], so we get no entry + // at all. For Concatenate the child standin has an observable effect only when the parent funnels null. final NestSingularRepeatedCase expr3 = (parentStandin, childStandin, childFanType, expected) -> assertEquals(expected, evaluate(buildNestSingularRepeatedExpr(parentStandin, childStandin, childFanType), emptyNested)); - expr3.verify(NULL, NULL, Concatenate, emptyListEntry); - expr3.verify(NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr3.verify(NULL, NULL, Concatenate, nullEntry); + expr3.verify(NULL, NULL_UNIQUE, Concatenate, nullUniqueEntry); expr3.verify(NULL, NOT_NULL, Concatenate, emptyListEntry); - expr3.verify(NULL_UNIQUE, NULL, Concatenate, emptyListEntry); - expr3.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, emptyListEntry); + expr3.verify(NULL_UNIQUE, NULL, Concatenate, nullEntry); + expr3.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, nullUniqueEntry); expr3.verify(NULL_UNIQUE, NOT_NULL, Concatenate, emptyListEntry); expr3.verify(NOT_NULL, NULL, Concatenate, emptyListEntry); expr3.verify(NOT_NULL, NULL_UNIQUE, Concatenate, emptyListEntry); @@ -789,20 +790,19 @@ void testNestSingularRepeated() { // Case: Null record // // The parent standin doesn’t matter, as `FieldKeyExpression` short-circuits on `message == null` before - // consulting the parent standin. The child standin doesn’t matter either: for both Concatenate and FanOut, - // `getNullResult()` does not consult the standin. FanOut yields no entries; Concatenate yields a single entry - // holding the empty list. + // consulting the parent standin. Only the child’s null standin matters. For `FanOut` the null code path yields + // [] regardless of the child standin. final NestSingularRepeatedCase expr4 = (parentStandin, childStandin, childFanType, expected) -> assertEquals(expected, evaluate(buildNestSingularRepeatedExpr(parentStandin, childStandin, childFanType), null)); - expr4.verify(NULL, NULL, Concatenate, emptyListEntry); - expr4.verify(NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr4.verify(NULL, NULL, Concatenate, nullEntry); + expr4.verify(NULL, NULL_UNIQUE, Concatenate, nullUniqueEntry); expr4.verify(NULL, NOT_NULL, Concatenate, emptyListEntry); - expr4.verify(NULL_UNIQUE, NULL, Concatenate, emptyListEntry); - expr4.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, emptyListEntry); + expr4.verify(NULL_UNIQUE, NULL, Concatenate, nullEntry); + expr4.verify(NULL_UNIQUE, NULL_UNIQUE, Concatenate, nullUniqueEntry); expr4.verify(NULL_UNIQUE, NOT_NULL, Concatenate, emptyListEntry); - expr4.verify(NOT_NULL, NULL, Concatenate, emptyListEntry); - expr4.verify(NOT_NULL, NULL_UNIQUE, Concatenate, emptyListEntry); + expr4.verify(NOT_NULL, NULL, Concatenate, nullEntry); + expr4.verify(NOT_NULL, NULL_UNIQUE, Concatenate, nullUniqueEntry); expr4.verify(NOT_NULL, NOT_NULL, Concatenate, emptyListEntry); expr4.verify(NULL, NULL, FanOut, noEntry); expr4.verify(NULL, NULL_UNIQUE, FanOut, noEntry);