Use checked casts when reading vints as ints #2043

aweisberg · 2022-12-06T20:41:53Z

patch by Ariel Weisberg; reviewed by ? and ? for CASSANDRA-18099

dcapwell

+1 assuming we all agree on the name =). It sounds like readUnsignedVInt32, but I am cool with other names

src/java/org/apache/cassandra/cql3/functions/types/TypeCodec.java

aweisberg · 2022-12-06T21:15:49Z

So I think one thing to be aware of is that we aren't reading an unsigned int32. There is maybe zero code that handles unsigned int32s? We are casting to a signed int and the checked cast is for a signed ints range of values.

belliottsmith · 2022-12-06T21:41:30Z

For consistency with the existing API though the valid thing to do would be to simply check its populating at most 32 bits, and if the 32nd bit is set it’s a negative int that may (or may not) be interpreted by the caller as an unsigned value.

That said, we could instead introduce eg readNaturalInt32 which would expect at most 31bits to be set, which might be better.

aweisberg · 2022-12-12T17:24:17Z

OK, this looks like it is passing tests now https://app.circleci.com/pipelines/github/aweisberg/cassandra?branch=cassandra-18099-trunk

src/java/org/apache/cassandra/utils/vint/VIntCoding.java

src/java/org/apache/cassandra/cql3/functions/types/TypeCodec.java

src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java

src/java/org/apache/cassandra/db/partitions/PartitionUpdate.java

src/java/org/apache/cassandra/db/rows/EncodingStats.java

src/java/org/apache/cassandra/db/rows/UnfilteredSerializer.java

src/java/org/apache/cassandra/db/streaming/ComponentManifest.java

src/java/org/apache/cassandra/db/Columns.java

src/java/org/apache/cassandra/db/RowIndexEntry.java

src/java/org/apache/cassandra/utils/vint/VIntCoding.java

src/java/org/apache/cassandra/db/SerializationHeader.java

src/java/org/apache/cassandra/hints/Hint.java

src/java/org/apache/cassandra/hints/HintMessage.java

src/java/org/apache/cassandra/hints/HintsWriter.java

src/java/org/apache/cassandra/io/util/DataInputPlus.java

src/java/org/apache/cassandra/io/util/RebufferingInputStream.java

src/java/org/apache/cassandra/utils/vint/VIntCoding.java

test/unit/org/apache/cassandra/db/streaming/ComponentManifestTest.java

maedhroz

My feedback is all pretty minor around documentation, import order, etc.

Otherwise, LGTM

dcapwell

can we address the interface comment? Rest are nits

dcapwell · 2022-12-19T17:22:27Z

src/java/org/apache/cassandra/hints/HintsWriter.java

                Hint.serializer.serialize(hint, out, descriptor.messagingVersion());
+                long actualSize = out.position() - startPosition;
+                checkState(actualSize == hintSize);


can we have a useful error msg? If this triggers we don't have enough info on what happened

src/java/org/apache/cassandra/io/util/DataOutputPlus.java

dcapwell

would be great to improve the error message of hint, but will leave that as a nit and not a blocker.

+1

patch by Ariel Weisberg; reviewed by David Capwell and Caleb Rackliffe for CASSANDRA-18099

dcapwell approved these changes Dec 6, 2022

View reviewed changes

src/java/org/apache/cassandra/cql3/functions/types/TypeCodec.java Outdated Show resolved Hide resolved

aweisberg force-pushed the cassandra-18099-trunk branch from d77e4c4 to 8a3574a Compare December 6, 2022 21:12

aweisberg requested review from belliottsmith and dcapwell December 6, 2022 21:13

aweisberg force-pushed the cassandra-18099-trunk branch 2 times, most recently from a423bc6 to 11ee42d Compare December 6, 2022 21:38

aweisberg force-pushed the cassandra-18099-trunk branch 5 times, most recently from db79471 to 6016e68 Compare December 8, 2022 21:55