After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 659600 - DELETE fails with certain xsd:double values
DELETE fails with certain xsd:double values
Status: RESOLVED FIXED
Product: tracker
Classification: Core
Component: Store
git master
Other Linux
: Normal normal
: ---
Assigned To: tracker-general
Jamie McCracken
Depends on:
Blocks:
 
 
Reported: 2011-09-20 15:07 UTC by Sam Thursfield
Modified: 2019-03-02 20:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-Do-not-CAST-insert-delete-solutions-of-type-DOUBLE-t.patch (32.73 KB, patch)
2017-05-11 08:44 UTC, Jose Miguel Arroyo
needs-work Details | Review

Description Sam Thursfield 2011-09-20 15:07:10 UTC
Here's an interesting bug I just came across ..

INSERT { <urn:test> a nfo:Video ; nfo:frameRate 1.0 }
DELETE { <urn:test> nfo:frameRate ?unknown } WHERE { <urn:test> nfo:frameRate ?unknown }
SELECT ?c { <urn:test> nfo:frameRate ?c }

Results:
  None


INSERT {<urn:test> a nfo:Video ; nfo:frameRate 1.1 }
DELETE { <urn:test> nfo:frameRate ?unknown } WHERE { <urn:test> nfo:frameRate ?unknown }
SELECT ?c { <urn:test> nfo:frameRate ?c }

Results:
  None


INSERT {<urn:test> a nfo:Video ; nfo:frameRate 0.333333333333333333333 }
DELETE { <urn:test> nfo:frameRate ?unknown } WHERE { <urn:test> nfo:frameRate ?unknown }"
SELECT ?c { <urn:test> nfo:frameRate ?c }

Results:
  0.333333333333333


Presumably this is a floating point comparison issue coming up somewhere along the line so the WHERE clause does not have any solutions.
Comment 1 Jose Miguel Arroyo 2017-05-09 16:27:53 UTC
I looked into this issue and I found that there is a clash between Tracker's string-to-double conversions and SQLite's float-to-string conversions during the execution of insert delete statements when there are "Objects" of type TRACKER_PROPERTY_TYPE_DOUBLE involved.

During an INSERT like:
INSERT {<urn:test> a nfo:Video ; nfo:frameRate 0.333333333333333333333 }

cache_insert_metadata_decomposed() will eventually be called for the framerate property. Inside this call, the "0.333333333333333333333" is converted to a "double" using atof() and the resulting value is set to a gvalue, using string_to_gvalue(), which is then binded (as a double) to the SQLite statement to insert the property.

Which means that this double value is inserted as is into the DB by sqlite:
SQLITE_PRIVATE void sqlite3VdbeMemSetDouble(Mem *pMem, double val){
  sqlite3VdbeMemSetNull(pMem);
  if( !sqlite3IsNaN(val) ){
    pMem->u.r = val;
    pMem->flags = MEM_Real;
  }
}

For this example, atof("0.333333333333333333333") gives me 0.33333333333333331 according to gdb or x/x 0x55	0x55	0x55	0x55	0x55	0x55	0xd5	0x3f.

However, when we use the query:
DELETE { <urn:test> nfo:frameRate ?unknown } WHERE { <urn:test> nfo:frameRate ?unknown }"
to delete this property, the sparql query will be translated to the following SQL expression:
"SELECT CAST (\"1_u\" AS TEXT) FROM (SELECT \"nfo:Video1\".\"nfo:frameRate\" AS \"1_u\" FROM \"nfo:Video\" AS \"nfo:Video1\" WHERE \"1_u\" IS NOT NULL AND \"nfo:Video1\".\"ID\" = (SELE
CT ID FROM Resource WHERE Uri = ?))"

Where the nfo:frameRate is casted to TEXT. This is done in tracker-sparql-expression.vala in function convert_expression_to_string().

