You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maedhroz (via GitHub)" <gi...@apache.org> on 2024/11/21 01:41:03 UTC
[PR] CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
maedhroz opened a new pull request, #3702:
URL: https://github.com/apache/cassandra/pull/3702
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-20100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret inet, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #3702:
URL: https://github.com/apache/cassandra/pull/3702#issuecomment-2529493223
Committed as https://github.com/apache/cassandra/commit/86868c6df88b23af3941a087b9f6e7082065d6db
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3702:
URL: https://github.com/apache/cassandra/pull/3702#discussion_r1851274030
##########
test/unit/org/apache/cassandra/index/sai/cql/DescClusteringRangeQueryTest.java:
##########
@@ -26,7 +30,7 @@
public class DescClusteringRangeQueryTest extends SAITester
{
@Test
- public void testReverseAndBetweenMemtable()
+ public void testReversedIntBetween() throws Throwable
Review Comment:
Notes: `int` behaves the same as all the other non-long primitive numerics, and UUID is EQ-only ATM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret inet, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz closed pull request #3702: CASSANDRA-20100 Interpret inet, bigint, varint, and decimal as non-reversed types for query construction and post-filtering
URL: https://github.com/apache/cassandra/pull/3702
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3702:
URL: https://github.com/apache/cassandra/pull/3702#discussion_r1851272446
##########
src/java/org/apache/cassandra/index/sai/utils/IndexTermType.java:
##########
@@ -667,16 +672,31 @@ else if (!indexType.subTypes().isEmpty() && !indexType.isMultiCell())
capabilities.add(Capability.VECTOR);
if (indexType instanceof InetAddressType)
+ {
capabilities.add(Capability.INET_ADDRESS);
+ reversed = false;
+ }
if (indexType instanceof IntegerType)
+ {
capabilities.add(Capability.BIG_INTEGER);
+ reversed = false;
+ }
if (indexType instanceof DecimalType)
+ {
capabilities.add(Capability.BIG_DECIMAL);
+ reversed = false;
+ }
if (indexType instanceof LongType)
+ {
capabilities.add(Capability.LONG);
+ reversed = false;
+ }
+
+ if (reversed)
+ capabilities.add(Capability.REVERSED);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3702:
URL: https://github.com/apache/cassandra/pull/3702#discussion_r1851270113
##########
src/java/org/apache/cassandra/index/sai/utils/IndexTermType.java:
##########
@@ -491,7 +497,9 @@ public int comparePostFilter(Expression.Value requestedValue, Expression.Value c
{
if (isInetAddress())
return compareInet(requestedValue.encoded, columnValue.encoded);
- // 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);
Review 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 ^
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3702:
URL: https://github.com/apache/cassandra/pull/3702#discussion_r1851222256
##########
src/java/org/apache/cassandra/index/sai/utils/IndexTermType.java:
##########
@@ -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();
Review Comment:
This bit isn't part of the issue described in the Jira, but it's just...clearly wrong, so I cleaned it up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3702:
URL: https://github.com/apache/cassandra/pull/3702#discussion_r1851270113
##########
src/java/org/apache/cassandra/index/sai/utils/IndexTermType.java:
##########
@@ -491,7 +497,9 @@ public int comparePostFilter(Expression.Value requestedValue, Expression.Value c
{
if (isInetAddress())
return compareInet(requestedValue.encoded, columnValue.encoded);
- // 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);
Review 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 think _for now_ what I'd like to do is simply comment this next line to explain ^
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20100 Interpret int, bigint, varint, and decimal as non-reversed types for query construction and post-filtering [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3702:
URL: https://github.com/apache/cassandra/pull/3702#discussion_r1852968601
##########
src/java/org/apache/cassandra/index/sai/utils/IndexTermType.java:
##########
@@ -491,7 +497,9 @@ public int comparePostFilter(Expression.Value requestedValue, Expression.Value c
{
if (isInetAddress())
return compareInet(requestedValue.encoded, columnValue.encoded);
- // 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);
Review 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:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org