ClojureScript

Instance Reader to Support Micro/Nanoseconds

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    N/A

Description

Any timestamps with a greater than millisecond precision cannot be handled by the ClouseScript reader:

> cljs.reader.read_date("2012-12-30T23:20:05.066980000-00:00")
> Error: Assert failed: timestamp millisecond field must be in range 0..999 Failed:  0<=66980000<=999 (<= low n high)

Here "2012-12-30T23:20:05.066980000-00:00" is an example of an ordinary timestamp that is returned from Postgres.

ClojureScript reader interprets the nanosecond portion "066980000" as milliseconds and the check here fails:

def parse-and-validate-timestamp
...
   (check 0 ms 999 "timestamp millisecond field must be in range 0..999")
  1. CLJS-454.diff
    31/Jul/13 4:21 AM
    6 kB
    Jonas Enlund
  2. CLJS-454-patch2.diff
    02/Aug/13 3:16 PM
    7 kB
    Jonas Enlund
  3. cljs-reader-nanosecond-workarround-corrected.patch
    05/Feb/13 3:37 PM
    1 kB
    Thomas Heller

Activity

Hide
David Nolen added a comment -

Is this behavior supported in Clojure?

Show
David Nolen added a comment - Is this behavior supported in Clojure?
Hide
Jozef Wagner added a comment -

It seems it is

Show
Jozef Wagner added a comment - It seems it is
Hide
Anatoly Polinsky added a comment -

@David,

Yep, it is supported. Enhancing Jozef's response:

