<< Back to previous view

[CLJS-454] Instance Reader to Support Micro/Nanoseconds Created: 04/Jan/13  Updated: 03/Aug/13  Resolved: 03/Aug/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Anatoly Polinsky Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: reader, timestamp
Environment:

N/A


Attachments: File CLJS-454.diff     File CLJS-454-patch2.diff     Text File cljs-reader-nanosecond-workarround-corrected.patch    

 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")


 Comments   
Comment by David Nolen [ 04/Jan/13 4:55 PM ]

Is this behavior supported in Clojure?

Comment by Jozef Wagner [ 05/Jan/13 6:48 AM ]

It seems it is

Comment by Anatoly Polinsky [ 06/Jan/13 2:18 AM ]

@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

Comment by David Nolen [ 09/Jan/13 10:59 PM ]

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

Comment by Thomas Heller [ 05/Feb/13 3:28 PM ]

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.

Comment by Thomas Heller [ 05/Feb/13 3:35 PM ]

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

Comment by Thomas Heller [ 05/Feb/13 3:37 PM ]

Sorry, for that brainfart.

Comment by David Nolen [ 06/Feb/13 11:39 AM ]

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

Comment by Thomas Heller [ 20/Feb/13 5:20 PM ]

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.

Comment by David Nolen [ 21/Feb/13 12:25 AM ]

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.

Comment by Jonas Enlund [ 31/Jul/13 4:21 AM ]

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.

Comment by Jonas Enlund [ 31/Jul/13 4:54 AM ]

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

Comment by David Nolen [ 31/Jul/13 4:59 AM ]

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.

Comment by Jonas Enlund [ 31/Jul/13 5:58 AM ]

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

Comment by David Nolen [ 31/Jul/13 6:06 AM ]

https://github.com/clojure/clojure/blob/master/test/clojure/test_clojure/reader.clj#L482

Comment by Jonas Enlund [ 31/Jul/13 6:18 AM ]

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.

Comment by David Nolen [ 03/Aug/13 5:05 PM ]

fixed, http://github.com/clojure/clojurescript/commit/a749ab6c04baa6bd4c890e860adf43b3d702932b





Generated at Fri Sep 19 21:06:01 CDT 2014 using JIRA 4.4#649-r158309.