Cast to Integer when there are no decimals #37
Conversation
composerinteralia
force-pushed
the
cast-to-integer
branch
from
dc6974f
to
7048795
Compare
|
Just for my own future re-reading, I'll note that this is branching on whether the returned column type has any decimal digits -- i.e. |
Thank you for reading what I meant instead of what I wrote 🤦🏻. I'll update it to say something more like that. |
Prior to this commit we'd always cast a SUM query to `BigDecimal`, since the column type we get from the server is 246 (i.e. TRILOGY_TYPE_NEWDECIMAL). After this commit we'll only cast to `BigDecimal` if there are digits in the decimal place. If there are none (i.e. if column->decimals is 0), we cast to `Integer` instead. Closes #36
composerinteralia
force-pushed
the
cast-to-integer
branch
from
7048795
to
bb471ce
Compare
contrib/ruby/ext/trilogy-ruby/cast.c
Outdated
| VALUE str = rb_str_new(value->data, value->data_len); | ||
| return rb_funcall(rb_mKernel, id_BigDecimal, 1, str); | ||
| if (column->decimals == 0) { | ||
| return rb_cstr2inum(value->data, 10); |
There was a problem hiding this comment.
@HParker asked me some questions about value->data and now I wonder whether this should be rb_cstr_parse_inum (or something like that), which takes a length.
There was a problem hiding this comment.
Well not that, since I don't think it's public. Maybe safer to do the same thing we do in the else branch—create a Ruby string and then call the function to cast it?
There was a problem hiding this comment.
create a Ruby string and then call the function to cast it?
I'm going to go with that for now and leave possible optimizations as a TODO
rb_cstr2inum takes a NUL-terminated string, which `value->data` does not appear to be. To be safe, this follows the existing pattern of casting with a Ruby string passed to the Integer() function.
Also clean up a few duplicated lines
In #37 there was a change that updated the value of SUM to be Integer if what is being summed are all Integers. It used to be that it will _always_ return a Decimal type. When we pulled in that change, there were tests that failed because we were doing arthimitic operations on the result of queries that have SUM. Ruby will have different results if you divide intergers and decimals. In order to keep that behavior for us (GitHub) while allows other consumers to use SUM and expect the _right_ type we are introducing a new query flag that we can set in our adapter. This query flag will be what will determine if we should be casting the value of SUM to follow the _new_ behavior or roll back to the _old_ behavior
Prior to this commit we'd always cast a SUM query to
BigDecimal, since the column type we get from the server is 246 (i.e. TRILOGY_TYPE_NEWDECIMAL).After this commit we'll only cast to
BigDecimalif the column includes decimal digits. If there are none (i.e. if column->decimals is 0), we cast toIntegerinstead.Closes #36