Storage engine issues comparing Dates and Timestamps

[Michael Cahill]

In testing the WiredTiger storage engine, we have run into https://jira.mongodb.org/browse/SERVER-3304

Specifically, in the jsCore type3.js test, due to implementation details of WiredTiger, we end up comparing a Date with a Timestamp, and IndexEntryComparison::compare asserts.

Is the change below a reasonable way to address 3304?  It makes type3.js pass for WiredTiger, so if this isn't the right way to go, can anyone suggest an alternative?

Thanks,

Michael.

diff --git a/jstests/core/date2.js b/jstests/core/date2.js

--- a/jstests/core/date2.js

+++ b/jstests/core/date2.js

@@ -8,6 +8,4 @@ t.ensureIndex( {a:1} );

 t.save( {a:new Timestamp()} );

-if ( 0 ) { // SERVER-3304

 assert.eq( 1, t.find( {a:{$gt:new Date(0)}} ).itcount() );

-}

diff --git a/jstests/core/type3.js b/jstests/core/type3.js

--- a/jstests/core/type3.js

+++ b/jstests/core/type3.js

@@ -45,16 +45,12 @@ assert.eq( [[{$maxElement:1},{$maxElemen

 t.remove({});

 t.save( {a:new Timestamp()} );

 assert.eq( 1, t.find( {a:{$type:17}} ).itcount() );

-if ( 0 ) { // SERVER-3304

 assert.eq( 0, t.find( {a:{$type:9}} ).itcount() );

-}

 // Type Date

 t.remove({});

 t.save( {a:new Date()} );

-if ( 0 ) { // SERVER-3304

 assert.eq( 0, t.find( {a:{$type:17}} ).itcount() );

-}

 assert.eq( 1, t.find( {a:{$type:9}} ).itcount() );

 // Type Code

diff --git a/src/mongo/bson/bsonelement.cpp b/src/mongo/bson/bsonelement.cpp

--- a/src/mongo/bson/bsonelement.cpp

+++ b/src/mongo/bson/bsonelement.cpp

@@ -831,14 +831,10 @@ namespace mongo {

         case Bool:

             return *l.value() - *r.value();

         case Timestamp:

-            // unsigned compare for timestamps - note they are not really dates but (ordinal + time_t)

-            if ( l.date() < r.date() )

-                return -1;

-            return l.date() == r.date() ? 0 : 1;

         case Date:

             {

-                long long a = (long long) l.Date().millis;

-                long long b = (long long) r.Date().millis;

+                long long a = (long long) l.date().millis;

+                long long b = (long long) r.date().millis;

                 if( a < b )

                     return -1;

                 return a == b ? 0 : 1;

diff --git a/src/mongo/bson/bsonelement.h b/src/mongo/bson/bsonelement.h

--- a/src/mongo/bson/bsonelement.h

+++ b/src/mongo/bson/bsonelement.h

@@ -193,6 +193,9 @@ namespace mongo {

             @see Bool(), trueValue()

         */

         Date_t date() const {

+            // SERVER-3304

+            if (type() == mongo::Timestamp )

+                return timestampTime();

             return *reinterpret_cast< const Date_t* >( value() );

         }

@@ -502,7 +505,12 @@ namespace mongo {

         friend class BSONObjIterator;

         friend class BSONObj;

         const BSONElement& chk(int t) const {

-            if ( t != type() ) {

+            // SERVER-3304

+            int mytype = type();

+            if ( (t == mongo::Timestamp && mytype == mongo::Date) ||

+                 (t == mongo::Date && mytype == mongo::Timestamp) )

+                t = mytype;

+            if ( t != mytype ) {

                 StringBuilder ss;

                 if( eoo() )

                     ss << "field not found, expected type " << t;

[Michael Cahill]

FWIW, I've opened Allow Date and Timestamp to be converted interchangeably for this change and updated SERVER-3304.

Michael.

[Andy Schwerin]

The challenge here is to avoid changing sort order, so that using the new code with existing indexes doesn't lead to btree corruption or incorrect query results.  This is challenging, of course, because the original code didn't have a well-defined sort order when Date and Timestamp were compared.  If I had my druthers, I'd change canonicalizeBSONType() so that Date and Timestamp weren't directly comparable.  They shouldn't be, anyways.  In both directions the comparison is poorly formed.

Your proposed change goes a long way to improving the sort order, but also does change it.  So the question becomes, was it possible to mix Date_t and Timestamp types in the same indexed field in prior versions, and under what circumstances.  I'll do some research, but if you have any thoughts on that, please let me know.

-Andy

On Tue, Sep 2, 2014 at 9:24 PM, Michael Cahill <m...@wiredtiger.com> wrote:

FWIW, I've opened Allow Date and Timestamp to be converted interchangeably for this change and updated SERVER-3304.

Michael.

--
You received this message because you are subscribed to the Google Groups "mongodb-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mongodb-dev...@googlegroups.com.
To post to this group, send email to mongo...@googlegroups.com.
Visit this group at http://groups.google.com/group/mongodb-dev.
For more options, visit https://groups.google.com/d/optout.

[Michael Cahill]

Hi Andy,

> Your proposed change goes a long way to improving the sort order, but also does change it. So the question becomes, was it possible to mix Date_t and Timestamp types in the same indexed field in prior versions, and under what circumstances. I'll do some research, but if you have any thoughts on that, please let me know.

Thanks for reviewing this change and giving feedback. The change in sort order between Timestamp values was certainly unintentional and I'd be happy to fix that.

As far as I can tell, it has never been possible to mix a Date and a Timestamp in the same indexed field. As soon as as there is an attempt to compare a Date with a Timestamp (where Date happens to be the lhs of the compare), an assertion fires. This would effectively prevent any mixing of Date and TImestamp in the same indexed field.

> If I had my druthers, I'd change canonicalizeBSONType() so that Date and Timestamp weren't directly comparable. They shouldn't be, anyways. In both directions the comparison is poorly formed.

Now that I understand the BSON code a little better, changing canonicalizeBSONType certainly seems like a cleaner solution. I don't understand all of the implications, though -- would it help for me to open a pull request with that change?

Thanks again,
Michael.