CNDB-13022: Fix analyzers for collections #1583
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1260,22 +1260,28 @@ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey partitionKey, | |
| private boolean contains(TableMetadata metadata, DecoratedKey partitionKey, Row row) | ||
| { | ||
| assert column.type.isCollection(); | ||
| 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.
Exactly, as mentioned in Slack I marked them mostly for history. |
||
|
|
||
| CollectionType<?> type = (CollectionType<?>) column.type; | ||
| List<ByteBuffer> analyzedValues = queryAnalyzer == null ? null : queryAnalyzer.analyze(value); | ||
|
|
||
| if (column.isComplex()) | ||
| { | ||
| ComplexColumnData complexData = row.getComplexColumnData(column); | ||
| if (complexData != null) | ||
| { | ||
| AbstractType<?> elementType = type.kind == CollectionType.Kind.SET ? type.nameComparator() : type.valueComparator(); | ||
| for (Cell<?> cell : complexData) | ||
| { | ||
| ByteBuffer elementValue = type.kind == CollectionType.Kind.SET ? cell.path().get(0) : cell.buffer(); | ||
| if (analyzedValues == null) | ||
| { | ||
| if (elementType.compare(elementValue, value) == 0) | ||
| return true; | ||
| } | ||
| else | ||
| { | ||
| if (Operator.ANALYZER_MATCHES.isSatisfiedBy(elementType, elementValue, analyzedValues, indexAnalyzer)) | ||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -1285,37 +1291,35 @@ private boolean contains(TableMetadata metadata, DecoratedKey partitionKey, Row | |
| else | ||
| { | ||
| ByteBuffer foundValue = getValue(metadata, partitionKey, row); | ||
| 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 |
||
| } | ||
| } | ||
|
|
||
| private boolean containsKey(TableMetadata metadata, DecoratedKey partitionKey, Row row) | ||
| { | ||
| assert column.type.isCollection() && column.type instanceof MapType; | ||
| MapType<?, ?> mapType = (MapType<?, ?>) column.type; | ||
| if (column.isComplex()) | ||
| { | ||
| if (queryAnalyzer != null) | ||
| { | ||
| assert indexAnalyzer != null; | ||
| List<ByteBuffer> values = queryAnalyzer.analyze(value); | ||
| for (Cell<?> cell : row.getComplexColumnData(column)) | ||
| { | ||
| AbstractType<?> elementType = mapType.nameComparator(); | ||
| ByteBuffer elementValue = cell.path().get(0); | ||
| if (Operator.ANALYZER_MATCHES.isSatisfiedBy(elementType, elementValue, values, indexAnalyzer)) | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| return row.getCell(column, CellPath.create(value)) != null; | ||
| } | ||
| else | ||
| { | ||
| ByteBuffer foundValue = getValue(metadata, partitionKey, row); | ||
| return foundValue != null && Operator.CONTAINS_KEY.isSatisfiedBy(mapType, foundValue, value, indexAnalyzer, queryAnalyzer); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.