CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering #3702
Conversation
maedhroz
force-pushed
the
CASSANDRA-20100-trunk
branch
3 times, most recently
from
cf17000
to
f82ab98
Compare
| @@ -141,20 +141,24 @@ private IndexTermType(ColumnMetadata columnMetadata, List<ColumnMetadata> partit | |||
| this.indexTargetType = indexTargetType; | |||
| this.capabilities = calculateCapabilities(columnMetadata, partitionColumns, indexTargetType); | |||
| this.indexType = calculateIndexType(columnMetadata.type, capabilities, indexTargetType); | |||
| if (indexType.subTypes().isEmpty()) | |||
|
|
|||
| AbstractType<?> baseType = indexType.unwrap(); | |||
There was a problem hiding this comment.
This bit isn't part of the issue described in the Jira, but it's just...clearly wrong, so I cleaned it up.
| // Override comparisons for frozen collections and composite types (map entries) | ||
| else if (isLong() || isBigDecimal() || isBigInteger()) | ||
| return indexType.unwrap().compare(requestedValue.raw, columnValue.raw); | ||
| // Override comparisons for frozen collections and composite types (map entries) | ||
| else if (isComposite() || isFrozen()) | ||
| return FastByteOperations.compareUnsigned(requestedValue.raw, columnValue.raw); | ||
|
|
There was a problem hiding this comment.
The question I would have as a reviewer here is why, since we're just post-filtering, we don't unwrap on the next line, when we've fallen through the other cases. The bounds from Expression are used to generate queries for the memtable index, the on-disk indexes, and also to do post-filtering. Because of the first two, and the fact that we currently index reversed byte-comparable for most reversed types, we reverse the bounds on Expression as well. In those cases, post-filtering uses the reversed compare() to "undo" that. Ultimately, we should fix this, especially if we want to change the on-disk format and memtable-adjacent index to always use the base type byte-comparable, but it would be more invasive (i.e. require making changes to query generation for both of those if we don't change the disk format).
I think for now what I'd like to do is simply comment this next line to explain ^
There was a problem hiding this comment.
took a bit for my brain to process, and you dumping in slack... data on disk is backwards due to ReverseType reversing the bytes, so we need to live with this behavior which is a pain... :sigh:
| } | ||
|
|
||
| if (reversed) | ||
| capabilities.add(Capability.REVERSED); |
There was a problem hiding this comment.
These will now have isReversed() return false during Expression creation to align w/ the fact we don't actually index reversed byte-comparables for these types.
| @@ -26,7 +30,7 @@ | |||
| public class DescClusteringRangeQueryTest extends SAITester | |||
| { | |||
| @Test | |||
| public void testReverseAndBetweenMemtable() | |||
| public void testReversedIntBetween() throws Throwable | |||
There was a problem hiding this comment.
Notes: int behaves the same as all the other non-long primitive numerics, and UUID is EQ-only ATM
…query construction and post-filtering patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-20100
maedhroz
force-pushed
the
CASSANDRA-20100-trunk
branch
from
f82ab98
to
f5dc540
Compare
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-20100