[sqllogictest] Move decimal.rs tests
#5086
Conversation
github-actions
bot
added
core
There was a problem hiding this comment.
Thanks @melgenek -- I reviewed all the tests and it seems like they were ported correctly
I left a comment about how to port the precision / type checking as well, but I think that could easily be done as a follow on PR as well
fyi @liukun4515
| // array decimal(10,6) + scalar decimal(20,0) => decimal(21,6) | ||
| assert_eq!( | ||
| &DataType::Decimal128(27, 6), |
There was a problem hiding this comment.
I noticed that the comment expects a precision 21, but the assertion is 27. I am not sure if it's a typo in the comment, or if something is wrong with the type. I moved this test to decimal.slt as is.
| select arrow_typeof(abs(c1)) from decimal_simple limit 1; | ||
| ---- | ||
| Float64 |
There was a problem hiding this comment.
The original test didn't check the type of abs, but I added a test entry for it. It seems like a bug to me. Decimal(10,6) was converted to a Float64 as a result of abs call. It is probably related to #1335.
|
@alamb I updated this pr to include the type checks in sqllogictests using |
There was a problem hiding this comment.
This looks great -- thank you @melgenek
It is wonderful to see the momentum / cleanup going into the tests. I very much appreciate your efforts and I hope other members of the community will appreciate the development improvements that come from this work
|
Benchmark runs are scheduled for baseline = 3da801d and contender = 69678d5. 69678d5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #4495
Rationale for this change
Data-driven tests could be described in sqllogictest.
What changes are included in this PR?
Moves the tests that expect results from
decimal.rsto sqllogictest.Are these changes tested?
Github actions sqllogictest job.
Are there any user-facing changes?
No.