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.

Sign up for GitHub

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

Jump to bottom

Fix event value is truncated #15408

Merged
merged 4 commits into from
Feb 24, 2020
Merged

Fix event value is truncated #15408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
030ab7d
Update column type for custom float
tsteur Jan 16, 2020
9e474e5
use double instead of decimal
tsteur Jan 17, 2020
e51b803
fix test
tsteur Jan 17, 2020
db5b3e1
remove update to double
tsteur Feb 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/Db/Schema/Mysql.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ public function getTablesCreateSql()
idvisit BIGINT(10) UNSIGNED NOT NULL,
idaction_url_ref INTEGER(10) UNSIGNED NULL DEFAULT 0,
idaction_name_ref INTEGER(10) UNSIGNED NULL,
custom_float FLOAT NULL DEFAULT NULL,
custom_float DOUBLE NULL DEFAULT NULL,
PRIMARY KEY(idlink_va),
INDEX index_idvisit(idvisit)
) ENGINE=$engine DEFAULT CHARSET=utf8
Expand Down
14 changes: 7 additions & 7 deletions ...API/tests/System/expected/test_AutoSuggestAPITest_eventValue__VisitsSummary.get_range.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<?xml version="1.0" encoding="utf-8" ?>
<result>
<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 •
edited
Loading

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?

<nb_actions>20</nb_actions>
<nb_visits_converted>4</nb_visits_converted>
<bounce_count>0</bounce_count>
<sum_visit_length>0</sum_visit_length>
<max_actions>0</max_actions>
<sum_visit_length>6492</sum_visit_length>
<max_actions>5</max_actions>
<bounce_rate>0%</bounce_rate>
<nb_actions_per_visit>0</nb_actions_per_visit>
<avg_time_on_site>0</avg_time_on_site>
<nb_actions_per_visit>5</nb_actions_per_visit>
<avg_time_on_site>1623</avg_time_on_site>
</result>