Skip to content
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

between broken with decimal columns #411

Closed
ioquatix opened this issue Feb 1, 2016 · 7 comments
Closed

between broken with decimal columns #411

ioquatix opened this issue Feb 1, 2016 · 7 comments

Comments

@ioquatix
Copy link

ioquatix commented Feb 1, 2016

If you try to supply a range made from BigDecimal values (e.g. provided by columns of type decimal), the code breaks.

def between other
if equals_quoted?(other.begin, -Float::INFINITY)
if equals_quoted?(other.end, Float::INFINITY)
not_in([])
elsif other.exclude_end?
lt(other.end)
else
lteq(other.end)
end
elsif equals_quoted?(other.end, Float::INFINITY)
gteq(other.begin)
elsif other.exclude_end?
gteq(other.begin).and(lt(other.end))
else
left = quoted_node(other.begin)
right = quoted_node(other.end)
Nodes::Between.new(self, left.and(right))
end
end

It's because you can't compare BigDecimal with INFINITY

> BigDecimal.new(10) == Float::INFINITY                           
FloatDomainError: Infinity
from (pry):10:in `to_r'

The solution is not to use comparison but to use Float#infinite? and BigDecimal#infinite?

[13] pry(main)> Float::INFINITY.infinite?                                       
=> 1
[14] pry(main)> -Float::INFINITY.infinite?                                      
=> -1
[15] pry(main)> BigDecimal::INFINITY.infinite?                                  
=> 1
[16] pry(main)> -BigDecimal::INFINITY.infinite?                                 
=> -1

The problem this introduces is that it no longer works for Numeric values, e.g. Fixnum and Bignum. However, these can never be infinite.

@ioquatix
Copy link
Author

ioquatix commented Feb 1, 2016

I have filed a bug report because it might make sense for Numeric to provide #infinite?

https://bugs.ruby-lang.org/issues/12039

Not sure what the best outcome is here, but I don't think this should fail.

@sgrif
Copy link

sgrif commented Feb 1, 2016

Currently, the rough plan is to remove the infinite handling from Arel, and move it into Active Record as a result of rails/rails@6efb394. Your proposed fix in Ruby would solve this, and is probably the best place to handle it.

@sgrif sgrif closed this as completed Feb 1, 2016
@ioquatix
Copy link
Author

ioquatix commented Feb 1, 2016

Thanks for the quick reply.

What is the reasoning behind moving it to the ActiveRecord code base?

@sgrif
Copy link

sgrif commented Feb 1, 2016

Arel is an AST, not a query builder. We need information that only exists at the AR level for this to be useful.

@ioquatix
Copy link
Author

@sgrif Just thought I'd let you know, that this is now fixed in Ruby.

@sgrif
Copy link

sgrif commented Nov 14, 2016

Thank you for following up.

@ioquatix
Copy link
Author

@sgrif Thanks so much for all your hard work dude, you're awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants