fix: correct inconsistent runtime typing for logicalVectorEvaluator
#5160
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
f84f478 to
55573a8
Compare
onelson
commented
Sep 1, 2022
Comment on lines
+478
to
+480
| (r) => { | ||
| return {x: (if r:{#O with a: v[bool], b: v[bool], x: v[#K], y: v[#K]}.a:v[bool] and:bool r:{#O with a: v[bool], b: v[bool], x: v[#K], y: v[#K]}.b:v[bool] then r:{#O with a: v[bool], b: v[bool], x: v[#K], y: v[#K]}.x:v[#K] else r:{#O with a: v[bool], b: v[bool], x: v[#K], y: v[#K]}.y:v[#K]):v[#G]}:{x: v[#G]} | ||
| }:(r: {#O with a: v[bool], b: v[bool], x: v[#K], y: v[#K]}) => {x: v[#G]}"##]].assert_eq(&crate::semantic::formatter::format_node( |
There was a problem hiding this comment.
The typing for the logical and conditional exprs here on the rust side look good. The runtime doesn't appear to rely on this info in this case, it seems.
onelson
commented
Sep 1, 2022
Comment on lines
+250
to
+254
| // Using a logical expr in the test of the conditional expr produced a | ||
| // runtime error: | ||
| // ``` | ||
| // cannot use test of type vector in conditional expression; expected boolean | ||
| // ``` |
There was a problem hiding this comment.
In theory, I think we might have seen this for any nested expression where logical ops were children of a larger expression. I'm trying to think of why we didn't see this in the test suite earlier...
added 2 commits
September 1, 2022 11:37
When a logical op appears in the `test` position for a conditional, we see: ``` cannot use test of type vector in conditional expression; expected boolean ``` The runtime typing for `logicalVectorEvaluator` was bool rather than a vector of bool which would cause the conditional evaluator to incorrectly select the non-vectorized implementation (it expects test, consequent and alternate to all be vectors in order to use the vectorized version). By updating the `Type()` method for `logicalVectorEvaluator` to be a vector of bool, the `conditionalVectorEvaluator` is able to be used.
55573a8 to
2e4f4ec
Compare
logicalVectorEvaluator
nathanielc
approved these changes
Sep 1, 2022
5 tasks
onelson
pushed a commit
that referenced
this pull request
Sep 8, 2022
* test: vectorized nested logical ops This is mostly just to check for regressions (the fix was already made in #5160). Prior to the fix in #5160, this test would fail with: ``` runtime error: cannot use operand of type vector with logical or; expected boolean ``` Today, the test passes and all is well. * chore: make generate
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.
Fixes #5155
...which shows a runtime error when nesting a logical expression inside a conditional expression.
When in the
testposition, we'd get:Originally,
logicalVectorEvaluatoradvertised a runtime type of bool, which is inconsistent with reality since it is actually a vector of bool.The error message we saw in this situation stemmed from the fact we choose either the vectorized or row-based conditional evaluator with this check here:
flux/compiler/compiler.go
Lines 657 to 669 in df2b4c4
Since
testin this case was alogicalVectorEvaluatorwith incorrect (bool) typing, this check failed and we fall back to the non-vectorized version for conditional.This is why the error message states "expected boolean" -- we were calling the non-vectorized version with a vector as input.
This diff updates the impl for
logicalVectorEvaluator.Type()such that it reports a vector of bool instead of bool.Checklist
Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.
experimental/N/Adocs/Spec.mdhas been updated N/ADear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.