user=> (use 'clojure.instant)
nil
user=> (read-instant-date "2012-12-30T23:20:05.066980000-00:00")
#inst "2012-12-30T23:20:05.066-00:00"

/Anatoly

Show
Anatoly Polinsky added a comment - @David, Yep, it is supported. Enhancing Jozef's response:
user=> (use 'clojure.instant)
nil
user=> (read-instant-date "2012-12-30T23:20:05.066980000-00:00")
#inst "2012-12-30T23:20:05.066-00:00"
/Anatoly
Hide
David Nolen added a comment -

Ok, do you all have a good idea about what a patch would look like?

Show
David Nolen added a comment - Ok, do you all have a good idea about what a patch would look like?
Hide
Thomas Heller added a comment -

Since JavaScript has no support for nanoseconds in Date, I'd vote for dropping the nanoseconds. Currently the reader just blows up when reading a String with java.sql.Timestamp printed in CLJ, since that is printed in nanosecond precision.

While probably not the best solution, I attached a patch for cljs.reader/parse-and-validate-timestamp fn that just drops the nanoseconds from the millisecond portion. Would be easier to just strip the ms string to 3 digits but that would cause "2012-12-30T23:20:05.066980000012312312123121-00:00" to validate.

Show
Thomas Heller added a comment - Since JavaScript has no support for nanoseconds in Date, I'd vote for dropping the nanoseconds. Currently the reader just blows up when reading a String with java.sql.Timestamp printed in CLJ, since that is printed in nanosecond precision. While probably not the best solution, I attached a patch for cljs.reader/parse-and-validate-timestamp fn that just drops the nanoseconds from the millisecond portion. Would be easier to just strip the ms string to 3 digits but that would cause "2012-12-30T23:20:05.066980000012312312123121-00:00" to validate.
Hide
Thomas Heller added a comment -

Well, Math. Just realized that this is not really correct, since 1000 is closer to 999 than it is to 100.

Show
Thomas Heller added a comment - Well, Math. Just realized that this is not really correct, since 1000 is closer to 999 than it is to 100.
Hide
Thomas Heller added a comment -

Sorry, for that brainfart.

Show
Thomas Heller added a comment - Sorry, for that brainfart.
Hide
David Nolen added a comment -

Or we could return a custom ClojureScript Date type that provides the same api as js/Date but adds support for nanoseconds.

Show
David Nolen added a comment - Or we could return a custom ClojureScript Date type that provides the same api as js/Date but adds support for nanoseconds.
Hide
Thomas Heller added a comment -

One could extend the js/Date prototype with a setNanos/getNanos method and call them accordingly. I'd offer to implement that but the cljs.reader/parse-and-validate-timestamp function scares me, any objections to rewriting that?

As for the js/Date extension, thats easy enough:

(set! (.. js/Date -prototype -setNanos) (fn [ns] (this-as self (set! (.-nanos self) ns))))
(set! (.. js/Date -prototype -getNanos) (fn [] (this-as self (or (.-nanos self) 0))))

Not sure if its a good idea though, messing with otherwise native code might not be "stable".

Not convinced that keeping the nanos around is "required" since javascript cannot construct Dates with nanos, but its probably better not to lose the nanos in case of round tripping from clj -> cljs -> clj.

Show
Thomas Heller added a comment - One could extend the js/Date prototype with a setNanos/getNanos method and call them accordingly. I'd offer to implement that but the cljs.reader/parse-and-validate-timestamp function scares me, any objections to rewriting that? As for the js/Date extension, thats easy enough: (set! (.. js/Date -prototype -setNanos) (fn [ns] (this-as self (set! (.-nanos self) ns)))) (set! (.. js/Date -prototype -getNanos) (fn [] (this-as self (or (.-nanos self) 0)))) Not sure if its a good idea though, messing with otherwise native code might not be "stable". Not convinced that keeping the nanos around is "required" since javascript cannot construct Dates with nanos, but its probably better not to lose the nanos in case of round tripping from clj -> cljs -> clj.
Hide
David Nolen added a comment - - edited

We definitely don't want to mutate Date's prototype without namespacing. I'm not sure we want to mutate Date's prototype at all. That's why I suggested a ClojureScript type with the same interface as Date just as Google has done in Closure.

Show
David Nolen added a comment - - edited We definitely don't want to mutate Date's prototype without namespacing. I'm not sure we want to mutate Date's prototype at all. That's why I suggested a ClojureScript type with the same interface as Date just as Google has done in Closure.
Hide
Jonas Enlund added a comment -

This patch (CLJS-454.diff) takes the "truncate to millisecs" approach which I think is correct considering that the clojure reader does the same thing (but they truncate to nanoseconds instead).

The patch does some refactoring of parse-and-validate-timestamp and in the process fixes CLJS-564 as well as a subtle bug where the interpretation of the fraction part differed between clojurescript and clojure (see commit msg for details)

I also added tests.

Show
Jonas Enlund added a comment - This patch (CLJS-454.diff) takes the "truncate to millisecs" approach which I think is correct considering that the clojure reader does the same thing (but they truncate to nanoseconds instead). The patch does some refactoring of parse-and-validate-timestamp and in the process fixes CLJS-564 as well as a subtle bug where the interpretation of the fraction part differed between clojurescript and clojure (see commit msg for details) I also added tests.
Hide
Jonas Enlund added a comment -

There are duplicate tests in core-tests[1] and reader-tests[2]. This wouldn't be a big deal but each of them generates 7000+ assertions which is a bit excessive. Also note that the assertions in reader-tests doesn't do any padding so instant literals of the form #inst "2010-1-1..." are generated (which are not valid but goes unnoticed in the current implementation). This is why the CLJS-454.diff patch fails to pass the tests.

Should I update the patch where I remove some of the tests (either in core-tests or reader-tests)?

[1] https://github.com/clojure/clojurescript/blob/master/test/cljs/cljs/core_test.cljs#L1770
[2] https://github.com/clojure/clojurescript/blob/master/test/cljs/cljs/reader_test.cljs#L57

Show
Jonas Enlund added a comment - There are duplicate tests in core-tests[1] and reader-tests[2]. This wouldn't be a big deal but each of them generates 7000+ assertions which is a bit excessive. Also note that the assertions in reader-tests doesn't do any padding so instant literals of the form #inst "2010-1-1..." are generated (which are not valid but goes unnoticed in the current implementation). This is why the CLJS-454.diff patch fails to pass the tests. Should I update the patch where I remove some of the tests (either in core-tests or reader-tests)? [1] https://github.com/clojure/clojurescript/blob/master/test/cljs/cljs/core_test.cljs#L1770 [2] https://github.com/clojure/clojurescript/blob/master/test/cljs/cljs/reader_test.cljs#L57
Hide
David Nolen added a comment -

I note that Clojure includes these tests so probably not a good idea to remove them. Also they actually test different things right? I'd rather see the tests fixed if they need to be adjusted for the new behavior.

Show
David Nolen added a comment - I note that Clojure includes these tests so probably not a good idea to remove them. Also they actually test different things right? I'd rather see the tests fixed if they need to be adjusted for the new behavior.
Hide
Jonas Enlund added a comment -

I can't find the tests you're referring to. Link?

Show
Jonas Enlund added a comment - I can't find the tests you're referring to. Link?
Hide
Jonas Enlund added a comment -

format take care of the padding

(format "#inst \"2010-%02d-%02dT%02d:14:15.666-06:00\"" month day hour)

so those tests won't generate strings of the form "2010-1-1T1:14:15.666-06:00".

Also, the clojure reader can't read such literals and requires zero-padding:

user=> #inst "2010-1-1T1:14:15.666-06:00"
RuntimeException Unrecognized date/time syntax: 2010-1-1T1:14:15.666-06:00  clojure.instant/fn--6183/fn--6184 (instant.clj:118)
user=> #inst "2010-01-01T01:14:15.666-06:00"
#inst "2010-01-01T07:14:15.666-00:00"

The clojurescript reader returns bogus dates:

ClojureScript:cljs.user> (reader/read-string "#inst \"2010-1-1T1:14:15.666-06:00\"")
#inst "1970-01-01T00:00:00.000-00:00"
ClojureScript:cljs.user> (reader/read-string "#inst \"2010-01-01T01:14:15.666-06:00\"")
#inst "2010-01-01T07:14:15.666-00:00"

which is probably the reason the tests from https://github.com/clojure/clojurescript/blob/master/test/cljs/cljs/reader_test.cljs#L57 still passes without assertion errors.

Show
Jonas Enlund added a comment - format take care of the padding
(format "#inst \"2010-%02d-%02dT%02d:14:15.666-06:00\"" month day hour)
so those tests won't generate strings of the form "2010-1-1T1:14:15.666-06:00". Also, the clojure reader can't read such literals and requires zero-padding:
user=> #inst "2010-1-1T1:14:15.666-06:00"
RuntimeException Unrecognized date/time syntax: 2010-1-1T1:14:15.666-06:00  clojure.instant/fn--6183/fn--6184 (instant.clj:118)
user=> #inst "2010-01-01T01:14:15.666-06:00"
#inst "2010-01-01T07:14:15.666-00:00"
The clojurescript reader returns bogus dates:
ClojureScript:cljs.user> (reader/read-string "#inst \"2010-1-1T1:14:15.666-06:00\"")
#inst "1970-01-01T00:00:00.000-00:00"
ClojureScript:cljs.user> (reader/read-string "#inst \"2010-01-01T01:14:15.666-06:00\"")
#inst "2010-01-01T07:14:15.666-00:00"
which is probably the reason the tests from https://github.com/clojure/clojurescript/blob/master/test/cljs/cljs/reader_test.cljs#L57 still passes without assertion errors.

People

Vote (4)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: