Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/7.0] Stop skipping shadow skip navigation slots when creating the shadow values array #30911

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

ajcvickers
Copy link
Member

Port of #30902
Fixes #30764

Description

We were previously not allocating a slot for shadow skip navigations when values come from query, since they are always null. This is fine for the common case where the navigation values are at the end of the array. However, for a shadow navigation in an abstract base class, the slot may be in the middle of the array, which then means all our indexes are off by one (or more). To fix this, we now create slots for the shadow navigations, even though they will always contain null.

Customer impact

Crash on executing queries reported by multiple customers. Perhaps more importantly, could result in silently corrupted data in other cases.

How found

Customer reports 7.0

Regression

Yes. (Even though shadow navigations are new in 7.0, we were incorrectly handling some cases in 6.0 which meant that they accidentally worked.)

Testing

Added regression tests.

Risk

Low; simple fix. Also quirked.

@ajcvickers ajcvickers requested a review from a team May 17, 2023 10:28
@ajcvickers ajcvickers added this to the 7.0.x milestone May 17, 2023
@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.8 May 25, 2023
@wtgodbe wtgodbe merged commit 716bba0 into release/7.0 Jun 7, 2023
@wtgodbe wtgodbe deleted the OutsideBoundsCourse0516 branch June 7, 2023 19:59
@luizfbicalho
Copy link

Nice, are there plans of the release date of this version?
I would like to test it

@ajcvickers
Copy link
Member Author

@luizfbicalho This is merged for 7.0.8, which is scheduled for July.

@luizfbicalho
Copy link

@luizfbicalho This is merged for 7.0.8, which is scheduled for July.

So sad that it's too long, when it will be on daily build as preview?

@ajcvickers
Copy link
Member Author

@luizfbicalho It should already be in the daily builds for 8, since it was merged for 8 on May 17. There are no daily builds for 7.0.x patch releases.

@ajcvickers ajcvickers removed this from the 7.0.8 milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants