Fix the NULL result of field() key expressions with fan-type Concatenate#4130
Open
RobertBrunel wants to merge 1 commit intoFoundationDB:mainfrom
Open
Fix the NULL result of field() key expressions with fan-type Concatenate#4130RobertBrunel wants to merge 1 commit intoFoundationDB:mainfrom
field() key expressions with fan-type Concatenate#4130RobertBrunel wants to merge 1 commit intoFoundationDB:mainfrom
Conversation
…tenate` 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 FoundationDB#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 FoundationDB#4129.
RobertBrunel
added
bug fix
RobertBrunel
changed the title
field() key expressions with fan-type `Conca…field() key expressions with fan-type Concatenate
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This change makes
FieldKeyExpression#getNullResult()propagate the null stand-in for fan-typeConcatenateinstead of returning an empty list. This alignsConcatenatewith theNonebranch, as the null stand-in is more appropriate behavior for this case. The empty list is only correct when the stand-in isNOT_NULL, since the empty list is 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 thescalar(NULL)going forward. We expect the impact to be limited, asConcatenateis not commonly used, andFieldKeyExpression#getNullResult()is reached only when the input message passed toevaluateMessage()is null. This, in particular, happens when anest()expression descends into an absent parent. In other words, the Concatenate field has to sit inside anest()expression under an optional parent and that parent submessage is absent.Testing:
KeyExpressionTest.Resolves #4129.