Adds support for writing binary Ion 1.1 timestamps by popematt · Pull Request #618 · amazon-ion/ion-java

popematt · 2023-10-19T20:50:49Z

Issue #, if available:

None

Description of changes:

Adds support for writing binary Ion 1.1 timestamps.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2023-10-19T20:59:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

popematt · 2023-10-25T18:45:25Z

src/com/amazon/ion/impl/bin/IonEncoder_1_1.java

-import com.amazon.ion.Decimal;
 import com.amazon.ion.IonType;
 import com.amazon.ion.Timestamp;
-import com.amazon.ion.impl.bin.utf8.Utf8StringEncoder;


These were unused imports.

popematt · 2023-10-25T19:28:12Z

test/com/amazon/ion/impl/bin/IonEncoder_1_1Test.java

            "false, 5F",
    })
-    public void testWriteBooleanValue(Boolean value, String expectedBytes) {
+    public void testWriteBooleanValue(boolean value, String expectedBytes) {


This is an incidental change. Then writeBooleanValue method accepts boolean, so I made the test do the same.

popematt · 2023-10-25T19:29:15Z

test/com/amazon/ion/impl/bin/IonEncoder_1_1Test.java

+    public void testWriteIntegerValueForBigInteger(BigInteger value, String expectedBytes) {
+        assertWritingValue(expectedBytes, value, IonEncoder_1_1::writeIntValue);


This is also an incidental change since I realized that JUnit already knows how to convert to BigInteger.

popematt · 2023-10-25T19:29:55Z

src/com/amazon/ion/impl/bin/WriteBuffer.java

    public int writeFixedUInt(final long value) {
        if (value < 0) {
-            throw new IllegalArgumentException("Attempted to write a FlexUInt for " + value);
+            throw new IllegalArgumentException("Attempted to write a FixedUInt for " + value);


Just fixing a typo that I noticed while working on Timestamps.

tgregg

I still need to double-check the tests for you, but leaving these comments here for now.

tgregg · 2023-10-30T22:30:00Z

src/com/amazon/ion/impl/bin/IonEncoder_1_1.java

+        }
+
+        // Condition 2: The fractional seconds are a common precision.
+        int secondsScale = value.getDecimalSecond().scale();


getDecimalSecond() creates a new BigDecimal on each call. Let's use getZFractionalSecond() instead. It's deprecated, but if we ever get around to removing it we'll replace it with some internal accessor. Note: this also requires a null check.

tgregg · 2023-10-30T22:33:03Z

src/com/amazon/ion/impl/bin/IonEncoder_1_1.java

+
+        // Condition 2: The fractional seconds are a common precision.
+        int secondsScale = value.getDecimalSecond().scale();
+        if (secondsScale != 0 && secondsScale != 3 && secondsScale != 6 && secondsScale != 9) {


Note: in my initial prototype I used secondsScale % 3 != 0 || secondsScale > 9. Comparing these alternatives, I think it's pretty much a wash. In the common case (that the scale matches one of the common precisions) my suggestion always requires two comparisons, but it also has the modulo. Yours varies depending on the actual precision. One comparison if the scale is 0, two if it's 3, and so on. Leaving this comment in case you feel like forming a strong opinion on which one is preferable.

I don't have a preference, and I suspect that it will not have very significant difference. I'll create an issue to revisit this later.

tgregg · 2023-11-01T00:10:15Z

src/com/amazon/ion/impl/bin/Ion_1_1_Constants.java

+    static final int L_TIMESTAMP_OFFSET_BIT_OFFSET = 34;
+    static final int L_TIMESTAMP_SECOND_BIT_OFFSET = 44;


This looks off. The offset is 12 bits, but here the second bit is only 10 after the offset bits.

Good catch.

tgregg · 2023-11-01T00:17:02Z

src/com/amazon/ion/impl/bin/IonEncoder_1_1.java

+
+            bits |= ((long) value.getSecond()) << S_U_TIMESTAMP_SECOND_BIT_OFFSET;
+
+            int secondsScale = value.getDecimalSecond().scale();


getZFractionalSecond() here too, and below

tgregg · 2023-11-01T00:20:14Z

src/com/amazon/ion/impl/bin/IonEncoder_1_1.java

+
+            int secondsScale = value.getDecimalSecond().scale();
+            if (secondsScale != 0) {
+                long fractionalSeconds = value.getDecimalSecond().remainder(BigDecimal.ONE).movePointRight(secondsScale).longValue();


I see value.unscaledValue().longValue() in my prototype. Maybe you can get that to work. Remember that any BigDecimal method that returns BigDecimal allocates a new one, so we want to minimize that.

tgregg · 2023-11-04T00:33:47Z

test/com/amazon/ion/impl/bin/IonEncoder_1_1Test.java

+    @ParameterizedTest
+    @CsvSource({
+            //                                    OpCode   MYYYYYYY DDDDDMMM mmmHHHHH ooooommm ssssssoo ffffffff ffffffff ffffffff ..ffffff
+            "2023-10-15T01:00-14:00,              01111000 00110101 01111101 00000001 01000000 00000010",


Should the offset bits all be zero for this one?

Why yes, it should. 😅
I've gone and manually double checked several more test cases with offsets both in the short form and the long form timestamps. I forgot about adjusting the offset for the short form, but the long form looks okay.

popematt requested a review from zslayton October 19, 2023 20:50

popematt mentioned this pull request Oct 20, 2023

Fix checkout step in regression workflow #621

Merged

popematt requested a review from tgregg October 20, 2023 20:48

popematt marked this pull request as draft October 23, 2023 18:31

popematt changed the base branch from master to ion-11-encoding October 25, 2023 18:43

popematt force-pushed the timestamp branch from 01bedaf to 52e428c Compare October 25, 2023 18:44

Adding support for timestamps

e222007

popematt force-pushed the timestamp branch from 52e428c to e222007 Compare October 25, 2023 19:26

popematt commented Oct 25, 2023

View reviewed changes

popematt marked this pull request as ready for review October 25, 2023 19:31

tgregg reviewed Nov 1, 2023

View reviewed changes

Adds suggested changes

54753cc

popematt mentioned this pull request Nov 2, 2023

See if speed of Ion 1.1 binary timestamp serialization can be improved #631

Open

Adds suggested changes

ea7cad8

tgregg approved these changes Nov 3, 2023

View reviewed changes

tgregg reviewed Nov 4, 2023

View reviewed changes

Fix short form timestamp offsets

7fce291

tgregg approved these changes Nov 8, 2023

View reviewed changes

popematt merged commit 9b59da2 into amazon-ion:ion-11-encoding Nov 8, 2023

popematt added a commit to popematt/ion-java that referenced this pull request Nov 16, 2023

Adds support for writing binary Ion 1.1 timestamps (amazon-ion#618)

4b6dbfa

popematt added a commit that referenced this pull request Nov 16, 2023

Adds support for writing binary Ion 1.1 timestamps (#618)

0a9ad1f

popematt deleted the timestamp branch November 21, 2023 00:53

tgregg mentioned this pull request Jan 18, 2024

Bumps version to 1.11.2-SNAPSHOT #701

Merged

tgregg mentioned this pull request Feb 9, 2024

Bumps version to 1.11.3-SNAPSHOT #720

Merged

tgregg mentioned this pull request Feb 22, 2024

Bumps version to 1.11.4-SNAPSHOT #753

Merged

tgregg mentioned this pull request Mar 1, 2024

Bumps version to 1.11.5-SNAPSHOT #760

Merged

This was referenced Apr 23, 2024

Bumps version to 1.11.6-SNAPSHOT #810

Closed

Bumps version to 1.11.8-SNAPSHOT. #815

Merged

		public void testWriteIntegerValueForBigInteger(BigInteger value, String expectedBytes) {
		assertWritingValue(expectedBytes, value, IonEncoder_1_1::writeIntValue);

		static final int L_TIMESTAMP_OFFSET_BIT_OFFSET = 34;
		static final int L_TIMESTAMP_SECOND_BIT_OFFSET = 44;


		bits \|= ((long) value.getSecond()) << S_U_TIMESTAMP_SECOND_BIT_OFFSET;

		int secondsScale = value.getDecimalSecond().scale();

Conversation

popematt commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgregg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

popematt commented Oct 19, 2023 •

edited

Loading

codecov bot commented Oct 19, 2023 •

edited

Loading