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

Fix event value is truncated #15408

Merged
merged 4 commits into from Feb 24, 2020
Merged

Fix event value is truncated #15408

merged 4 commits into from Feb 24, 2020

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Jan 16, 2020

fix #14934

should require 2 bytes for the right part, and about 7 byes for the left part.

@tsteur tsteur added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Jan 16, 2020
@tsteur tsteur added this to the 4.0.0 milestone Jan 16, 2020
@tsteur
Copy link
Member Author

tsteur commented Jan 17, 2020

FYI Using decimal it causes the values to appear like this: https://travis-ci.org/matomo-org/matomo/jobs/638207041#L5670

We can workaround this by using eg TRIM(TRAILING '0' FROM custom_float) AFAIK but adds some additional overhead. Decided to simply switch to DOUBLE which should store most values accurately as well if there are no objections while needing same amount of storage basically. Should be also better for comparisons etc.

<nb_visits>0</nb_visits>
<nb_actions>0</nb_actions>
<nb_visits_converted>0</nb_visits_converted>
<nb_visits>4</nb_visits>
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this test didn't used to work. Ran the test with float and double and in both cases the value looked like they were stored correctly.
This is what the fixture data looks like
image

Copy link
Member

Choose a reason for hiding this comment

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

It didn't work as the comparison custom_float = '345.678' doesn't return any results when the field is a float. The number seems not to be stored accurate enough to be able to compare it correctly. When changing the column to DOUBLE it gets converted to 345.6780090332031.

Not sure if it would make sense to truncate some minor fraction digits after changing the type 🤔

Using a DOUBLE the number is stored accurate enough to be compared correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

Could use something like DOUBLE(15,5) or so and limit it to 5 numbers after the dot. I just tested it and the problem is that then a similar issue as with decimal happens that all numbers are formatted to have 5 digits after dot
image

Copy link
Member

Choose a reason for hiding this comment

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

Could we maybe additionally run a script like UPDATE log_link_visit_action SET custom_float = TRUNCATE(custom_float, 5) after it was converted to double? That should throw away all fraction digits after the fifth one 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@sgiehl from what I see mysql stores also new values like this.

and truncate seems to ensure there's always that many zeros too?
image

Copy link
Member Author

Choose a reason for hiding this comment

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

@sgiehl not sure I understand this fully? If we just use DOUBLE then this shouldn't be needed?

Copy link
Member

Choose a reason for hiding this comment

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

When we currently have a value like 345.678 and the field is converted to DOUBLE, the new value will be something like 345.6780090332031. That's the reason why I suggested to run something like a TRUNCATE, so we have a similar value as before

Copy link
Member Author

Choose a reason for hiding this comment

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

Which MySQL version is that? This wasn't happening for me on MySQL 8. Or maybe I'm not checking correctly?

Copy link
Member

Choose a reason for hiding this comment

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

mysql 5.7

Copy link
Member Author

@tsteur tsteur Feb 20, 2020

Choose a reason for hiding this comment

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

I see, can reproduce it on 10.2-MariaDB. I just created 3 rows with values 3, 4.1 and 4.12. After converting to float I got this:

image

After executing your suggested query I got this:

image

So it's still changing the values quite a bit.

It seems there might not be a safe way to change the field? I wonder if it's worth it or maybe we rather create an FAQ for people needing more accuracy and only try to change it for new installs.

@mattab there doesn't seem to be an easy way to change the field for this column. I reckon we could really rather only change it for new installs maybe and create an FAQ for all other cases. any thoughts?

@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jan 17, 2020
@sgiehl
Copy link
Member

sgiehl commented Feb 12, 2020

while needing same amount of storage basically

Doesn't DOUBLE need the double amount of space than FLOAT 🤔

According to https://dev.mysql.com/doc/refman/8.0/en/floating-point-types.html it uses 8 instead of 4 bytes.

But nevertheless, the numbers will get stored more accurate...

@tsteur
Copy link
Member Author

tsteur commented Feb 12, 2020

while needing same amount of storage basically

That was compared to the decimal alternative, not compared to float.

@tsteur
Copy link
Member Author

tsteur commented Feb 23, 2020

@sgiehl as discussed changing the type only for new installs and created FAQ for existing installs https://matomo.org/faq/how-to/how-do-i-fix-a-truncated-event-value/

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Looks good now. Seems all failing tests are because the branch is a bit outdated...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS Tracker: trackEvent bug, event value is truncated
2 participants