CNDB-13022: Fix analyzers for collections #1583
Conversation
Checklist before you submit for review
|
| return mapType.compose(foundValue).containsValue(mapType.getValuesType().compose(value)); | ||
| } | ||
| throw new AssertionError(); | ||
| return foundValue != null && Operator.CONTAINS.isSatisfiedBy(type, foundValue, value, indexAnalyzer, queryAnalyzer); |
There was a problem hiding this comment.
Test coverage complains we cover 3 of 4 conditions only
There was a problem hiding this comment.
Is there a reason we cannot do a similar logical simplification in the column.isComplex() case?
| * @param indexAnalyzer an index-provided function to transform the left-side compared value before comparison, | ||
| * it shouldn't be {@code null}. | ||
| */ | ||
| public boolean isSatisfiedBy(AbstractType<?> type, |
There was a problem hiding this comment.
This new method can be used in a future improvement patch to cache the tokens of the right, fixed part of the operation in the calling RowFilter.Expression. That way we wouldn't need to analyze that fixed part once and again for every evaluated row. I'm leaving that improvement out of this ticket so we can focus on fixing the correctness bug as soon as possible.
|
PR for CNDB: https://github.com/riptano/cndb/pull/13047 |
| List<ByteBuffer> rightTokens, | ||
| Index.Analyzer indexAnalyzer) | ||
| { | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
For the record - Test coverage complains we don't cover this.....
| { | ||
| assert column.type.isCollection(); | ||
| CollectionType<?> type = (CollectionType<?>)column.type; | ||
| assert (indexAnalyzer == null) == (queryAnalyzer == null); |
There was a problem hiding this comment.
This is the assertion test coverage complains about
There was a problem hiding this comment.
This is something that shouldn't happen given how the expressions are built (see here). I can only reproduce this by convoluted synthetic means which probably don't give too much value.
There was a problem hiding this comment.
In my opinion, the point of assertions is to test invariants that are not currently reachable but might become reachable in the future but that would break this code.
There was a problem hiding this comment.
In my opinion, the point of assertions is to test invariants that are not currently reachable but might become reachable in the future but that would break this code.
Exactly, as mentioned in Slack I marked them mostly for history.
If later someone wants to check the coverage for an old PR - they see only the percentage and they cannot get to the details in Sonar anymore.
|
Just flagged on the PR which are the conditions/linkes test coverage was complaining about |
In Apache trunk that was already quite well done last summer. It makes things nicely compact and easier to add on top of them. |
|
❌ Build ds-cassandra-pr-gate/PR-1583 rejected by Butler1 new test failure(s) in 3 builds Found 1 new test failures
Found 8 known test failures |
There was a problem hiding this comment.
CI seems fine to me. @michaeljmarshall you may want to check the Generic Order By failures, but they do not seem related to what we do here. More like flakies. They also do not fail for me locally.
+1
Fix `RowFilter` application of analyzers to queries on collections. `RowFilter.SimpleExpression#isSatisfiedBy` for `CONTAINS` and `CONTAINS_KEY` operators should extract the collection values and check them with `Operator.ANALYZER_MATCHES` rather than with equality.
Fix `RowFilter` application of analyzers to queries on collections. `RowFilter.SimpleExpression#isSatisfiedBy` for `CONTAINS` and `CONTAINS_KEY` operators should extract the collection values and check them with `Operator.ANALYZER_MATCHES` rather than with equality.
Fix
RowFilterapplication of analyzers to queries on collections.RowFilter.SimpleExpression#isSatisfiedByforCONTAINSandCONTAINS_KEYoperators should extract the collection values and check them withOperator.ANALYZER_MATCHESrather than with equality.