Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct the coefficient value of timestamp nanoseconds when writing to Ion binary #177

Merged
merged 10 commits into from
Jun 3, 2021
2 changes: 1 addition & 1 deletion ion/binarywriter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ func TestWriteBinaryTimestamp(t *testing.T) {
nowish, _ := NewTimestampFromStr("2019-08-04T18:15:43.863494+10:00", TimestampPrecisionNanosecond, TimezoneLocal)

testBinaryWriter(t, eval, func(w Writer) {
assert.NoError(t, w.WriteTimestamp(NewTimestamp(time.Time{}, TimestampPrecisionNanosecond, TimezoneUTC)))
assert.NoError(t, w.WriteTimestamp(NewTimestamp(time.Time{}, TimestampPrecisionSecond, TimezoneUTC)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

@byronlin13 byronlin13 Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the test binary data doesn't have nanoseconds. It was failing because calling NewTimestamp with TimestampPrecisionNanosecond will set fractional precision: 9.

assert.NoError(t, w.WriteTimestamp(nowish))
})
}
Expand Down
38 changes: 11 additions & 27 deletions ion/bitstream.go
Original file line number Diff line number Diff line change
Expand Up @@ -652,30 +652,12 @@ func (b *bitstream) ReadTimestamp() (Timestamp, error) {

// Check the fractional seconds part of the timestamp.
if length > 0 {
// First byte indicates number of precision units in fractional seconds.
fracSecsBytes, err := b.in.Peek(1)
nsecs, overflow, fractionPrecision, err = b.readNsecs(length)
if err != nil {
return Timestamp{}, err
}

nsecs, overflow, err = b.readNsecs(length)
if err != nil {
return Timestamp{}, err
}

if nsecs > 0 {
fractionPrecision = 9

// Adjust fractionPrecision for each trailing zero.
// ie. .123456000 should have 6 fractionPrecision instead of 9
ns := nsecs
for ns > 0 && (ns%10) == 0 {
ns /= 10
fractionPrecision--
}
precision = TimestampPrecisionNanosecond
} else if len(fracSecsBytes) > 0 && fracSecsBytes[0] > 0xC0 && (fracSecsBytes[0]^0xC0) > 0 {
fractionPrecision = fracSecsBytes[0] ^ 0xC0
if fractionPrecision > 0 {
precision = TimestampPrecisionNanosecond
}
}
Expand All @@ -692,32 +674,34 @@ func (b *bitstream) ReadTimestamp() (Timestamp, error) {
}

// ReadNsecs reads the fraction part of a timestamp and rounds to nanoseconds.
// This function returns the nanoseconds as an int, overflow as a bool, and an error
// This function returns the nanoseconds as an int, overflow as a bool, exponent as an uint8, and an error
// if there was a problem executing this function.
func (b *bitstream) readNsecs(length uint64) (int, bool, error) {
func (b *bitstream) readNsecs(length uint64) (int, bool, uint8, error) {
d, err := b.readDecimal(length)
if err != nil {
return 0, false, err
return 0, false, 0, err
}

nsec, err := d.ShiftL(9).trunc()
if err != nil || nsec < 0 || nsec > 999999999 {
msg := fmt.Sprintf("invalid timestamp fraction: %v", d)
return 0, false, &SyntaxError{msg, b.pos}
return 0, false, 0, &SyntaxError{msg, b.pos}
}

nsec, err = d.ShiftL(9).round()
if err != nil {
msg := fmt.Sprintf("invalid timestamp fraction: %v", d)
return 0, false, &SyntaxError{msg, b.pos}
return 0, false, 0, &SyntaxError{msg, b.pos}
}

exponent := uint8(d.scale)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment explaining that this cast should be safe because the max precision of 9 can fit inside uint8.

Copy link
Contributor Author

@byronlin13 byronlin13 Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not true because the Ion timestamp spec mentions:

Fractional seconds are allowed, with at least one digit of precision and an unlimited maximum.

Other Ion implementations can support arbitrary precision so Ion binary generated from Ion Java can have precision reduced.

Currently Ion Go support timestamps up to 9 precision units.

I created a ticket to update timestamps to support unlimited precision.


// Overflow to second.
if nsec == 1000000000 {
return 0, true, nil
return 0, true, exponent, nil
}

return int(nsec), false, nil
return int(nsec), false, exponent, nil
}

// ReadDecimal reads a decimal value of the given length: an exponent encoded as a
Expand Down
10 changes: 1 addition & 9 deletions ion/marshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"math/big"
"reflect"
"sort"
"strconv"
"time"
)

Expand Down Expand Up @@ -443,15 +442,8 @@ func (m *Encoder) encodeTimeDate(v reflect.Value) error {
kind = TimezoneUnspecified
}

// Get number of fractional seconds precisions
ns := t.Nanosecond()
numFractionalSeconds := 0
if ns > 0 {
numFractionalSeconds = len(strconv.Itoa(ns))
}

// Time.Date has nano second component
timestamp := NewTimestampWithFractionalSeconds(t, TimestampPrecisionNanosecond, kind, uint8(numFractionalSeconds))
timestamp := NewTimestampWithFractionalSeconds(t, TimestampPrecisionNanosecond, kind, maxFractionalPrecision)
return m.w.WriteTimestamp(timestamp)
}

Expand Down
19 changes: 16 additions & 3 deletions ion/marshal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@ func TestMarshalText(t *testing.T) {
test(MustParseDecimal("1.20"), "1.20")
test(NewTimestamp(time.Date(2010, 1, 1, 0, 0, 0, 0, time.UTC), TimestampPrecisionSecond, TimezoneUTC),
"2010-01-01T00:00:00Z")
test(time.Date(2010, 1, 1, 0, 0, 0, 770000000, time.UTC), "2010-01-01T00:00:00.77Z")
test(time.Date(2010, 1, 1, 0, 0, 0, 1, time.UTC), "2010-01-01T00:00:00.000000001Z")
test(time.Date(2010, 1, 1, 0, 0, 0, 770000000, time.UTC), "2010-01-01T00:00:00.770000000Z")
loc, _ := time.LoadLocation("EST")
test(time.Date(2010, 1, 1, 0, 0, 0, 0, loc), "2010-01-01T00:00:00-05:00")
test(time.Date(2010, 1, 1, 0, 0, 0, 0, loc), "2010-01-01T00:00:00.000000000-05:00")
loc = time.FixedZone("UTC+8", 8*60*60)
test(time.Date(2010, 1, 1, 0, 0, 0, 0, loc), "2010-01-01T00:00:00+08:00")
test(time.Date(2010, 1, 1, 0, 0, 0, 0, loc), "2010-01-01T00:00:00.000000000+08:00")

test("hello\tworld", "\"hello\\tworld\"")

Expand Down Expand Up @@ -138,6 +139,18 @@ func TestMarshalBinary(t *testing.T) {
0x8A, 0x21, 0x2A,
0x8B, 0x20,
}))

test(time.Date(2010, 1, 1, 0, 0, 0, 1, time.UTC), "time with 1 nanosecond", prefixIVM([]byte{
0x6A, 0x80, 0x0F, 0xDA, 0x81, 0x81, 0x80, 0x80, 0x80,
0xC9, // exponent: 9
0x01, // coefficient: 1
}))

test(time.Date(2010, 1, 1, 0, 0, 0, 5000, time.UTC), "time with 5000 nanoseconds", prefixIVM([]byte{
0x6B, 0x80, 0x0F, 0xDA, 0x81, 0x81, 0x80, 0x80, 0x80,
0xC9, // exponent: 9
0x13, 0x88, // coefficient: 5000
}))
}

func prefixIVM(data []byte) []byte {
Expand Down
17 changes: 8 additions & 9 deletions ion/textutils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,20 @@ func TestParseTimestamp(t *testing.T) {
test("1234-05-06T", "1234-05-06T00:00:00Z", TimestampPrecisionDay, TimezoneUnspecified, 0)
test("1234-05-06T07:08Z", "1234-05-06T07:08:00Z", TimestampPrecisionMinute, TimezoneUTC, 0)
test("1234-05-06T07:08:09Z", "1234-05-06T07:08:09Z", TimestampPrecisionSecond, TimezoneUTC, 0)
test("1234-05-06T07:08:09.100Z", "1234-05-06T07:08:09.100Z", TimestampPrecisionNanosecond, TimezoneUTC, 1)
test("1234-05-06T07:08:09.100100Z", "1234-05-06T07:08:09.100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 4)
test("1234-05-06T07:08:09.100Z", "1234-05-06T07:08:09.100Z", TimestampPrecisionNanosecond, TimezoneUTC, 3)
test("1234-05-06T07:08:09.100100Z", "1234-05-06T07:08:09.100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 6)

// Test rounding of >=9 fractional seconds.
test("1234-05-06T07:08:09.000100100Z", "1234-05-06T07:08:09.000100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 7)
test("1234-05-06T07:08:09.100100100Z", "1234-05-06T07:08:09.100100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 7)
test("1234-05-06T07:08:09.00010010044Z", "1234-05-06T07:08:09.000100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 7)
test("1234-05-06T07:08:09.00010010044Z", "1234-05-06T07:08:09.000100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 7)
test("1234-05-06T07:08:09.000100100Z", "1234-05-06T07:08:09.000100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 9)
test("1234-05-06T07:08:09.100100100Z", "1234-05-06T07:08:09.100100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 9)
test("1234-05-06T07:08:09.00010010044Z", "1234-05-06T07:08:09.000100100Z", TimestampPrecisionNanosecond, TimezoneUTC, 9)
test("1234-05-06T07:08:09.00010010055Z", "1234-05-06T07:08:09.000100101Z", TimestampPrecisionNanosecond, TimezoneUTC, 9)
test("1234-05-06T07:08:09.00010010099Z", "1234-05-06T07:08:09.000100101Z", TimestampPrecisionNanosecond, TimezoneUTC, 9)
test("1234-05-06T07:08:09.99999999999Z", "1234-05-06T07:08:10.000000000Z", TimestampPrecisionNanosecond, TimezoneUTC, 9)
test("1234-12-31T23:59:59.99999999999Z", "1235-01-01T00:00:00.000000000Z", TimestampPrecisionNanosecond, TimezoneUTC, 9)
test("1234-05-06T07:08:09.000100100+09:10", "1234-05-06T07:08:09.000100100+09:10", TimestampPrecisionNanosecond, TimezoneLocal, 7)
test("1234-05-06T07:08:09.100100100-10:11", "1234-05-06T07:08:09.100100100-10:11", TimestampPrecisionNanosecond, TimezoneLocal, 7)
test("1234-05-06T07:08:09.00010010044+09:10", "1234-05-06T07:08:09.000100100+09:10", TimestampPrecisionNanosecond, TimezoneLocal, 7)
test("1234-05-06T07:08:09.000100100+09:10", "1234-05-06T07:08:09.000100100+09:10", TimestampPrecisionNanosecond, TimezoneLocal, 9)
test("1234-05-06T07:08:09.100100100-10:11", "1234-05-06T07:08:09.100100100-10:11", TimestampPrecisionNanosecond, TimezoneLocal, 9)
test("1234-05-06T07:08:09.00010010044+09:10", "1234-05-06T07:08:09.000100100+09:10", TimestampPrecisionNanosecond, TimezoneLocal, 9)
test("1234-05-06T07:08:09.00010010055-10:11", "1234-05-06T07:08:09.000100101-10:11", TimestampPrecisionNanosecond, TimezoneLocal, 9)
test("1234-05-06T07:08:09.00010010099+09:10", "1234-05-06T07:08:09.000100101+09:10", TimestampPrecisionNanosecond, TimezoneLocal, 9)
test("1234-05-06T07:08:09.99999999999-10:11", "1234-05-06T07:08:10.000000000-10:11", TimestampPrecisionNanosecond, TimezoneLocal, 9)
Expand Down
79 changes: 47 additions & 32 deletions ion/timestamp.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ const (
TimestampPrecisionNanosecond
)

const maxFractionalPrecision = 9

func (tp TimestampPrecision) String() string {
switch tp {
case TimestampNoPrecision:
Expand Down Expand Up @@ -131,23 +133,34 @@ type Timestamp struct {

// NewDateTimestamp constructor meant for timestamps that only have a date portion (ie. no time portion).
func NewDateTimestamp(dateTime time.Time, precision TimestampPrecision) Timestamp {
return Timestamp{dateTime, precision, TimezoneUnspecified, 0}
numDecimalPlacesOfFractionalSeconds := uint8(0)
if precision >= TimestampPrecisionNanosecond {
numDecimalPlacesOfFractionalSeconds = maxFractionalPrecision
}
return Timestamp{dateTime, precision, TimezoneUnspecified, numDecimalPlacesOfFractionalSeconds}
}

// NewTimestamp constructor
func NewTimestamp(dateTime time.Time, precision TimestampPrecision, kind TimezoneKind) Timestamp {
numDecimalPlacesOfFractionalSeconds := uint8(0)

if precision <= TimestampPrecisionDay {
// Timestamps with Year, Month, or Day precision necessarily have TimezoneUnspecified timezone.
kind = TimezoneUnspecified
} else if precision >= TimestampPrecisionNanosecond {
numDecimalPlacesOfFractionalSeconds = maxFractionalPrecision
}
return Timestamp{dateTime, precision, kind, 0}
return Timestamp{dateTime, precision, kind, numDecimalPlacesOfFractionalSeconds}
}

// NewTimestampWithFractionalSeconds constructor
func NewTimestampWithFractionalSeconds(dateTime time.Time, precision TimestampPrecision, kind TimezoneKind, fractionPrecision uint8) Timestamp {
if fractionPrecision > 9 {
if fractionPrecision > maxFractionalPrecision {
// 9 is the max precision supported
fractionPrecision = 9
fractionPrecision = maxFractionalPrecision
}
if precision < TimestampPrecisionNanosecond {
fractionPrecision = 0
}
return Timestamp{dateTime, precision, kind, fractionPrecision}
}
Expand All @@ -159,30 +172,15 @@ func NewTimestampFromStr(dateStr string, precision TimestampPrecision, kind Time
if precision >= TimestampPrecisionNanosecond {
pointIdx := strings.LastIndex(dateStr, ".")
if pointIdx != -1 {
nonZeroFraction := false

idx := pointIdx + 1
for idx < len(dateStr) && isDigit(int(dateStr[idx])) {
if dateStr[idx] != '0' {
nonZeroFraction = true
}
fractionUnits++
idx++
}

if idx == len(dateStr) {
return Timestamp{}, fmt.Errorf("ion: invalid date string '%v'", dateStr)
}

// We do not want to include trailing zeros for a non-zero fraction (ie. .1234000 -> .1234)
// So we adjust fractionUnits accordingly.
if nonZeroFraction {
idx--
for idx > pointIdx && dateStr[idx] == '0' {
fractionUnits--
idx--
}
}
}
}

Expand Down Expand Up @@ -461,7 +459,7 @@ func (ts Timestamp) String() string {
// So we may need to make some adjustments.

// Add back removed trailing zeros from fractional seconds (ie. ".000")
if ts.precision >= TimestampPrecisionNanosecond && ts.dateTime.Nanosecond() == 0 && ts.numFractionalSeconds > 0 {
if ts.precision >= TimestampPrecisionNanosecond && ts.numFractionalSeconds > 0 {
// Find the position of 'T'
tIndex := strings.Index(format, "T")
if tIndex == -1 {
Expand All @@ -471,23 +469,39 @@ func (ts Timestamp) String() string {
}
}

index := strings.LastIndex(format, "Z")
if index == -1 || index < tIndex {
index = strings.LastIndex(format, "+")
if index == -1 || index < tIndex {
index = strings.LastIndex(format, "-")
timeZoneIndex := strings.LastIndex(format, "Z")
if timeZoneIndex == -1 || timeZoneIndex < tIndex {
timeZoneIndex = strings.LastIndex(format, "+")
if timeZoneIndex == -1 || timeZoneIndex < tIndex {
timeZoneIndex = strings.LastIndex(format, "-")
}
}

// This position better be right of 'T'
if index != -1 && tIndex < index {
if timeZoneIndex != -1 && tIndex < timeZoneIndex {
zeros := strings.Builder{}
zeros.WriteByte('.')
for i := uint8(0); i < ts.numFractionalSeconds; i++ {
numZerosNeeded := 0

// Specify trailing zeros if fractional precision is less than the nanoseconds.
// e.g. A timestamp: 2021-05-25T13:41:31.00001234 with fractional precision: 2 will return "2021-05-25 13:41:31.00"
ns := ts.dateTime.Nanosecond()
if ns == 0 || maxFractionalPrecision-len(strconv.Itoa(ns)) >= int(ts.numFractionalSeconds) {
zeros.WriteByte('.')
numZerosNeeded = int(ts.numFractionalSeconds)
} else {
decimalPlaceIndex := strings.LastIndex(format, ".")
if decimalPlaceIndex != -1 {
decimalPlacesOccupied := timeZoneIndex - decimalPlaceIndex - 1
numZerosNeeded = int(ts.numFractionalSeconds) - decimalPlacesOccupied
}
}

// Add trailing zeros until the fractional seconds component is the correct length
for i := 0; i < numZerosNeeded; i++ {
zeros.WriteByte('0')
}

format = format[0:index] + zeros.String() + format[index:]
format = format[0:timeZoneIndex] + zeros.String() + format[timeZoneIndex:]
}
}

Expand Down Expand Up @@ -515,12 +529,13 @@ func (ts Timestamp) Equal(ts1 Timestamp) bool {
ts.numFractionalSeconds == ts1.numFractionalSeconds
}

// TruncatedNanoseconds returns nanoseconds with trailing zeros removed (ie. 123456000 gets truncated to 123456).
// TruncatedNanoseconds returns nanoseconds with trailing values removed up to the difference of max fractional precision - time stamp's fractional precision
// e.g. 123456000 with fractional precision: 3 will get truncated to 123.
func (ts Timestamp) TruncatedNanoseconds() int {
nsecs := ts.dateTime.Nanosecond()
for i := uint8(0); i < (9-ts.numFractionalSeconds) && nsecs > 0 && (nsecs%10) == 0; i++ {

for i := uint8(0); i < (maxFractionalPrecision-ts.numFractionalSeconds) && nsecs > 0; i++ {
nsecs /= 10
}

return nsecs
}
Loading