gosnowflake icon indicating copy to clipboard operation
gosnowflake copied to clipboard

Published 20 hours ago • verified publisher icon snowflakedb

Fixes floats from being truncated to 7 digit precision

Open tampajohn opened this issue 2 years ago • 4 comments

Description

#453 introduced a bug with how float64 values are handled. Since float64 is a native type, there is no reason to utilise the higherPrecision check. The string formatting that was being utilised was truncating float values past the 7th decimal place.

This PR reverts the change that caused the issue.

Checklist

  • [x] Code compiles correctly
  • [x] Run make fmt to fix inconsistent formats
  • [x] Run make lint to get lint errors and fix all of them
  • [ ] Created tests which fail without the change (if possible)
  • [x] All tests passing
  • [ ] Extended the README / documentation, if necessary

tampajohn avatar Sep 09 '22 21:09 tampajohn

@sfc-gh-jbahk It seems like the tests failed for some unrelated reason?

tampajohn avatar Sep 12 '22 17:09 tampajohn

@sfc-gh-jbahk It seems like the tests failed for some unrelated reason?

@sfc-gh-jbahk @sfc-gh-igarish could you please have another look and get this merged?

srabraham avatar Sep 13 '22 13:09 srabraham

Here what I tried:

  1. Get the changes of this PR and created a new PR. This doesn't have CLA Assist issue. But still it has test failures. Look in to test failures and it's related to this change. I discuss with Harsh and Nate. This is going to be a breaking change as currently without high precision context, it expect float in a String. So we suggest to use WithHigherPrecision context during query execution. e.g. "rows := dbt.mustQueryContext( WithHigherPrecision(context.Background()), selectVariousTypes)". This will get you float64 always and it will have high precision value.

sfc-gh-igarish avatar Sep 15 '22 20:09 sfc-gh-igarish

I guess what I don't get is that #453 introduced this breaking change, as well as the test to support the change of float -> string bug.

Is this truly a breaking change, or is this actually a bug fix?

Change referenced: https://github.com/snowflakedb/gosnowflake/pull/453/files#diff-ecad48054a05ca5aecf42b278530331772747ed7211a193aacd94214cb53aad0R427

Test that was added: https://github.com/snowflakedb/gosnowflake/pull/453/files#diff-89d7a56d38f9f263a0c8f47053c091b9808cb45e9ac35f834275a860e8f9b2fcR292

Looks like this bug introduced the expectation that the resulting value should be a string?

tampajohn avatar Sep 16 '22 14:09 tampajohn