Skip to content

[WIP] Add add support for Timestamp type#431

Closed

frsyuki wants to merge 1 commit intodevelop from

msgpack-21-timestamp

Closed

[WIP] Add add support for Timestamp type #431
frsyuki wants to merge 1 commit intodevelop from
msgpack-21-timestamp

Conversation

Copy link

Member

@frsyuki frsyuki commented Aug 10, 2017 •

edited

Loading

Implementation is still working in progress.
Here is the spec of the new timestamp type: msgpack/msgpack#209


 Add add support for Timestamp type

eddf7c2

xuwei-k reviewed

View reviewed changes

msgpack-core/src/main/java/org/msgpack/core/MessagePacker.java

import java.nio.charset.CharsetEncoder;

import java.nio.charset.CoderResult;

import java.nio.charset.CodingErrorAction;

import java.time.Instant;

Copy link

Contributor

@xuwei-k xuwei-k Aug 14, 2017

Choose a reason for hiding this comment

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

drop Java 7 support ?

msgpack-java/.travis.yml

Lines 12 to 13 in 81d540d

- openjdk7

- oraclejdk7

Copy link

Member

@xerial xerial Oct 21, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

@frsyuki has confirmed it works in Java6 and Java7 even if Instant class is in the import statement. Only programs that use Instance related methods will fail to compile.

Copy link

Member

xerial commented Oct 21, 2017 •

edited

Loading

TODO:

Add timestamp MessageFormat types?
Add timestamp Value type

Copy link

mattbishop commented Nov 27, 2017 •

edited

Loading

How is the timestamp feature coming along? I know the msgpack-js project is getting close to a release: kawanet/msgpack-lite#76

Copy link

Member

xerial commented Nov 29, 2017 •

edited

Loading

@mattbishop We haven't decided how to add above two things; whether to add MessageFormat
type and ValueType for describing timestamp.

In the msgpack spec, timestamp is just one of the Extention types. We think adding these two additional types will be convenient for timestamp users. However, adding these types is a big change to the API (since most of the users is using switch case statement for checking data type, so rewriting will be necessary by adding case Timestamp: ...). We need to be careful so as not to break user programs that started to use timestamp types in future versions.

xerial reviewed

View reviewed changes

msgpack-core/src/main/java/org/msgpack/core/MessageUnpacker.java

return sec * 1000L + nsec / 1000L;

}

case 12: {

int nsec = readInt();

Copy link

Member

@xerial xerial May 27, 2018

Choose a reason for hiding this comment

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

@frsyuki nsec is uint32, so &xffffffffL mask is necessary to convert negative int32 to uint32

xerial reviewed

View reviewed changes

msgpack-core/src/main/java/org/msgpack/core/MessageUnpacker.java

}

switch (ext.getLength()) {

case 4: {

int u32 = readInt();

Copy link

Member

@xerial xerial May 27, 2018

Choose a reason for hiding this comment

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

@frsyuki Need to convert to uint32 by applying 0xFFFFFFFFL mask

xerial reviewed

View reviewed changes

msgpack-core/src/main/java/org/msgpack/core/MessageUnpacker.java

return Instant.ofEpochSecond(sec, nsec);

}

case 12: {

int nsec = readInt();

Copy link

Member

@xerial xerial May 27, 2018

Choose a reason for hiding this comment

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

This also needs to be converted to uint32 with 0xFFFFFFFFL mask

Copy link

gustf commented Apr 18, 2020

Hi there, any progress on this feature?

@xerial xerial mentioned this pull request

Timestamp support #565

Merged

8 tasks

Copy link

Member

xerial commented May 12, 2021 •

edited

Loading

Continuing the work at #565

Copy link

Member

xerial commented May 12, 2021

Closing this PR in favor of #565

@xerial xerial closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants

@frsyuki @xerial @mattbishop @gustf @xuwei-k