Add the minus operation #45
Conversation
felixyz
commented
May 14, 2024
felixyz
commented
- No optimizations apart from the existing ones for n-ary operations
* No optimizations apart from the existing ones for n-ary operations
lib/bmg/operator/minus.rb
Outdated
|
|
||
| def all? | ||
| false | ||
| end |
There was a problem hiding this comment.
was under the impression some part of translation of nary operations expected it, but you're right it's not needed, will remove.
lib/bmg/operator/minus.rb
Outdated
| initial = operands[0].to_a | ||
| tuples = operands.drop(1).inject(initial) do |agg, op| | ||
| agg - op.to_a | ||
| end |
There was a problem hiding this comment.
Definitely not the optimal implementation, but I suppose we don't really care about it (?)
There was a problem hiding this comment.
Maybe using Ruby's Set? Which I presume uses object hashes? That should be reasonably good.
I'll do a perf test to check that it's actually faster (although the code will be very similar).
There was a problem hiding this comment.
Fixed this. My benchmark went from ~25s to ~1s.
See: https://gist.github.com/felixyz/7151d6acc1ef3d0d674caeacc9538862
Slight worry: the reported remaining number of tuples went down from ~55k to ~42k.
The optimization commit is here: d75847b
| check_union_compatible(other, "Minus") | ||
| dup.tap{|x| | ||
| ### attrlist stays the same | ||
| x.predicate = self.predicate & predicate |
There was a problem hiding this comment.
Not sure that's correct.
self.predicateis certainly correct and safe (but not very strong).- Intuitively, I would say
self.predicate & !predicatebut it should be further checked though...
| dup.tap{|x| | ||
| ### attrlist stays the same | ||
| x.predicate = self.predicate & predicate | ||
| x.keys = self._keys.union(self, x, other) if knows_keys? |
There was a problem hiding this comment.
I don't think that's correct. We simply keep the keys of the left operand I would say, and gain no other one.
| @@ -2,7 +2,7 @@ module Bmg | |||
| module Version | |||
| MAJOR = 0 | |||
| MINOR = 21 | |||
| TINY = 5 | |||
| TINY = 6 | |||
There was a problem hiding this comment.
Strictly speaking we should upgrade to 0.22.0
|
@felixyz no obligation to add optimizations for now, most of the time I do it on a opportunistic way when we discover clearly unoptimal query plans. Pushing restrictions and projections down the tree come to mind, though, if you want an exercice ;) |
|
@felixyz cleaned & merged. I had to release 0.22.0 for a customer project, and took that as an opportunity to include minus. |
