Re: [Tracker] Review of wip/garnacho/sparql1.1 on gnome's git

[Carlos Garnacho <carlosg gnome org>]

Hey Philip,

Thanks a lot for the review :), very appreciated despite the late reply...

I've now branched for 1.6 and partially merged this branch on master,
more replies below

On Mon, Aug 31, 2015 at 8:42 PM, Philip Van Hoof <philip codeminded be> wrote:
> Hi Carlos,
>
> Great work on the SPARQL1.1 support. Here is my review.
>
> 77dbe7a8847511f56563b50c65599dbfc14453ed: ok
> 23109a5c8744d057cff19ee6f767314a4a761ebd: ok
> 3a0f7a8317f0b8c97bd3c7305bf5386484a027b9: ok
> 18fe5f9c04a433d7a11cfff255893956f4383b0c: ok
> d4869094aa0b1a76acc384ec7a7a352fe8b79abe: ok
> 4c0a58ead3d450bb295cb1ffefe74fc6358e07d2: ok
> 6e193ec049f789bbb7e0f717d86b92425bfe0109: ok
> 860031689066aaa12bf7dd1c5d9b969da5c7666f: ok

These are now on master

> 40dbb5123894bd6fe473475314cd5793a0db27ed: ok, with questions
>   SparqlEncodeForUri and ENCODE_FOR_URI can't support URLs with UTF-8
>   encoded characters?, FALSE is used with g_uri_escape_string

ENCODE_FOR_URI docs mostly defer to fn:encode-for-uri:
http://www.w3.org/TR/sparql11-query/#func-encode
http://www.w3.org/TR/xpath-functions/#func-encode-for-uri

There it seems to be quite specific about alphabetic characters being
just in the range of [a-zA-Z] (see also the "bébé" example). Using
false there seems consistent with that, and the way we end up storing
nie:url on utf8 filenames.

This ended up in master as well.

> d92eb74f63bb5307f22c378c2d74d1902e47ee47: ok
> 1d20e38f4cfde75d4ee9d966d4226b40e30cdce2: ok but
>
> For these:
> + sqlite3_result_text (context, g_strdup (loc + len), -1, g_free);
>
> I think that with sqlite we can use this too:
>
> + sqlite3_result_text (context, loc + len, -1, NULL);
>
> loc (which comes from that str ptr) indeed lives in the context of
> sqlite, but afaik it wont disappear while the consumer of the sqlite3
> result can legally consume it. We'd need to check this. But it would
> avoid a strdup, which is always a good thing for performance.

Indeed :), fixed and pushed to master.

> 8766a6fdac7f4f65e492169b8d3b6b871c58ab6d: ok
> 2d5912c2604f55bf5aa679925aa36ff21a07b982: ok
> 1e4b8992d473879307d9d7a52fb8d9647779686e: ok

These are in master too

> afbef87a54814099d5ebde4589ef01530ed7fe52: ok but:
> What is wrong with this "select random()" in sqlite? Not sure why we are
> making SparqlRand here. Not that I'm against it, but if sqlite's
> standard random() works, then why not use it?
> 27be3a97e8fd8478c66ca766685f3a9377ce009d: ok but:
>
> So where we could also just use something like this then, I guess
> (perhaps cast it to double if that's what sparql1.1 wants):
>
> + expect (SparqlTokenType.CLOSE_PARENS);
> + sql.append ("random()");
> + return PropertyType.DOUBLE;

It's not just the casting to double, sqlite random() defines the
return value as "integer between -9223372036854775808 and
+9223372036854775807", while sparql defines it as "xsd:double between
0 (inclusive) and 1.0e0 (exclusive)", I could fix the range within the
query, but it's not completely obvious... otoh, g_random_double()
works as expected :).

I've left this patch aside nonetheless, still in wip/garnacho/sparql1.1

> 3634e54d333c153a7e32f6c73c1ef58e7f35fd07: nok:
>
> strftime's %s is seconds since epoch,

Yes, notice that it returns a PropertyType.DATETIME, so it must be a
timestamp. Timedate values usually go through our SparqlFormatTime
sqlite function before making it to the user, so this query:

  SELECT NOW() {}

gets transformed into this sql:

  SELECT SparqlFormatTime (strftime ('%s', 'now')) FROM (SELECT 1)

> https://www.sqlite.org/lang_datefunc.html :
>
> %s
>
> seconds since 1970-01-01
>
> I would use
>
> YYYY-MM-DDTHH:MM:SS.SSS-HH:MM
>
> I also don't think you are using local time with the time string 'now',
> the sqlite documentation stipulates that you'll use UTC. When I read the
> sparql1.1 spec, it specifically mentions the HH:MM local time
> information in the example. So you'll need to use the localtime (nr. 12)
> modifier somewhere.

Ok, this is convoluted... that query above returns

2015-10-19T12:40:47Z

Which is AFAICT the correct UTC time at the time of the query, I agree
that returning "2015-10-19T14:40:47-02:00" would be nicer, but I think
it comes down to SparqlFormatTime, and tracker_date_to_string() used
underneath.

> f5a09f2db0d5b3e2fd869299750a953eb9fb4b97: ok, here localtime is
> respected :). /me happy
>
> b2735b8809df41669092d970d83ba400b8d5d757: ok. neat.

these are now in master :),

> 2942e4e888b381b05f206544b711a9a44b29b747: nok, no check for the string
> checksumstr while you do have a check for str (fine, the else case will
> happen) but .. if str is NULL shouldn't you do sqlite_result_error?

Hmm, I think that's mostly a paranoia check, you should get "" at
least there, or the argc in the query would be different. It is more
open to debate whether we should allow empty strings at certain places
though.

I've added the basic check for checksumstr and pushed to master.

> I see this often in the custom sparql functions of
> tracker-db-interface-sqlite.c. Perhaps we should fix them all?

Right, we should indeed issue error on those paths.

> 8e80adeea0b60bea1d66729d3e726e70149f3e1a: ok
> d9851cfdaf483c4eb11533cc175bbc40bda276dd: ok

Those are now on master :).

So, I've pushed again wip/garnacho/sparql1.1 with the remainings and a
few new patches:

2d56a49 libtracker-miner: Optimize move operations

  This one is the optimization I wanted to do in the first place :),
uses DELETE...INSERT...WHERE and BIND so recursive nie:url
modifications on move operations are done in a single query.

ca5d939 libtracker-data: Add support for BIND
e825bf2 libtracker-data: Resolve variables looking up the binding if
we're in same context
2a5a555 libtracker-data: Relate Variable objects to their origin Context

  These implement BIND support.

b4257e6 libtracker-data: Implement sparql1.1 delete/insert

  You seemed to miss this one in the previous iteration :)

1ab3cbc libtracker-data: Add support for NOW() builtin function
7678fad libtracker-data: Add support for RAND() builtin function
e00afca libtracker-data: Add SparqlRand sqlite function

  These are the patches dropped in this review. Pending decision, but
no changes.

Cheers,
  Carlos