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
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
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.
8766a6fdac7f4f65e492169b8d3b6b871c58ab6d: ok
2d5912c2604f55bf5aa679925aa36ff21a07b982: ok
1e4b8992d473879307d9d7a52fb8d9647779686e: ok
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;
3634e54d333c153a7e32f6c73c1ef58e7f35fd07: nok:
strftime's %s is seconds since epoch,
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.
f5a09f2db0d5b3e2fd869299750a953eb9fb4b97: ok, here localtime is
respected :). /me happy
b2735b8809df41669092d970d83ba400b8d5d757: ok. neat.
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?
I see this often in the custom sparql functions of
tracker-db-interface-sqlite.c. Perhaps we should fix them all?
8e80adeea0b60bea1d66729d3e726e70149f3e1a: ok
d9851cfdaf483c4eb11533cc175bbc40bda276dd: ok
Kind regards,
Philip
Attachment:
signature.asc
Description: This is a digitally signed message part