auraphp / Aura.SqlQuery

Independent query builders for MySQL, PostgreSQL, SQLite, and Microsoft SQL Server.
MIT License
452 stars 86 forks source link

Group by [Feature] #121

Closed harikt closed 7 years ago

harikt commented 7 years ago

Hi,

One of the missing feature I think about the library is creating grouping statements. I have been looking at https://github.com/paragonie/easydb#grouping-of-conditions .

An example taken :

$statement = EasyStatement::open()
    ->group()
        ->with('subtotal > ?')
        ->andWith('taxes > ?')
    ->end()
    ->orGroup()
        ->with('cost > ?')
        ->andWith('cancelled = 1')
    ->end();

echo $statement; /* (subtotal > ? AND taxes > ?) OR (cost > ? AND cancelled = 1) */

Does anyone like this feature ?

pavarnos commented 7 years ago

would be nice to do a similar syntax with where()

pmjones commented 7 years ago

I am not enthusiastic about this. It can be accomplished just as easily with 2.x or 3.x, either like so ...

$select
    ->where(' ( subtotal > :subtotal')
    ->where('taxes > :taxes ) ')
    ->orWhere(' ( cost > :cost')
    ->where('cancelled = 1 ) ');

... or like so:

$select
    ->where(' ( subtotal > :subtotal AND taxes > :taxes ) ')
    ->orWhere(' ( cost > :cost AND cancelled = 1 ) ');

Do you see what I mean?

harikt commented 7 years ago

I see what you mean, but considering the use of query builder

Example

$query->group()
if ($this->subtotal) {
    $query->where('subtotal = :subtotal', $this->subtotal)
}
if ($this->taxes) {
    $query->where('taxes = :taxes', $this->taxes)
}
$query->end()

UPDATE : This query probably will be wrong when the subtotal and taxes are null .

pmjones commented 7 years ago

/me tilts head

OK, maybe.

At the very least, group() is too easily confused with groupBy() so you'd need a different name. Maybe begin() or open() or start() or something like that.

Further, I imagine that this is a relatively complex thing to implement. It will need ...

I'm sure there is even more to it than that.

harikt commented 7 years ago

@pmjones I agree with you, it is a bit complex.

You can see most of the query builders support the same.

Yet another one inspired by Aura : https://github.com/shadowhand/latitude#grouping-conditions you may have seen, if not https://github.com/shadowhand/latitude#aurasqlquery .

pmjones commented 7 years ago

I have in fact seen; I would have preferred a collaboration, had he asked. (/me shrugs)

pmjones commented 7 years ago

@harikt Meanwhile, if you want to try building out this feature, please go ahead and I'll review.

harikt commented 7 years ago

I would have tried this, but due to certain reasons I don't expect seeing it happening soon.

I have no problem if you give this idea a halt, due to time / life constraints. We can always revisit this for next releases.

pmjones commented 7 years ago

Noted -- I'll leave it open for anyone else who has time & inclination to pursue it.

pmjones commented 7 years ago

@harikt @pavarnos et al --

Given the example from @harikt above, would it be sufficient to allow where() and having() to take a closure as the first argument, a la:

$select->where(function ($select) use ($this) {
    if ($this->subtotal) {
        $select->where('subtotal = :subtotal', $this->subtotal)
    }
    if ($this->taxes) {
        $select->where('taxes = :taxes', $this->taxes)
    }
});

The closure would place parentheses in the where() and having() stack before and after the closure-related calls.

Thoughts?

pavarnos commented 7 years ago

Clever idea. Interested to see how you would implement it

pmjones commented 7 years ago

Agh -- "clever" is a terrible thing to say! ;-)

Look for an impl. shortly -- should be quick.

pmjones commented 7 years ago

@harikt @pavarnos take a look at #136 -- inelegant, but operational.

pmjones commented 7 years ago

Closing this as "fixed" by #136 -- let me know if it needs to be reopened.