Repository navigation
Fix failing BenchmarkCheck cases - #1576
Merged
TristonianJones merged 2 commits intoOct 22, 2026
Merged
TristonianJones merged 2 commits into
TristonianJones merged 2 commits into
Conversation
BenchmarkCheck builds its environment the way TestCheck does, but it ignored testEnv.jsonFieldNames, so the cases that set it reported undefined fields and failed. It now passes JSONFieldNames to both the registry and the checker, as TestCheck does.
Check rewrites the parsed AST in place, for example replacing a qualified select with an identifier, so iterations after the first checked an already rewritten AST. For `.y.z` the rewrite drops the leading dot, and the case failed whenever b.N was above 1. Each iteration now checks a copy of the parsed AST.
1 of 3 tasks
Collaborator
|
/gcbrun |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
TristonianJones
approved these changes
Oct 22, 2026
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
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.
I found these while measuring #1553. CI doesn't run benchmarks, so they went unnoticed. Since #1283, the three cases with
jsonFieldNames: truefail with "undefined field 'singleInt32'". Separately,[{'z': 0}].exists(y, .y.z == y.z)fails whenever b.N is above 1, becauseCheckrewrites the parsed AST in place and every iteration after the first checked the rewritten tree. With both commits, all 142 cases pass.Copying the AST happens inside the timed loop, alongside the printing and comparison the loop already does, so absolute numbers rise slightly.
cel.Env.Checkpasses itsAstto the checker without copying, so the same applies there: with a variabley.z, checking the parsed[{'z': 1}].exists(y, .y.z == y.z)a second time and callingAstToStringgives[{"z": 1}].exists(y, y.z == y.z), which compares the iteration variable with itself. Evaluation of the checked AST is unaffected; I'm happy to file an issue if you want this tracked.