Adds support for all scalar types to IonRawBinaryWriter_1_1 #667
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## ion-11-encoding #667 +/- ##
==================================================
Coverage ? 64.77%
Complexity ? 5732
==================================================
Files ? 199
Lines ? 24720
Branches ? 4392
==================================================
Hits ? 16013
Misses ? 7408
Partials ? 1299 ☔ View full report in Codecov by Sentry. |
| fun writeInt(value: Byte) | ||
| fun writeInt(value: Int) |
There was a problem hiding this comment.
🗺️ The implementations of these ended up just casting value to a Long and then calling the existing implementation for writing a Long. If we want to have optimized implementations for smaller integer sizes, we can add that later.
| fun writeInt(value: Long) | ||
| fun writeInt(value: BigInteger) | ||
|
|
||
| fun writeFloat(value: Float) | ||
| fun writeFloat(value: Double) | ||
|
|
||
| fun writeDecimal(value: BigDecimal) | ||
| fun writeDecimal(value: Decimal) |
There was a problem hiding this comment.
🗺️ I realized that this is redundant because Decimal extends BigDecimal.
| @@ -129,7 +128,7 @@ public static int writeIntValue(WriteBuffer buffer, final BigInteger value) { | |||
| * Writes a float to the given WriteBuffer using the Ion 1.1 encoding for Ion Floats. | |||
| * @return the number of bytes written | |||
| */ | |||
| public static int writeFloat(WriteBuffer buffer, final float value) { | |||
| public static int writeFloatValue(WriteBuffer buffer, final float value) { | |||
There was a problem hiding this comment.
🗺️ This was renamed to write___Value to be consistent with the other methods in this class.
| } else if (numCoefficientBytes == 1){ | ||
| buffer.writeByte((byte) 0); | ||
| } else { | ||
| throw new IllegalStateException("Unreachable! coefficientBytes should not be null when numCoefficientBytes > 1"); |
There was a problem hiding this comment.
🗺️ So, this is kind of interesting. I had already determined that this should be unreachable (hence the message in the exception), but IntelliJ deduced that it is unreachable, which means it must be provably unreachable... so I removed it.
| */ | ||
| public static int writeSymbolValue(WriteBuffer buffer, long value) { | ||
| public static int writeSymbolValue(WriteBuffer buffer, int value) { |
There was a problem hiding this comment.
🗺️ Since Java standard library collections track their size using an int, SIDs are effectively limited to those that can be represented in an int.
|
|
||
| currentContainer.length += buffer.writeFlexSym(utf8StringEncoder.encode(text.toString())) | ||
| hasFieldName = true | ||
| } | ||
|
|
||
| private fun switchToFlexSym() { | ||
| private fun switchCurrentStructToFlexSym() { |
There was a problem hiding this comment.
🗺️ Now that I've had a bit of time from writing this function, when I came back here and saw it, I thought the name was a bit too vague.
| override fun writeTimestamp(value: Timestamp) { | ||
| TODO("Not yet implemented") | ||
| } | ||
| override fun writeTimestamp(value: Timestamp) = writeScalar { writeTimestampValue(buffer, value) } | ||
|
|
||
| override fun writeTimestamp(value: Instant) { | ||
| TODO("Not yet implemented") |
There was a problem hiding this comment.
🗺️ I started to implement this, but I realized how expensive it is to convert an Instant into its year, month, day, etc., and so I don't think it offers much benefit over just converting the Instant to a Timestamp. As long as there's no objection, I think I'll just remove it.
It might be useful to have streamlined function for LocalDate and it's ilk at some point in the future.
| @@ -32,6 +32,7 @@ class FlexIntTest { | |||
| " 1048576, 00001000 00000000 00000000 00000001", | |||
| " 134217727, 11111000 11111111 11111111 01111111", | |||
| " 134217728, 00010000 00000000 00000000 00000000 00000001", | |||
| " ${Int.MAX_VALUE}, 11110000 11111111 11111111 11111111 00001111", | |||
There was a problem hiding this comment.
🗺️ This got added just because I was asking myself what is the FlexInt representation of Int.MAX_VALUE, so now it's here for reference.
| val commentRegex = Regex("\\|.*$") | ||
| val excessWhitespaceRegex = Regex("\\s+") | ||
| val cleanedHexBytes: String = hexBytes.lines() | ||
| .map { it.replace(commentRegex, "").trim() } | ||
| .filter { it.isNotBlank() } | ||
| .joinToString(" ") | ||
| .replace(excessWhitespaceRegex, " ") | ||
| .uppercase() | ||
| .trim() |
There was a problem hiding this comment.
🗺️ I had to fix this up a bit so that it could correctly handle lines that have comments but no hex pairs. E.g.
C2 | {
03 5E | $1: true
| }
| @@ -798,4 +808,212 @@ class IonRawBinaryWriterTest_1_1 { | |||
| writeBool(false) | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| fun `write int`() { | |||
There was a problem hiding this comment.
🗺️ For all of these scalar types, I'm relying on the tests for IonEncoder_1_1 to ensure edge cases are covered since the implementations in IonRawBinaryWriter_1_1 are basically pass through methods.
tgregg
changed the title
Issue #, if available:
None
Description of changes:
Adds support for ints, floats, decimals, timestamps, symbols, strings, blobs, and clobs.
See 🗺️ comments for some specific details.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.