CASSANDRA-6936: Byte-comparable API #1294
Conversation
blambov
force-pushed
the
CASSANDRA-6936
branch
from
8933c89
to
527d548
Compare
blambov
force-pushed
the
CASSANDRA-6936
branch
2 times, most recently
from
b668eea
to
bf3b10b
Compare
src/java/org/apache/cassandra/utils/bytecomparable/ByteComparable.md
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/utils/bytecomparable/ByteComparable.md
Outdated
Show resolved
Hide resolved
| Because variable-length integers are also often used to store smaller range integers, it makes sense to also apply | ||
| the variable-length integer encoding. Thus, the current varint scheme chooses to: | ||
| - Strip any leading zeros. Note that for negative numbers, `BigInteger` encodes leading 0 as `0xFF`. | ||
| - Map numbers directly to their variable-length integer encoding, if they have 6 bytes or less. |
There was a problem hiding this comment.
@blambov I'm guessing the "variable-length integer encoding" here is the one we use for VariableLengthInteger, but it doesn't look like that's outlined in this document. Would it make sense to add that (more or less the JavaDoc for VariableLengthInteger) to this section?
There was a problem hiding this comment.
There's a section on this earlier. I'm adding a link to it.
| { | ||
| FastByteOperations.copy(src, srcPos, dst, dstPos, length); | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: These two methods appear to be unused?
There was a problem hiding this comment.
No idea why this is here. Removed.
| exponent. | ||
| - Output a byte encoding: | ||
| - the sign of the number encoded as `0x80` if positive and `0x00` if negative, | ||
| - the exponent length (stripping leading 0s) in bytes as `0x40 + exponent_length * exponent_sign`. |
There was a problem hiding this comment.
Reading the code makes it more clear that this first byte is sign bit inverted * 0x80 + 0x40 + signed exponent length, where exponent is negated if value is negative, but these two bullet points leave that a little ambiguous. Perhaps something like...
Output a byte encoding the
signed exponent+0x40+signed exponent length, where...
...thesigned exponentis0x80if positive or0x00if negative, and...
...signed exponent lengthis the length of the exponent in bytes after stripping leading 0s (negated if negative)
There was a problem hiding this comment.
Changed to use "modulated" instead of "signed" and went with "modulated exponent length" here. Let me know if it still looks unclear.
src/java/org/apache/cassandra/utils/bytecomparable/ByteSource.java
Outdated
Show resolved
Hide resolved
| * @param padding a padding byte (an int subject to a 0xFF mask) | ||
| * @return | ||
| */ | ||
| public static ByteSource cutOrRightPad(ByteSource src, int cutoff, int padding) |
There was a problem hiding this comment.
nit: Unused at the moment, but I'm guessing this is used somewhere in something not yet published?
There was a problem hiding this comment.
This one is for SAI.
|
|
||
| /** | ||
| * Escape value. Used, among other things, to mark the end of subcomponents (so that shorter compares before anything longer). | ||
| * Actual zeros in input need to be escaped if this is in use (see BufferReinterpreter). |
There was a problem hiding this comment.
| * Actual zeros in input need to be escaped if this is in use (see BufferReinterpreter). | |
| * Actual zeros in input need to be escaped if this is in use (see {@link BufferReinterpreter}). |
(...or perhaps AbstractReinterpreter would make more sense?)
| /** | ||
| * Reinterprets a byte buffer as a byte-comparable source that has 0s escaped and finishes in an escape. | ||
| * This provides a weakly-prefix-free byte-comparable version of the content to use in sequences. | ||
| * (See ByteSource.BufferReinterpreter/Multi for explanation.) |
There was a problem hiding this comment.
| * (See ByteSource.BufferReinterpreter/Multi for explanation.) | |
| * (See {@link AbstractReinterpreter} for a detailed explanation.) |
| int EXCLUDED = 0x18; | ||
|
|
||
| /** | ||
| * Reinterprets a byte buffer as a byte-comparable source that has 0s escaped and finishes in an escape. |
There was a problem hiding this comment.
nit: We only finish in an escape if version == LEGACY right?
|
|
||
| /** | ||
| * Variable-length encoding. Escapes 0s as ESCAPE + zero or more ESCAPED_0_CONT + ESCAPED_0_DONE. | ||
| * Finishes with an escape value (to which Multi will add non-zero component separator) |
There was a problem hiding this comment.
| * Finishes with an escape value (to which Multi will add non-zero component separator) | |
| * Finishes with an escape value (to which {@link Multi} will add non-zero component separator) |
| * Finishes with an escape value (to which Multi will add non-zero component separator) | ||
| * E.g. A00B translates to 4100FEFF4200 | ||
| * A0B0 4100FF4200FE (+00 for {@link Version#LEGACY}) | ||
| * A0 4100FE (+00 for {@link Version#LEGACY}) |
There was a problem hiding this comment.
nit: Would it be a little clearer to be explicit about AO being a string and the encoding as being hex?
| * A0 4100FE (+00 for {@link Version#LEGACY}) | |
| * "A0" 0x4100FE (+00 for {@link Version#LEGACY}) |
|
|
||
|
|
||
| /** | ||
| * Variable-length encoding. Escapes 0s as ESCAPE + zero or more ESCAPED_0_CONT + ESCAPED_0_DONE. |
There was a problem hiding this comment.
nit: Worth mentioning that there is no ESCAPED_0_DONE if the zero is at the end? (ex. "A0")
There was a problem hiding this comment.
Reworked paragraph.
| * prefix-free. Additionally, any such prefix sequence will compare smaller than the value to which it is a prefix, | ||
| * because any permitted separator byte will be smaller than the byte following the prefix. | ||
| */ | ||
| abstract static class AbstractReinterpreter implements ByteSource |
There was a problem hiding this comment.
nit: I'm noodling on renaming these XEscaper rather than XReinterpreter, given the former seems to more directly indicate what they do, but don't feel strongly about it.
| int EXCLUDED = 0x18; | ||
|
|
||
| /** | ||
| * Reinterprets a byte buffer as a byte-comparable source that has 0s escaped and finishes in an escape. |
There was a problem hiding this comment.
| * Reinterprets a byte buffer as a byte-comparable source that has 0s escaped and finishes in an escape. | |
| * Reinterprets byte-accessible data as a byte-comparable source that has 0s escaped and finishes in an escape. |
| * This provides a weakly-prefix-free byte-comparable version of the content to use in sequences. | ||
| * (See ByteSource.BufferReinterpreter/Multi for explanation.) | ||
| */ | ||
| static ByteSource of(long address, int length, ByteComparable.Version version) |
There was a problem hiding this comment.
nit: Along the lines of possibly renaming the XReinterpreter classes, perhaps this method could be called escaped() (at least until it does something more than that)?
There was a problem hiding this comment.
I kept is as of intentionally, to indicate that this is the correct way to encode a string/blob.
| /** | ||
| * Combines a chain of sources, turning their weak-prefix-free byte-comparable representation into the combination's | ||
| * prefix-free byte-comparable representation, with the included terminator character. | ||
| * For correctness, the terminator must be within MIN-MAX_SEPARATOR and different from NEXT_COMPONENT+/-1. |
There was a problem hiding this comment.
nit: Would it make sense to codify this as an assertion, or is that problematic because of how hot it is combined with our inability to disable assertions? :)
There was a problem hiding this comment.
Added assertions but had to rework some call-sites for the legacy uses.
| { | ||
| final ByteSource[] srcs; | ||
| int srcnum = -1; | ||
| int sequenceTerminator; |
There was a problem hiding this comment.
Done, also added private access here and elsewhere.
src/java/org/apache/cassandra/utils/bytecomparable/ByteSource.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/utils/bytecomparable/ByteSource.java
Outdated
Show resolved
Hide resolved
| assert i >= 0 && i <= 0xFF; | ||
| return new ByteSource() | ||
| { | ||
| boolean given = false; |
There was a problem hiding this comment.
nit: alternate name could be consumed
| }; | ||
| } | ||
|
|
||
| public static ByteSource cut(ByteSource src, int cutoff) |
There was a problem hiding this comment.
nit: alternate name could be trim(src, length)
There was a problem hiding this comment.
I prefer cut because of trim's association with removing whitespace.
src/java/org/apache/cassandra/utils/bytecomparable/ByteSource.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/utils/bytecomparable/ByteSource.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @param accessor Accessor to use to construct components. Because this will be used to construct individual | ||
| * arrays/buffers for each component, it may be sensible to use an accessor that allocates larger | ||
| * buffers in advance. |
There was a problem hiding this comment.
Not entirely sure what "use an accessor that allocates larger buffers in advance" means...there are only two ValueAccessor implementations, right? (Reuse larger buffers? Allocate up front for the whole bound?)
There was a problem hiding this comment.
I don't remember how this came to be, but I agree that there's no way to use a slab allocator here; removed this part of the comment.
| if (orderedBytes == null) | ||
| return null; | ||
|
|
||
| // First check for special cases (partition key only, static clustering) that can do without buffers. |
| byte[] keyBytes = ByteSourceInverse.getUnescapedBytes(ByteSourceInverse.nextComponentSource(peekable)); | ||
| // Consume the terminator byte. | ||
| int terminator = peekable.next(); | ||
| assert terminator == ByteSource.TERMINATOR; |
There was a problem hiding this comment.
nit: Worth a quick error message?
| { | ||
| return version -> | ||
| { | ||
| assert (version != Version.LEGACY); |
There was a problem hiding this comment.
nit: Perhaps worth a quick message about bound not being supported w/ the legacy format?
| return this.peer + 4; | ||
| } | ||
|
|
||
| @Override | ||
| public ByteBuffer getKey() | ||
| { | ||
| return MemoryUtil.getByteBuffer(peer + 4, MemoryUtil.getInt(peer), ByteOrder.BIG_ENDIAN); |
There was a problem hiding this comment.
nit: I guess we could use address() and length() here now.
| * | ||
| * See {@link #asComparableBytesLegacy} for description of the legacy format. | ||
| */ | ||
| public <V> ByteSource asComparableBytes(ValueAccessor<V> accessor, V data, ByteComparable.Version version) |
| |int MAX_VALUE |7F FF FF FF| FF FF FF FF | ||
| |long MIN_VALUE|80 00 00 00 00 00 00 00| 00 00 00 00 00 00 00 00 | ||
|
|
||
| ## Variable-length encoding of integers (current bigint) |
There was a problem hiding this comment.
Perhaps it would be helpful to mention that this corresponds to VariableLengthInteger? (maybe that's too low level when bigint is the corresponding CQL type)
There was a problem hiding this comment.
Yes, the level of description here is the abstract types and we don't go to specifics of the implementation classes. Keeping as is, unless you have a strong opinion on it.
| * 0 as 80 | ||
| * 1 as 81 | ||
| * 127 as C07F | ||
| * 255 as 80FF |
There was a problem hiding this comment.
Should this be C0FF? (i.e. 11000000 inverted sign bit, then a 1 for one remaining byte, which is FF)
| * 2^32-1 as F8FFFFFFFF | ||
| * 2^32 as F900000000 | ||
| * 2^56-1 as FEFFFFFFFFFFFFFF | ||
| * 2^56 as FF000100000000000000 |
There was a problem hiding this comment.
nit: Just a general note around the integer encoding docs...it might be helpful if we used a standard set of examples. It might make it a little easier for readers to jump between them. (ex. 2^32 is missing for VariableLengthInteger but present in both formats here.)
There was a problem hiding this comment.
The point is to highlight interesting transitions (the 2^56 ones) or differences between encodings (legacy vs this for 2^32). A standard list may have to include everything (i.e. be too long).
| int pos = -2; | ||
| ByteSource lengthEncoding = new VariableLengthUnsignedInteger(limit - startpos - FULL_FORM_THRESHOLD); | ||
|
|
||
| public int next() |
| return fromComparableBytesListOrSet(accessor, comparableBytes, version, getElementsType()); | ||
| } | ||
|
|
||
| static <V> ByteSource asComparableBytesListOrSet(AbstractType<?> elementsComparator, |
There was a problem hiding this comment.
nit: Perhaps this should live in CollectionType?
There was a problem hiding this comment.
Moved, together with fromComparableBytesListOrSet, compareListOrSet and setOrListToJsonString.
| long hiBits = ByteSourceInverse.getUnsignedFixedLengthAsLong(comparableBytes, 8); | ||
| long loBits = ByteSourceInverse.getUnsignedFixedLengthAsLong(comparableBytes, 8); | ||
|
|
||
| long version1 = hiBits >>> 60 & 0xF; |
There was a problem hiding this comment.
| long version1 = hiBits >>> 60 & 0xF; | |
| long uuidVersion = hiBits >>> 60 & 0xF; |
| @@ -196,6 +203,63 @@ static <VL, VR> int compareListOrSet(AbstractType<?> elementsComparator, VL left | |||
| return sizeL == sizeR ? 0 : (sizeL < sizeR ? -1 : 1); | |||
There was a problem hiding this comment.
| return sizeL == sizeR ? 0 : (sizeL < sizeR ? -1 : 1); | |
| return Integer.compare(sizeL, sizeR); |
| return ByteSource.withTerminatorMaybeLegacy(version, 0x00, srcs); | ||
| } | ||
|
|
||
| static <V> V fromComparableBytesListOrSet(ValueAccessor<V> accessor, |
There was a problem hiding this comment.
nit: Perhaps this should live in CollectionType?
|
|
||
| List<V> buffers = new ArrayList<>(); | ||
| int terminator = version == Version.LEGACY | ||
| ? 0x00 |
There was a problem hiding this comment.
nit: Perhaps extract a LEGACY_TERMINATOR = 0x00 to use here and above in asComparableBytesListOrSet()?
There was a problem hiding this comment.
Same in asComparableBytesMap()
There was a problem hiding this comment.
Listing it in the constants would validate it; I prefer not to do that because using 00 as a terminator is incorrect.
On the other hand, this shouldn't support the legacy encoding (AFAICS we can get a valid value being decoded incorrectly), removed this choice and added an assertion.
| @@ -218,6 +221,72 @@ public static <TL, TR> int compareMaps(AbstractType<?> keysComparator, AbstractT | |||
| return sizeL == sizeR ? 0 : (sizeL < sizeR ? -1 : 1); | |||
There was a problem hiding this comment.
| return sizeL == sizeR ? 0 : (sizeL < sizeR ? -1 : 1); | |
| return Integer.compare(sizeL, sizeR); |
| @@ -389,4 +470,27 @@ public static <V> V build(ValueAccessor<V> accessor, boolean isStatic, V... valu | |||
| out.flip(); | |||
| return accessor.valueOf(out); | |||
There was a problem hiding this comment.
nit: Would it be possible to just replace this w/ return build(accessor, isStatic, values, (byte) 0); now?
| srcs[i * 2 + 2] = ByteSource.oneByte(lastEoc & 0xFF ^ 0x80); // end-of-component also takes part in comparison as signed byte | ||
| ++i; | ||
| } | ||
| if (i * 2 + 1 < srcs.length) |
There was a problem hiding this comment.
nit: Might be worth a quick comment mentioning this can happen when there aren't values for all the types?
| } | ||
| } | ||
|
|
||
| public <V> ByteSource asComparableBytesLegacy(ValueAccessor<V> accessor, V data) |
| return ByteSource.withTerminatorLegacy(ByteSource.END_OF_STREAM, srcs); | ||
| } | ||
|
|
||
| public <V> ByteSource asComparableBytesNew(ValueAccessor<V> accessor, V data, ByteComparable.Version version) |
| } | ||
| // consume terminator | ||
| int terminator = comparableBytes.next(); | ||
| assert terminator == ByteSource.TERMINATOR; |
There was a problem hiding this comment.
nit: short message indicating this should have been the terminator
| * We store: | ||
| * - sign bit inverted * 0x80 + 0x40 + signed exponent length, where exponent is negated if value is negative | ||
| * - zero or more exponent bytes (as given by length) | ||
| * - 0x80 + first pair of decimal digits, negative is value is negative, rounded to -inf |
There was a problem hiding this comment.
| * - 0x80 + first pair of decimal digits, negative is value is negative, rounded to -inf | |
| * - 0x80 + first pair of decimal digits, negative if value is negative, rounded to -inf |
|
Any target date when this will be merged? Any planned target release? |
Provides an API for converting all values of types that can be used in primary keys to byte sequences that can be compared lexicographically by unsigned byte value (i.e. byte-comparable sequences) and back. patch by Branimir Lambov, Dimitar Dimitrov and Jacek Lewandowski; reviewed by Caleb Rackliffe, Dimitar Dimitrov, Jacek Lewandowski and Aleksey Yevchenko for CASSANDRA-6936
|
I'm planning to commit this today. |
cc @EricBorczuk |
Provides an API for converting all values of types that can be used in
primary keys to byte sequences that can be compared lexicographically
by unsigned byte value (i.e. byte-comparable sequences) and back.