mongodb / laravel-mongodb

A MongoDB based Eloquent model and Query builder for Laravel (Moloquent)
https://www.mongodb.com/docs/drivers/php/laravel-mongodb/
MIT License
7.02k stars 1.43k forks source link

Query\Builder->union() doesn't work #2188

Open elazar opened 3 years ago

elazar commented 3 years ago

Description

Illuminate\Database\Query\Builder, the base class of Jenssegers\Mongodb\Query\Builder, contains two methods, union() and unionAll(). Calls to these methods don't have the intended effect for the latter class because it doesn't make use of the $unions property of the parent class that is populated by these two methods.

This issue was previously reported in #990, but that issue was closed because a work-around of sorts was found, but it isn't one that's feasible for all use cases. Specifically, this work-around is to fetch both result sets and then use Laravel's collection methods to combine those result sets on the PHP side rather than on the Mongo side. For large result sets, this amounts to unnecessarily fetching a lot of data into PHP that ends up being discarded when pagination occurs.

Steps to reproduce

  1. Create a Mongo collection containing two documents with differing property values and an Eloquent model for that collection.
  2. Populate two query builder instances with criteria such that each instance will match a different document from the collection.
  3. Call union() or unionAll() on one of these query builder instances and pass it the other query builder instance.
  4. Execute the resulting query and inspect the results.

Actual behaviour

Only one document, the one corresponding to the query builder that receives the union() or unionAll() call, is returned.

Expected behaviour

Both documents should be returned when possible.

A $unionWith aggregation pipeline stage was added in Mongo 4.4; see this related API documentation and blog post. $unionWith is the closest Mongo equivalent to the SQL UNION ALL operator referenced in the blog post and represented by the unionAll() method of Illuminate\Database\Query\Builder. $unionWith should ultimately be included in the query as a result of calling unionAll() when the current Mongo version is >= 4.4.

For Mongo versions >= 3.6 and < 4.4, it may be possible (albeit less ideal) to simulate the same effect using the aggregation pipeline stage; see this example. Otherwise, an exception should be thrown indicating that the current Mongo version doesn't support this operation, rather than unionAll() silently failing to have the intended effect.

The SQL UNION operator removes duplicate records from the result set before returning it, while UNION ALL does not. Mongo doesn't have an equivalent of UNION, so it may be best to make this obvious by making calls to union() throw an exception.

divine commented 3 years ago

Hello,

PRs are welcome.

Thanks!

elazar commented 3 years ago

@divine

Hello,

PRs are welcome.

Thanks!

That's fine, but can I at least get some level of confirmation that everything I've said above sounds reasonable, just so I don't go to the trouble of writing a PR that ends up being rejected?

divine commented 3 years ago

Hello @elazar ,

I'm really sorry for the delayed response.

I don't have a strong positive opinion about adding another feature. This library needs some important fixes before any new feature will be added.

@Smolevich what do you think?

Thanks!

Smolevich commented 3 years ago

@divine @elazar If feauture don't break current functionality, PR's are welcome