[SERVER-21861] Better Timestamp object comparison in mongo shell Created: 11/Dec/15 Updated: 09/Sep/20 Resolved: 05/Aug/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Shell |
| Affects Version/s: | 3.2.0 |
| Fix Version/s: | 4.7.0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Randolph Tan | Assignee: | Daniel Gottlieb (Inactive) |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||
| Backwards Compatibility: | Minor Change | ||||||||||||||||||||||||||||||||
| Sprint: | STM 2020-08-10 | ||||||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||||||
| Linked BF Score: | 16 | ||||||||||||||||||||||||||||||||
| Story Points: | 3 | ||||||||||||||||||||||||||||||||
| Description |
|
It would be great if the <, >, <=, >= operators on Timestamp objects would work as expected. The bsonWoCompare can be used for the mean time even though it will end up comparing the member functions as well as long as 't' fields are compared first over the 'i' fields. |
| Comments |
| Comment by Daniel Gottlieb (Inactive) [ 05/Aug/20 ] | ||||||
|
The pushed patch offers some guard rails when using Timestamp objects. Technically, the mongo shell is unchanged. Timestamps can still be compared (i.e: toString -ed) without an error. However, when the shell is invoked from within resmoke.py will inject an error when calling Timestamp.toString. Most notably, this makes logging timestamps an error. The easiest path forward is to instead log `tojson(timestampVariable)`, similar to what one would do when logging a bson object. | ||||||
| Comment by Githook User [ 05/Aug/20 ] | ||||||
|
Author: {'name': 'Daniel Gottlieb', 'email': 'daniel.gottlieb@mongodb.com', 'username': 'dgottlieb'}Message: | ||||||
| Comment by Githook User [ 05/Aug/20 ] | ||||||
|
Author: {'name': 'Daniel Gottlieb', 'email': 'daniel.gottlieb@mongodb.com', 'username': 'dgottlieb'}Message: | ||||||
| Comment by Daniel Gottlieb (Inactive) [ 14/Jul/20 ] | ||||||
|
I'm going to explore the first option. I could be mistaking how change two or three would manifest, but I'm worried that options which output a different timestamp string will be a problematic user change. For example, when spelunking test logs, it's common to copy a timestamp object from a server response that's output by the shell. One would no longer be able to simply paste that string as-is into a search bar for cross-referencing with server logs emitting the timestamp – the server logs won't be zero-padded. | ||||||
| Comment by Robert Guo (Inactive) [ 14/Jul/20 ] | ||||||
|
jesse We're going to try the 1st one. Brooke's first sentence was referring to an initial idea that we later rejected. | ||||||
| Comment by A. Jesse Jiryu Davis [ 14/Jul/20 ] | ||||||
|
Thanks - that seems a bit contradictory. Which suggestion are we exploring? | ||||||
| Comment by Brooke Miller [ 14/Jul/20 ] | ||||||
|
We discussed this ticket in STM's estimation session today and individuals encouraged that we avoid the first two suggestions outlined above because resmoke.py shouldn't cover up Javascript issues in the shell. However, we discussed that the first suggestion is the best option and we should attempt that route to see how it goes and if there are fallouts. | ||||||
| Comment by A. Jesse Jiryu Davis [ 10/Jul/20 ] | ||||||
|
To mitigate the pain for now, I got some ideas from max.hirschhorn and matthew.saltz, and I propose either:
| ||||||
| Comment by Brooke Miller [ 10/Mar/20 ] | ||||||
|
Based on our conversation with the STM team during Triaging, this is something we would like to consider down the road when Javascript has better support for operator overloading. | ||||||
| Comment by Kevin Pulo [ 01/Jul/16 ] | ||||||
This is the crux of the problem — they're not actually "Types" (even though that's how we think about them), they're just Objects (to Javascript). | ||||||
| Comment by Max Hirschhorn [ 30/Jun/16 ] | ||||||
|
NumberLong instances are impacted in the same way as Timestamp instances. The + operator will call the NumberLong.prototype.valueOf() function, which converts the value to a double-precision 64-bit IEEE 754 value because that's the only number type that JavaScript really has.
It seems like a risky and non-trivial amount of work to modify the JS::Value class in SpiderMonkey in order to be able to accurately represent an int64_t type. | ||||||
| Comment by Spencer Brody (Inactive) [ 30/Jun/16 ] | ||||||
|
We've already added new types to the mongo shell beyond what exists in pure javascript, like NumberLong. Can we not do something similar with Timestamp? | ||||||
| Comment by Kevin Pulo [ 30/Jun/16 ] | ||||||
|
It really isn't possible to do this "properly" (ie. ts1 < ts2) without hacking the language to be Not-Quite-Javascript ("MongoScript"?). See Javascript simply has not been designed with any consideration for the idea that objects might want to act like native types. Even with the above valueOf() hack in place, == and != still won't work as expected (and there is no kludge that can make them work, short of hacking the guts of the JS interpreter/JIT compiler). This would mean that you can have a and b such that a >= b && a <= b, yet a != b, which is arguably worse. Value objects are likely to be able to address this, but they are currently a long way from being in an ES standard. The only real option is to have ugly comparison methods and use them everywhere, eg. ts1.eq(ts2), ts1.lte(ts2). But there's still no way of prohibiting a jstest or user from accidentally doing ts1 == ts2 or ts1 <= ts2. | ||||||
| Comment by Spencer Brody (Inactive) [ 16/Jun/16 ] | ||||||
|
Is there no way to make the comparison happen in C++ rather than javascript, where it could do the right thing? I just wasted a full day diagnosing a build failure that turned out to be a direct result of this. I filed | ||||||
| Comment by Kevin Pulo [ 01/Jan/16 ] | ||||||
|
A simpler, easier alternative is to make Timestamp's valueOf() return a string representation that has the necessary lexicographic order. Sufficient is to zero-pad the numbers, since they are fixed precision 32 bit ints. And use hex to make them be a bit shorter, but still readable.
Converting to and comparing strings is a bit slow, but the implicit conversion is doing that already anyway. This way is only slightly slower (the strings are always 33 bytes long, rather than 15 to 33 bytes), but has the significant advantage of being correct in all cases — instead of often right but sometimes silently wrong. This also has the nice upshot of allowing the standard assert helpers (assert.lt, etc) to continue working without modification, thereby obviating | ||||||
| Comment by Max Hirschhorn [ 11/Dec/15 ] | ||||||
|
I don't think it's possible to have <, >, <=, and >= work as expected for all Timestamp instances. Timestamp instances are converted to strings when compared using relational operators. We could change Timestamp.prototype.valueOf() to convert them to a Number instance as follows:
However, that only gives us 21 bits to use from high before we'd exceed Number.MAX_SAFE_INTEGER, which is insufficient for representing the number of seconds that have elapsed since the Unix epoch. Using bsonWoCompare() would provide a complete solution because a similar issue also affects large NumberLong instances. We could even define Timestamp.prototype.compare(other), etc. functions for convenience. |