https://www.getdaft.io logo
Join Slack
Powered by
got all unit tests to pass; fixing integration tes...
# daft-dev
a
got all unit tests to pass; fixing integration tests for my counts PR
are these equivalent physically?
we are trying to "assign" second type to an array with first physical type
d
The same platonic floating point value will have different physical representations for the two types
a
well this is decimal not floating point?
just repr by an integer
probs
?
d
Yeah that array conversion bug is an artifact from arrow2. It's one of the todos in the backlog
a
😭
d
I meant floating point as in the math number
But the integers are different under the hood
You essentially shift the integers left or right depending on the scale
a
what I am saying is 12-bit number does not exist
(10+2)
so i wonder what physical repr is
d
Under the hood they're all probably int128
a
interesting..
although weird it is 32,32 and not 64,64 imo
if I had to guess round trip is messing up
cause we have arrow Decimal(10,2) which is being converted to daft Int128 that is being converted back to Decimal(32,32)
I think at least
d
Yeah roundtrip is broken. Decimal(32, 32) means that the integral part of the decimal MUST be 0
So any decimal with a non-zero integral results in an error
a
ahhh
I was thinking Decimal(32,32) meant 32 bits left of 
.
and 32 bits right of it
still weird it is not 128, 128 then?
is each 
1
repr a nibble?
d
Would need to refresh my knowledge of decimal implementation to answer that
Anyway semantically you can refer to this
a
ok thanks
d
Decimal(precision, scale) - precision gives us total number of digits, scale gives us number of digits to the right of the decimal point
Oh wait it's digits. If you want to represent 32 digits you need to use at least 
log_2(10^32) ~= 106
bits, so it makes sense to use 128 bits
a
@jay could be useful to talk about how we should go about fixing this integration test
maybe tomorrow a ltitle?
d
Let's decouple this from the implementation of count values? Comment out decimal array test and re-enable when decimal arrays aren't broken?
a
It’s because PR also changes how map types work
j
I’m probably not the best to work with on this issue Grab someone tomorrow, keep it focused to 15 mins to decide whether or not we can fix this? Also ok if we skip for now and file and issue for a known bug so we can fix that separately.
@Desmond Cheong probably knows the most about the decimals but yeah I would timebox it to 15m, triage and figure out what needs to be done
👍 1
a
yea might be best to just triage
I thought about it more
I think really onlny way to properly fix it is for us to add a decimal type
5 Views
Open in Slack
PreviousNext
Powered by