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

[Bug?] Performing SUM function on integer column returns BigDecimal result #36

Closed
adrianna-chang-shopify opened this issue Nov 23, 2022 · 4 comments · Fixed by #37
Closed

Comments

@adrianna-chang-shopify
Copy link
Collaborator

Hi folks!

Encountering something a bit odd -- when performing SUM functions, the result is returned as a BigDecimal . A simple repro:

diff --git a/contrib/ruby/test/client_test.rb b/contrib/ruby/test/client_test.rb
index d7bc60c..71ce771 100644
--- a/contrib/ruby/test/client_test.rb
+++ b/contrib/ruby/test/client_test.rb
@@ -132,6 +132,18 @@ class ClientTest < TrilogyTest
     assert_equal [1, 4, 2, 3, 3, 1], result.rows
   end
 
+  def test_trilogy_sum_query
+    client = new_tcp_client
+    create_test_table(client)
+
+    client.query("INSERT INTO trilogy_test (int_test) VALUES ('4')")
+    client.query("INSERT INTO trilogy_test (int_test) VALUES ('3')")
+    client.query("INSERT INTO trilogy_test (int_test) VALUES ('1')")
+
+    result = client.query("SELECT SUM(int_test) FROM trilogy_test")
+    puts result.inspect
+  end
+
   def test_trilogy_query_result_object
     client = new_tcp_client
➜  ruby git:(main) ✗ be rake test TEST=test/client_test.rb TESTOPTS="--name='test_trilogy_sum_query'"
install -c tmp/arm64-darwin21/cext/3.1.2/cext.bundle lib/trilogy/cext.bundle
cp tmp/arm64-darwin21/cext/3.1.2/cext.bundle tmp/arm64-darwin21/stage/lib/trilogy/cext.bundle
/opt/rubies/3.1.2/bin/ruby -w -I"lib:test" -I"/Users/adriannachang/.gem/ruby/3.1.2/gems/rake-13.0.1/lib" "/Users/adriannachang/.gem/ruby/3.1.2/gems/rake-13.0.1/lib/rake/rake_test_loader.rb" "test/client_test.rb" --name='test_trilogy_sum_query'
Run options: --name=test_trilogy_sum_query --seed 62033

# Running:

#<Trilogy::Result:0x0000000104988730 @fields=["SUM(int_test)"], @rows=[[0.8e1]], @query_time=0.000148>
.

Finished in 0.091646s, 10.9116 runs/s, 0.0000 assertions/s.

I'm wondering if this is a bug, or if I'm just missing a query option to cast the result to an Integer properly. At Shopify we're using Sorbet, and it complains loudly if a query ends up returning a BigDecimal when it was expecting an Integer.

Appreciate the help! ❤️

@composerinteralia
Copy link
Contributor

Casting to an Integer when SUMming int columns makes sense to me. I don't think it's too difficult (we already get back information about whether the column includes decimals). I can open a PR later today.

@adrianna-chang-shopify
Copy link
Collaborator Author

Awesome, thanks Daniel! 🙏 ❤️

composerinteralia added a commit that referenced this issue Nov 23, 2022
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 decimal
values. If there are none (i.e. if column->decimals is 0), we cast
to `Integer` instead.

Closes #36
composerinteralia added a commit that referenced this issue Nov 23, 2022
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 decimal
values. If there are none (i.e. if column->decimals is 0), we cast
to `Integer` instead.

Closes #36
composerinteralia added a commit that referenced this issue Nov 23, 2022
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
@dbussink
Copy link
Contributor

dbussink commented Jul 7, 2023

I'm wondering if this is a bug, or if I'm just missing a query option to cast the result to an Integer properly. At Shopify we're using Sorbet, and it complains loudly if a query ends up returning a BigDecimal when it was expecting an Integer.

Fwiw, this is part of how MySQL itself functions. A SUM() also on integer types always casts to DECIMAL since there's no guarantee otherwise that the result fits. A SUM over bigint columns could easily overflow a bigint.

This is different from COUNT for example which returns a bigint since that's also the maximum number of rows. Also see https://dev.mysql.com/doc/refman/8.0/en/aggregate-functions.html.

I'd argue to be honest here that Sorbet is wrong here. It should expect a DECIMAL like type for a SUM query and not really an integer. Anyway, the ship here probably long ago sailed on these changes.

@composerinteralia
Copy link
Contributor

I vaguely recall wanting to do this to match what the mysql2 gem does, to make it easier for folks switching over from that gem. We decided against making this change at GitHub and made it configurable in #60, in case that is helpful.

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

Successfully merging a pull request may close this issue.

3 participants