maedhroz commented on code in PR #2540:
URL: https://github.com/apache/cassandra/pull/2540#discussion_r1289100927
##########
test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java:
##########
@@ -360,22 +348,4 @@ private void compareToAndEqualsTests(PrimaryKey.Factory
factory, PrimaryKey... k
}
}
}
-
- private void byteComparisonTests(PrimaryKey.Factory factory, PrimaryKey...
keys)
- {
- for (int index = 0; index < keys.length - 1; index++)
- {
- PrimaryKey key = keys[index];
- PrimaryKey tokenOnlyKey = factory.createTokenOnly(key.token());
- assertByteComparison(tokenOnlyKey, key, -1);
- assertByteComparison(key, key, 0);
- assertByteComparison(tokenOnlyKey, tokenOnlyKey, 0);
-
- for (int comparisonIndex = index + 1; comparisonIndex <
keys.length; comparisonIndex++)
- {
- assertByteComparison(key, keys[comparisonIndex], -1);
- assertByteComparison(tokenOnlyKey, keys[comparisonIndex], -1);
- }
- }
- }
Review Comment:
> I have considered not using byte comparable at all for the primary keys
and this may be option for the future.
I'd wondered that reading this patch as well. Assuming it doesn't have any
real impact on compression ratio, that would be the way to go. Not opposed to
it being a separate issue, but you can also make a case we never would have
used byte-comparable stuff without the trie. To someone not familiar w/ the
evolution, it will look a bit odd :D
##########
test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java:
##########
@@ -360,22 +348,4 @@ private void compareToAndEqualsTests(PrimaryKey.Factory
factory, PrimaryKey... k
}
}
}
-
- private void byteComparisonTests(PrimaryKey.Factory factory, PrimaryKey...
keys)
- {
- for (int index = 0; index < keys.length - 1; index++)
- {
- PrimaryKey key = keys[index];
- PrimaryKey tokenOnlyKey = factory.createTokenOnly(key.token());
- assertByteComparison(tokenOnlyKey, key, -1);
- assertByteComparison(key, key, 0);
- assertByteComparison(tokenOnlyKey, tokenOnlyKey, 0);
-
- for (int comparisonIndex = index + 1; comparisonIndex <
keys.length; comparisonIndex++)
- {
- assertByteComparison(key, keys[comparisonIndex], -1);
- assertByteComparison(tokenOnlyKey, keys[comparisonIndex], -1);
- }
- }
- }
Review Comment:
> I have considered not using byte comparable at all for the primary keys
and this may be option for the future.
I'd wondered that reading this patch as well. Assuming it doesn't have any
real impact on compression ratio, that would be the way to go. Not opposed to
it being a separate issue, but you can also make a case we never would have
used byte-comparable stuff without the trie. To someone not familiar w/ the
evolution of the sorted terms stuff, it will look a bit odd :D
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