Later on, after the binding of the URI is done and the cursor is started, solutions are gotten invariably as strings inside tracker-sparql-query.vala in tracker_sparql_query_execute_insert_delete().
...
while (cursor.next ()) {
        // get values of all variables to be bound
        for (var_idx = 0; var_idx < solution.hash.size (); var_idx++) {
                solution.values.add (cursor.get_string (var_idx));
        }
        n_solutions++;
}
...

It is somewhat unclear, but from the online documentation and reading the SQLite code (https://www.sqlite.org/datatype3.html#affinity): "For conversions between TEXT and REAL storage classes, SQLite considers the conversion to be lossless and reversible if the first 15 significant decimal digits of the number are preserved."

What internally happens is that Floating Point numbers casted to text are approximated to 15 significant decimal digits.

This means that the solution stored at this point will be "0.333333333333333" which is different from the actually stored value (0.33333333333333331).


Afterwards, the actual deletion is attempted inside tracker-data-update.c in delete_metadata_decomposed().

Here we first get the current value in the DB for the given TrackerProperty and store it in old_values. In this case, the real value is recovered from the DB entry (0.33333333333333331).

Afterwards, our previous solution value is converted to a gvalue using string_to_gvalue() and it's property type. This convertion ends up being a single call to atof().

However, our previous solution value is not the real value in DB, it has less precision, so the atof("0.333333333333333") call returns a different value than the one stored in the DB.

Given that the solution value casted into double and the value in the DB are different, value_set_remove_value (old_values, &gvalue) returns FALSE as if the value hasn't been found and no DELETE is made.
Comment 2 Jose Miguel Arroyo 2017-05-09 16:38:12 UTC
Which is a very long-winded way of saying that:

When deleting properties, during the resolution of valid solutions to the SQL statement gotten from the SPARQL query, any solutions of type Double are casted into TEXT.

This cast approximates to at most 15 significant decimal digits, so information is lost.

Afterwards during the comparison from found solutions and old values in the tables, the value in DB is different from the truncated value so no DELETE is performed.
Comment 3 Jose Miguel Arroyo 2017-05-11 08:43:08 UTC
I dived into the code to try and find ways in which this issue could be handled.

I see two possible solutions:

1.- Use GValues when finding solutions and query their type before setting the GValue.
2.- When inside 'delete_metadata_decomposed()' and getting the old property values via 'get_old_property_values()', matching properties of type xsd:double should be cast by sqlite3 to text and do the comparison matching these strings.


The advantage of solution 2 is that it requires a lot less code modifications in better contained places, at least at a first glance. The main disadvantage to this solution is that Deletes of doubles loses precision and may give some false positives.

E.g: Something like this:
INSERT {<urn:test> a nfo:Video ; nfo:frameRate 0.333333333333333333333 }
DELETE { <urn:test> nfo:frameRate 0.3333333333333333355 } WHERE { <urn:test> a rdfs:Resource }"

Would actually delete the nfo:frameRate entry, which may not be acceptable behaviour.

On the other hand, solution 1 handles the issue correctly, but requires much more extensive code modifications at various places.

I came up with a very quick&dirty patch mainly to show the extent of what this kind of solution would require.

It still probably needs a lot of polishing (I didn't pay too much attention to the handling of journal entries, for example, and the checking of the solution type could maybe be reworked) and I didn't test it extensively, but at least it handles this specific corner case.
Comment 4 Jose Miguel Arroyo 2017-05-11 08:44:07 UTC
Created attachment 351606 [details] [review]
0001-Do-not-CAST-insert-delete-solutions-of-type-DOUBLE-t.patch
Comment 5 Carlos Garnacho 2019-03-02 20:07:01 UTC
This was a problem thus far... till now :).

I do agree that eventually it would be nicer to move tracker_data_*_statement() to using GValues, however the necessary changes run too deep. I question the short/mid term future of the journal, after that it should get considerably easier to reconsider this.

In the meantime, I went somewhat the other way around, and ensured the solution gets a string with a full precision double (i.e. not using sqlite to cast to string). Less nice, but at least we can call this fixed.

See https://gitlab.gnome.org/GNOME/tracker/merge_requests/69