Skip to content

fix: use g17 format for doubles #434

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Dec 6, 2022
Merged

fix: use g17 format for doubles #434

merged 5 commits into from
Dec 6, 2022

Conversation

powersj
Copy link
Contributor

@powersj powersj commented Dec 5, 2022

fixes: #408

Proposed Changes

Updates the format used for doubles when building a Point to use the G17 format. This ensures 17 bits of precision and that values are successful in round-trips. This is the recommendation from the dot.net docs as well.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • dotnet test completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@powersj
Copy link
Contributor Author

powersj commented Dec 5, 2022

It looks like the tests are currently failing due to some handling of the floating points:

  Expected: "poco,tag=tag\\ val value=15.444,ValueWithEmptyName=25,ValueWit..."
  But was:  "poco,tag=tag\\ val value=15.444000000000001,ValueWithEmptyName..."

  Expected: "...,integer=7i,long=1i,point=13.3,sbyte=12i,short=8i,string="..."
  But was:  "...,integer=7i,long=1i,point=13.300000000000001,sbyte=12i,sho..."

  Expected: "...33,double16=16.3333333333333333,double17=17.33333333333333333"
  But was:  "...32,double16=16.333333333333332,double17=17.333333333333332"

This is a behavior change. Hopefully, someone does not depend on exact floating points, but I have concerns about this causing issues for existing users. I was hoping to have @bednar and @davidby-influx chime in on the change.

Thanks!

@davidby-influx
Copy link

My suggestion is to change the tests to round the got value to the precision of the expected value. There may be customers who depend exact floating point comparison, but that always leads to tears.

Something like:

expected = 2.33333

got = ComputedValue()

if Math.Round(got, 5) != expected
{
UnhappyNoises()
}

This may not fix everything (because midpoint rounding can cause problems, but it will fix a lot of the cases.

@powersj
Copy link
Contributor Author

powersj commented Dec 5, 2022

Thanks for the looking at the PR!

The tests are primarily comparing two strings of line protocol. I think in those cases if we are good with updates to the floating points and it sounds like we are, I can update the expected. I'll let this this sit over night and see if Jakub chimes in.

Thanks again, I appreciate the review.

@bednar
Copy link
Contributor

bednar commented Dec 6, 2022

It looks good for me 👍

@ashishusoni
Copy link

How will this work when we add a filter to compare
|> filter(fn: (r) => r["_value"] >= -180)
currently it throws an error something like below
Error –
{

"code": "invalid",
"message": "runtime error @6:6-6:44: filter: cannot compile @ 6:17-6:43: unsupported binary expression string >= int"

}

@powersj
Copy link
Contributor Author

powersj commented Dec 6, 2022

Thank you both - the failing tests are updated, formatting corrected, and readme rebased.

Copy link

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

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

LGTM

@powersj powersj merged commit c4b64ae into master Dec 6, 2022
@powersj powersj deleted the fix/408 branch December 6, 2022 21:00
@bednar bednar added this to the 4.9.0 milestone Dec 6, 2022
powersj added a commit that referenced this pull request Feb 15, 2023
powersj added a commit that referenced this pull request Feb 23, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conversion of double to string can result in a loss of precision
4 participants