[release/7.0] Fix for #30266 and #30565 - projecting json entity/reference as well as regular entity collection - #30600
Merged
Merged
Conversation
…as regular entity collection #30266 Problem was that when we projection collection of entities we use result coordinator and generate different pattern in shaper code. Json entity shaper code is generated inside `resultContext.Values == null` block and all the products should be referred to via resultContext.Values array access. We were not doing that for json, so in the final projection code we would refer to parameters (storing the actual json entities) that were out of scope. Fix is to use the correct result coordinator pattern, just like we do for non-json entities. #30565 When we project entity collection, the shaper uses result coordinator and generates different shaper. However we process this scenario as if "regular" code path was being executed. Basically, we would generate code for collection projection outside of `resultContext.Values == null` block (this needs to happen for regular collections due to result coordination). For JSON it's not needed, because entire object is loaded from a single row. So we can move the processing into the block and then just refer to it's product in the final projection. We also need to shuffle around the order in which we build the block. We should build expressions and json entities first, then populate resultContext and then build includes (which depend on values in the context). Before we were populating result values before we generated json entities, (which didn't matter because we were not using that info) but now is necessary. Fixes #30266 Fixes #30565
maumar
force-pushed
the
fix_30266_30565_70
branch
from
April 3, 2023 20:59
869305d to
432bed3
Compare
Member
|
@roji I can merge this once you've approved it (CC @ajcvickers in case somebody else needs to review it) |
Member
|
Will look into the pending patch PRs tomorrow! |
roji
approved these changes
Apr 5, 2023
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
6 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.
Port of #30566
Port of #30568
Fixes #30266
Fixes #30565
Description
Projecting JSON entity or JSON collection combined with projecting of regular entity collection fails (with unhelpful message) during compile time. Workaround exists but it's not very discoverable and requires query rewrite (project entire entity and bring back the requested collection by applying include beforehand).
Customer impact
Queries with affected entities throw during compilation.
How found
Multiple customer reports on 7.0
Regression
No. JSON support is new functionality in 7.0.
Testing
Added regression tests for affected scenarios.
Risk
Low: While scenario is quite common, the fix is isolated to only affecting the scenario (i.e. when entity collection is projected along with json entity/collection). Moreover, fix is to apply the same logic that we use for non-JSON entity scenarios, so we have good degree of confidence that the logic is sound.
Those two issues are very closely related. Initially we thought that fix for #30565 would be significantly more risky/complex, so we split the bugs apart, but we were able to find a better fix that is only marginally more risky than #30266 and the resulting risk for combined issue is still low.
Added quirk to revert to old behavior if necessary.