Re: [Tracker] Anonymous File Nodes branch review

[Martyn Russell <martyn lanedo com>]

On 03/02/10 12:51, Philip Van Hoof wrote:
> On Wed, 2010-02-03 at 12:08 +0000, Martyn Russell wrote:
> > -       tracker_sparql_builder_object_iri (sparql, uri);
> > +       tracker_sparql_builder_object (sparql, "_:foo");
>
> Looks right to me

Just to repeat what I said on IRC:

Technically there is nothing wrong with "_:foo", however, the "foo" part 
is used in the generation of the id and it isn't professional or setting 
a good example to other apps that copy our code - we should use 
something a little more self explanatory IMO.

> > Shouldn't that be \"nie:DataObject\" ?do . \"nie:url\" ...
>
> Don't be confused, this isn't a SPARQL query but a native SQLite query.
>
> There's a dot before "nie:url" because "nie:DataObject" is the table
> name and "nie:url" is the column name. Without that dot it would not be
> a correct native SQLite query.

Ah yes, of course :) thanks. The rules all change when it becomes SQL :D

> > 6. I think we should add proper error handling here:
> >
> > +          /* For fallback to the old behaviour, we could do this here:
> > +           * resource_id = tracker_data_query_resource_id (url); */
> > +          return;
>
> No there's no error in that case. It just means that no removing needs
> to take place (we can silently ignore it).

OK, I got the impression from the code there was, but if this is normal 
then good :)

> > 7. Hmm, not sure why this change was added, usually if result is set and
> >   >  0 in length then there was no error, checking that here seems
> > pointless to me as we would still check the array and its length even if
> > there was no error so? This is in
> > src/tracker-writeback/tracker-writeback-consumer.c in sparql_query_cb().
> >
> > -       if (result&&  result->len>  0) {
> > +       if (!error&&  result&&  result->len>  0) {
>
>
> I had the situation where error was set and result wasn't NULL. I think
> result is left untouched in case error is set, and if then the stack
> variable isn't cleared to NULL, error isn't NULL. So just checking for
> result isn't sufficient.

I see. Generally in this case I handle error in a separate situation 
then, i.e. if (error) { ...; return; } and do the result stuff after, 
but if this fixed a problem great.

> > 8. Again, I would make queries like this easier to read for maintainability:
>
> What is hard about this? Note that only the "'%s'" and the
> "nie:url ?url" got added tot he original query.

Nothing hard at all, it is simply quicker and easier to read and that's 
always an improvement for maintainability.

> > + query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
> > +                          "<%s>  ?predicate ?object ; nie:url ?url ."
> > +                          "  ?predicate tracker:writeback true "
> > +                          "}", data->subject, data->subject);

To me this:

query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
                          "  <%s> ?predicate ?object ;"
                          "  nie:url ?url ."
                          "  ?predicate tracker:writeback true "
                          "}",
                          data->subject,
                          data->subject);

is clearer than this:

query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
                          "<%s>  ?predicate ?object ; nie:url ?url ."
                          "  ?predicate tracker:writeback true "
                          "}", data->subject, data->subject);

> > Also, I presume this is valid SPARQL to put '%s' after ?url, looks to me
>
> Yes, that's correct SPARQL

OK, I assumed it was.

> > like it will just be included as one of the columns in the results -
> > isn't that a little inefficient (depending on how many results are
> > returned).
>
> There's no inefficiency caused by this. And doing it differently will
> break the writeback modules and make things *a lot* harder to port and
> support (all writeback modules need a rewrite).

I see, but really we only have a few now so sooner is better than later 
to fix something like this. It is only an efficiency. Really if we 
expect a lot of results to be returned here we might want to consider 
changing this.

--
Regards,
Martyn