fix: skip strict null logicals #5221
Merged
Conversation
This file contains 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
scbrickley
approved these changes
Sep 23, 2022
|
|
added 2 commits
September 23, 2022 01:08
Looking at a perf regression...
35cb410 to
246a5dd
Compare
|
Wasn't sure it was polite to say the EAR out here. |
onelson
pushed a commit
that referenced
this pull request
Sep 28, 2022
A new version of our logical evaluator was introduced in an effort to re-align the lang with the spec regarding nulls. Ref: <#5192> The "strict null" flavor of the logical evaluator was bypassed as part of a perf regression investigation. Ref: <#5221> The thought was checking the feature flag with each eval was adding overhead and raising query latency, but with a susequent refactor (<#5222>) we are now able to check the flag earlier, reducing the overhead.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Not yet sure this is a problem or not, but the thinking is the feature flag check we do inside the new strict null logical evaluator is too expensive to tolerate.
Selecting the original evaluator will bypass the feature flag check so we can watch the perf impact and learn if it is truly too expensive 😓
Testcases that depend on the (newer) strict null version have been skipped to allow the CI to pass.
Checklist
Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.
experimental/docs/Spec.mdhas been updatedDear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.