-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the minus operation #45
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
module Bmg | ||
module Operator | ||
# | ||
# Minus operator. | ||
# | ||
# Returns all tuples which are in the left operand but not | ||
# in the right operand. | ||
# | ||
# This implementation is actually a NAry-Minus, since it handles | ||
# an arbitrary number of operands. | ||
# | ||
class Minus | ||
include Operator::Nary | ||
|
||
def initialize(type, operands) | ||
@type = type | ||
@operands = operands | ||
end | ||
|
||
public | ||
|
||
def all? | ||
false | ||
end | ||
|
||
def each(&bl) | ||
return to_enum unless block_given? | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely not the optimal implementation, but I suppose we don't really care about it (?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe using Ruby's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
tuples.each do |t| | ||
yield t | ||
end | ||
felixyz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
|
||
def to_ast | ||
[ :except ] + operands.map(&:to_ast) | ||
felixyz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
|
||
end # class Union | ||
end # module Operator | ||
end # module Bmg |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -287,20 +287,33 @@ def ungroup(attrlist) | |
} | ||
end | ||
|
||
def union(other) | ||
def check_union_compatible(other, opname) | ||
if typechecked? && knows_attrlist? && other.knows_attrlist? | ||
missing = self.attrlist - other.attrlist | ||
raise TypeError, "Union incompatible: missing right attributes #{missing.join(', ')}" unless missing.empty? | ||
raise TypeError, "#{opname} requires compatible attribute lists, but the right operand is missing the following attributes: #{missing.join(', ')}" unless missing.empty? | ||
extra = other.attrlist - self.attrlist | ||
raise TypeError, "Union incompatible: missing left attributes #{extra.join(', ')}" unless extra.empty? | ||
raise TypeError, "#{opname} requires compatible attribute lists, but the left operand is missing the following attributes: #{extra.join(', ')}" unless extra.empty? | ||
end | ||
end | ||
|
||
def union(other) | ||
check_union_compatible(other, "Union") | ||
dup.tap{|x| | ||
### attrlist stays the same | ||
x.predicate = self.predicate | predicate | ||
x.keys = self._keys.union(self, x, other) if knows_keys? | ||
} | ||
end | ||
|
||
def minus(other) | ||
check_union_compatible(other, "Minus") | ||
dup.tap{|x| | ||
### attrlist stays the same | ||
x.predicate = self.predicate & predicate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure that's correct.
|
||
x.keys = self._keys.union(self, x, other) if knows_keys? | ||
blambeau marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's correct. We simply keep the keys of the left operand I would say, and gain no other one. |
||
} | ||
end | ||
|
||
def unwrap(attrlist) | ||
known_attributes!(attrlist) if typechecked? && knows_attrlist? | ||
dup.tap{|x| | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ module Bmg | |
module Version | ||
MAJOR = 0 | ||
MINOR = 21 | ||
TINY = 5 | ||
TINY = 6 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strictly speaking we should upgrade to 0.22.0 |
||
end | ||
VERSION = "#{Version::MAJOR}.#{Version::MINOR}.#{Version::TINY}" | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
--- | ||
- bmg: |- | ||
suppliers.minus(suppliers) | ||
sqlite: |- | ||
SELECT | ||
`t1`.`sid`, | ||
`t1`.`name`, | ||
`t1`.`city`, | ||
`t1`.`status` | ||
FROM | ||
`suppliers` AS 't1' | ||
EXCEPT | ||
SELECT | ||
`t1`.`sid`, | ||
`t1`.`name`, | ||
`t1`.`city`, | ||
`t1`.`status` | ||
FROM | ||
`suppliers` AS 't1' | ||
- bmg: |- | ||
suppliers.minus(suppliers).minus(suppliers) | ||
sqlite: |- | ||
SELECT | ||
`t1`.`sid`, | ||
`t1`.`name`, | ||
`t1`.`city`, | ||
`t1`.`status` | ||
FROM | ||
`suppliers` AS 't1' | ||
EXCEPT | ||
SELECT | ||
`t1`.`sid`, | ||
`t1`.`name`, | ||
`t1`.`city`, | ||
`t1`.`status` | ||
FROM | ||
`suppliers` AS 't1' | ||
EXCEPT | ||
SELECT | ||
`t1`.`sid`, | ||
`t1`.`name`, | ||
`t1`.`city`, | ||
`t1`.`status` | ||
FROM | ||
`suppliers` AS 't1' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
require 'spec_helper' | ||
module Bmg | ||
module Operator | ||
describe Minus do | ||
|
||
let(:r1) { | ||
[ | ||
{ a: 1 }, | ||
{ a: 2 }, | ||
{ a: 3 }, | ||
{ a: 4 }, | ||
{ a: 5 } | ||
] | ||
} | ||
|
||
let(:r2) { | ||
[ | ||
{ a: 1 }, | ||
{ a: 4 } | ||
] | ||
} | ||
|
||
let(:r3) { | ||
[ | ||
{ a: 3 } | ||
] | ||
} | ||
|
||
it 'works' do | ||
difference = Minus.new(Type::ANY, [r1, r2, r3]) | ||
expect(difference.to_a).to eql([{ a: 2 },{ a: 5 }]) | ||
end | ||
|
||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was under the impression some part of translation of nary operations expected it, but you're right it's not needed, will remove.