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

[WIP] Add add support for Timestamp type #431

Closed
wants to merge 1 commit into from

Conversation

frsyuki
Copy link
Member

@frsyuki frsyuki commented Aug 10, 2017

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

@@ -32,6 +32,8 @@
import java.nio.charset.CharsetEncoder;
import java.nio.charset.CoderResult;
import java.nio.charset.CodingErrorAction;
import java.time.Instant;
Copy link
Contributor

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

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.

@xerial
Copy link
Member

xerial commented Oct 21, 2017

TODO:

  • Add timestamp MessageFormat types?
  • Add timestamp Value type

@mattbishop
Copy link

mattbishop commented Nov 27, 2017

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

@xerial
Copy link
Member

xerial commented Nov 29, 2017

@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.

return sec * 1000L + nsec / 1000L;
}
case 12: {
int nsec = readInt();
Copy link
Member

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

}
switch (ext.getLength()) {
case 4: {
int u32 = readInt();
Copy link
Member

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

return Instant.ofEpochSecond(sec, nsec);
}
case 12: {
int nsec = readInt();
Copy link
Member

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

@gustf
Copy link

gustf commented Apr 18, 2020

Hi there, any progress on this feature?

@xerial xerial mentioned this pull request May 11, 2021
8 tasks
@xerial
Copy link
Member

xerial commented May 12, 2021

Continuing the work at #565

@xerial
Copy link
Member

xerial commented May 12, 2021

Closing this PR in favor of #565

@xerial xerial closed this May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants