dkubb / axiom

Simplifies querying of structured data using relational algebra
https://github.com/dkubb/axiom
MIT License
456 stars 22 forks source link

Relation equality considers completely unequal relations as equal #6

Closed d11wtq closed 13 years ago

d11wtq commented 13 years ago

Given two relations of different types, testing them for equality should always return false.

Veritas::TABLE_DEE == Veritas::Relation.new([ [:ID, Integer] ], [ [20] ])
# => true

This is a bug. The only relations that should ever be considered equal are those that have the same headers and the same set of tuples (order irrelevant).

The issue comes from the fact Veritas is coercing and projecting the other side of the comparison to match itself, before doing the comparison, which breaks the intention.

Veritas::Relation.new([ [:ID, Integer] ], [ [20] ]).header
# => [<Attribute::Integer name: ID>]

Veritas::TABLE_DEE.send(:coerce, Veritas::Relation.new([ [:ID, Integer] ], [ [20] ])).header
# => []
dkubb commented 13 years ago

@d11wtq nice catch. The main reason I do the projection is an implementation detail, to align the arrays inside the tuples into the same order so the comparison can be delegated down to Array#==. Before I do that I should probably check the headers on both sides and see if they are equivalent first.

dkubb commented 13 years ago

Actually, I realize now that a month or two ago I refactored Tuple so that underneath the data is stored in a Hash instead of an Array. That means there's no need to use project to "reorder" the header. This should simplify everything.

dkubb commented 13 years ago

I also followed this up with another commit that changes coerce do it basically passes-through the object if it's already a relation: https://github.com/dkubb/veritas/commit/c1e8c73e6f725ef46df952737ee8756080bdd0c9

Before:

Veritas::TABLE_DEE.send(:coerce, Veritas::Relation.new([ [:ID, Integer] ], [ [20] ])).header  # => []

After:

Veritas::TABLE_DEE.send(:coerce, Veritas::Relation.new([ [:ID, Integer] ], [ [20] ])).header  # => [<Attribute::Integer name: ID>]

Even though Relation#coerce is an internal private method I still felt like it was important to have it "do the right thing" in case I or someone else used it in the future in another context.