[SERVER-6194] Nested $add operators may be improperly reordered by constant folding implementation Created: 24/Jun/12  Updated: 11/Jul/16  Resolved: 27/Jul/12

Status: Closed
Project: Core Server
Component/s: Aggregation Framework
Affects Version/s: None
Fix Version/s: 2.2.0-rc1

Type: Bug Priority: Major - P3
Reporter: Aaron Staple Assignee: Matt Dannenberg
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-6570 disallow $add with strings Closed
is related to SERVER-6195 consider $concat to concatenate strin... Closed
Operating System: ALL
Participants:

 Description   

Observed behavior: When a nested $add is used for string concatenation, the arguments may be concatenated in the wrong order.
Expected behavior: Concatenation in the proper order.

There is some code to detect string $add operands and avoid an improper constant folding, but this is not applied for nested $add expressions:

 
                /*
                  If the child operand is the same type as this, then we can
                  extract its operands and inline them here because we already
                  know this is commutative and associative because it has a
                  factory.  We can detect sameness of the child operator by
                  checking for equality of the factory
 
                  Note we don't have to do this recursively, because we
                  called optimize() on all the children first thing in
                  this call to optimize().
                */
                ExpressionNary *pNary =
                    dynamic_cast<ExpressionNary *>(pE.get());
                if (!pNary)
                    pNew->addOperand(pE);
                else {
                    intrusive_ptr<ExpressionNary> (*const pChildFactory)() =
                        pNary->getFactory();
                    if (pChildFactory != pFactory)
                        pNew->addOperand(pE);
                    else {
                        /* same factory, so flatten */
                        size_t nChild = pNary->vpOperand.size();
                        for(size_t iChild = 0; iChild < nChild; ++iChild) {
                            intrusive_ptr<Expression> pCE(
                                pNary->vpOperand[iChild]);
                            if (dynamic_cast<ExpressionConstant *>(pCE.get()))
                                pConst->addOperand(pCE);
                            else
                                pNew->addOperand(pCE);
                        }
                    }
                }

Test:

c = db.c;
c.drop();
 
c.save( { x:3 } );
 
project = { $project:{ a:{ $add:[ 1, 2, { $add:[ 'foo', '$x' ] } ] } } };
 
printjson( c.runCommand( 'aggregate', { pipeline:[ project ], explain:true } ) );
 
// Actual result is '312foo'.
assert.eq( '12foo3', c.aggregate( project ).result[ 0 ].a );



 Comments   
Comment by auto [ 18/Dec/12 ]

Author:

{u'date': u'2012-07-24T21:29:40Z', u'name': u'Matt Dannenberg', u'email': u'dannenberg.matt@gmail.com'}

Message: SERVER-6195 implement $concat for strings in aggro

this also eliminates the problems seen in SERVER-6194

Note: modified by Mathias on 2012/12/17 to use new Doc/Val API
Branch: master
https://github.com/mongodb/mongo/commit/966fa94895fb5526367327c66f284dabbc566a8e

Comment by Mathias Stearn [ 24/Jul/12 ]

Not a problem due to SERVER-6570

Comment by Aaron Staple [ 03/Jul/12 ]

Sounds fine to me. As a reminder, if we comment out optimize() call(s) we should verify there isn't any code that functionally depends on them.

Comment by Mathias Stearn [ 03/Jul/12 ]

I just looked through all of the optimize() calls and none of them seem to cover real-world scenarios that are likely to occur. I think if we can't get all of the bugs worked out it would be reasonable to just comment out the call to optimize() for 2.2.

Generated at Thu Feb 08 03:11:00 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.