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

fix: New decimal implementation compatible with Arrow's. #945

Merged
merged 1 commit into from
May 3, 2023

Conversation

vrongmeal
Copy link
Contributor

Fixes: #918

@vrongmeal vrongmeal requested a review from scsmithr May 3, 2023 20:44
@vrongmeal vrongmeal force-pushed the vrongmeal/decimal branch from 71223c4 to cb5d2c3 Compare May 3, 2023 20:44
Copy link
Member

@scsmithr scsmithr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Queries worked for me (except 14, that just has an extra line):

~/Code/github.com/glaredb/glaredb % ./scripts/run-tpch-queries.sh
SCHEMA: tpch_10353
CREATE TABLE
query 1
Time: 22142.523 ms (00:22.143)
query 2
Time: 6037.794 ms (00:06.038)
query 3
Time: 6252.224 ms (00:06.252)
query 4
Time: 3392.729 ms (00:03.393)
query 5
Time: 7645.839 ms (00:07.646)
query 6
Time: 3925.391 ms (00:03.925)
query 7
Time: 9323.585 ms (00:09.324)
query 8
Time: 10622.482 ms (00:10.622)
query 9
Time: 10279.361 ms (00:10.279)
query 10
Time: 7669.660 ms (00:07.670)
query 11
Time: 4858.016 ms (00:04.858)
query 12
Time: 5218.854 ms (00:05.219)
query 13
Time: 6029.353 ms (00:06.029)
query 14
Time: 4424.038 ms (00:04.424)
psql:./testdata/tpch/14.sql:17: ERROR:  sql parser error: Expected an SQL statement, found: first
Time: 0.236 ms
query 15
Time: 668.985 ms
Time: 5862.500 ms (00:05.863)
Time: 22.914 ms
query 16
Time: 3175.444 ms (00:03.175)
query 17
Time: 7716.816 ms (00:07.717)
query 18
Time: 9586.670 ms (00:09.587)
query 19
Time: 7326.894 ms (00:07.327)
query 20
Time: 7683.453 ms (00:07.683)
query 21
Time: 10256.634 ms (00:10.257)
query 22
Time: 2705.241 ms (00:02.705)
DROP SCHEMA

Comment on lines +502 to +505
// buf.clear();
// let decimal = Decimal128::new(3950123456, 6).unwrap();
// Writer::write_decimal(buf, &decimal).unwrap();
// assert_buf(buf, &[0, 3, 0, 0, 0, 0, 0, 6, 15, 110, 4, 210, 21, 224]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this Decimal128 doesn't implement ToSql yet. Once that's done, i can go ahead with adding NUMERIC support to postgres datasource as well.

@vrongmeal vrongmeal force-pushed the vrongmeal/decimal branch from cb5d2c3 to 72857e3 Compare May 3, 2023 21:01
@vrongmeal vrongmeal enabled auto-merge (squash) May 3, 2023 21:03
@vrongmeal vrongmeal force-pushed the vrongmeal/decimal branch from 72857e3 to 8a862e4 Compare May 3, 2023 21:04
@vrongmeal vrongmeal merged commit 41a3cbe into main May 3, 2023
@vrongmeal vrongmeal deleted the vrongmeal/decimal branch May 3, 2023 21:18
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 this pull request may close these issues.

Panic with converting from decimal
2 participants