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

Floating point precision differences after migrating from SDKv2 #815

Closed
jharley opened this issue Jul 31, 2023 · 6 comments · Fixed by #817
Closed

Floating point precision differences after migrating from SDKv2 #815

jharley opened this issue Jul 31, 2023 · 6 comments · Fixed by #817
Assignees
Labels
bug Something isn't working tf-devex-triage Terraform DevEx project tracking types Issues and pull requests about our types abstraction and implementations.
Milestone

Comments

@jharley
Copy link

jharley commented Jul 31, 2023

I recently migrated the honeycombio_trigger resource from the Plugin SDKv2 to the Plugin Framework and received a bug report that the way floating point numbers are being handled is differently.

I think this is not so much a 'bug' -- I can see increased precision as a feature! -- but I am looking for guidance on how to handle this case. A custom planmodifier?

Module version

github.com/hashicorp/terraform-plugin-framework v1.3.3

Relevant provider source code

https://github.com/honeycombio/terraform-provider-honeycombio/blob/main/internal/provider/trigger_resource.go#L132-L153

Terraform Configuration Files

data "honeycombio_query_specification" "test" {
  calculation {
    op     = "COUNT"
  }

  time_range = 1200
}

resource "honeycombio_trigger" "test" {
  name = "floating point test"

  dataset  = var.dataset
  query_id = data.honeycombio_query_specification.test.id

  threshold {
    op    = ">"
    value = 1 - 0.99
  }
}

Debug Output

https://gist.github.com/jharley/90ec9c38c70858c0e1bdcaaea0707c05

Expected Behavior

As with the SDKv2-based resource, it parses threshold[0].value as 0.01

Actual Behavior

       Error: Provider produced invalid plan
        
        Provider "registry.terraform.io/hashicorp/honeycombio" planned an invalid
        value for honeycombio_trigger.test.threshold[0].value: planned value
        cty.NumberFloatVal(0.01) does not match config value
        cty.MustParseNumberVal("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003").
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

Steps to Reproduce

terraform plan

Test to reproduce is on this branch: honeycombio/terraform-provider-honeycombio@467e603

References

Reported bug: honeycombio/terraform-provider-honeycombio#335

@bflad
Copy link
Member

bflad commented Aug 1, 2023

Hi @jharley 👋 Thank you for reporting this and sorry you are running into this unexpected situation here. The team is looking into this and we will try to provide an update soon.

@bflad bflad added the types Issues and pull requests about our types abstraction and implementations. label Aug 1, 2023
@bflad
Copy link
Member

bflad commented Aug 1, 2023

Can you also confirm your Terraform CLI version? From the debug logs I see:

Adding potential Terraform CLI source of releases.hashicorp.com exact version "1.0.11" for installation

But if you could confirm that, that would be great. It would also be helpful to try it out on the latest version (1.5.4 as of this writing), if it does happen to be that older version, just to confirm whether this is a bug that has resolved in Terraform itself.

@jharley
Copy link
Author

jharley commented Aug 1, 2023

The test suite is using 1.0.11 For Reasons™ via TF_ACC_TERRAFORM_VERSION

Confirmed the same behaviour with 1.5.4

@austinvalle austinvalle self-assigned this Aug 1, 2023
@austinvalle austinvalle added the tf-devex-triage Terraform DevEx project tracking label Aug 1, 2023
@bflad
Copy link
Member

bflad commented Aug 1, 2023

Terraform's number handling is arbitrary precision, meaning that the framework itself must preserve that precision when roundtripping a number value. The fix here will likely be a patch release of the terraform-plugin-framework Go module where the internals of the Float64 value storage will be switched from the float64 type to the *big.Float type to match that implementation detail. The existing functions for getting/setting values should remain the same though.

@austinvalle
Copy link
Member

austinvalle commented Aug 3, 2023

Hi there @jharley 👋🏻 , we've released a fix for this bug in terraform-plugin-framework v1.3.4.

One behavior to be aware of is that Plugin SDKv2 has legacy data consistency handling which would allow the following config to be stored in state as 0.01:

resource “examplecloud_resource” “test” {
   float64_attribute = 1 - 0.99
}

When you migrate a resource like this to Plugin Framework, the prior value of 0.01 will be preserved in state. If you were to delete/recreate this resource, the value stored in state will be exactly what is in config, 0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003.

Providers are allowed to store config exactly or store prior state exactly, so either of these situations are valid 👍🏻

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working tf-devex-triage Terraform DevEx project tracking types Issues and pull requests about our types abstraction and implementations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants