Re: [Tracker] Ready for review - Was: Ontology change support upgrading nrl:maxCardinality 1 to multiple

[Philip Van Hoof <philip codeminded be>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 29/09/2014 12:30, Martyn Russell wrote:
> On 26/09/14 16:09, Philip Van Hoof wrote:
> > Here you go, that makes reviewing easier :-)
> >
> > https://git.gnome.org/browse/tracker/log/?h=maxcardinality-change-support-rebased
>
> >
> Thanks Philip,
>
> Comments:
>
> > +            g_debug("Dealth with allowed nrl:maxCardinality
> > change for %s's %s", +
> > tracker_class_get_name(class), +
> > tracker_property_get_name(property));
>
> Is this debug statement a typo?

Yes, it can be removed.

> > @@ -2394,12 +2427,14 @@ db_get_static_data (TrackerDBInterface
> > *iface,
> >
> > tracker_property_set_is_new_domain_index (property, 
> > tracker_ontologies_get_class_by_uri (domain_uri), FALSE); 
> > tracker_property_set_is_new (property, FALSE); +
> > tracker_property_set_cardinality_changed (property, FALSE); 
> > tracker_property_set_transient (property, transient); 
> > tracker_property_set_uri (property, uri); tracker_property_set_id
> > (property, id); tracker_property_set_domain (property, 
> > tracker_ontologies_get_class_by_uri (domain_uri)); 
> > tracker_property_set_range (property, 
> > tracker_ontologies_get_class_by_uri (range_uri)); 
> > tracker_property_set_multiple_values (property, multi_valued); +
> > tracker_property_set_orig_multiple_values (property, 
> > multi_valued); tracker_property_set_indexed (property, indexed); 
> > tracker_property_set_default_value (property, default_value); 
> > tracker_property_set_force_journal (property, force_journal);
>
> Hmm, shouldn't the _set_cardinality_changed() API not be FALSE but
> be based on orig_multiple_values != multiple_values ?

Not sure what you mean?
> > @@ -645,10 +651,12 @@ tracker_data_ontology_load_statement
> > (const gchar *ontology_path, 
> > tracker_property_reset_domain_indexes (property); 
> > tracker_property_reset_super_properties (property); 
> > tracker_property_set_indexed (property, FALSE); +
> > tracker_property_set_cardinality_changed (property, FALSE); 
> > tracker_property_set_secondary_index (property, NULL); 
> > tracker_property_set_writeback (property, FALSE);
> >
> > tracker_property_set_is_inverse_functional_property (property,
> > FALSE); tracker_property_set_default_value (property, NULL); +
> > tracker_property_set_multiple_values (property, TRUE);
>
> Just wondering why you set multiple_values to TRUE here... is that
> the default with no cardinality statement? If so, ignore me :)

Yes, it's the default: no value filled in means TRUE for the meaning
of "has multiple values" in case of maxCardinality. This is the place
where those defaults are defined. In case of "has multiple values" the
default, therefore, is true (and not false like with many others where
lack of the property being filled in in the ontology does mean false,
not so here. It not being filled in means true - for has-multiple-values).

> > @@ -1922,7 +1955,7 @@ tracker_data_ontology_process_statement
> > (const gchar *graph, } else if (g_strcmp0 (predicate,
> > RDFS_SUB_PROPERTY_OF) == 0          || g_strcmp0 (predicate,
> > RDFS_DOMAIN) == 0                   || g_strcmp0 (predicate,
> > RDFS_RANGE) == 0                    || -               g_strcmp0
> > (predicate, NRL_MAX_CARDINALITY) == 0           || +
> > /* g_strcmp0 (predicate, NRL_MAX_CARDINALITY) == 0        || */ 
> > g_strcmp0 (predicate, TRACKER_PREFIX "indexed") == 0      || 
> > g_strcmp0 (predicate, TRACKER_PREFIX "transient") == 0    || 
> > g_strcmp0 (predicate, TRACKER_PREFIX "fulltextIndexed") == 0) {
>
> Just to clarify, it looks like before it was ignoring any
> cardinality changes where by the change an update and now we
> don't.

I'm not sure about this g_strcmp0 myself. I don't really understand
what that code is doing and why it's doing ignores. I was hoping Jürg
could enlighten me here :)

I was planning to test without commenting this out and if that works,
don't comment it out here.

> > @@ -2576,7 +2612,7 @@ create_decomposed_metadata_property_table 
> > (TrackerDBInterface *iface, service_name, field_name); }
> >
> > -            if (in_change && !tracker_property_get_is_new
> > (property)) { +            if (in_change &&
> > !tracker_property_get_is_new (property) &&
> > !tracker_property_get_cardinality_changed (property)) {
>
> Presumably, the check here is because we don't support going from 
> multivalued to single valued in this code block and we protect
> ourselves against that...?

Correct.

> I wonder if we should be using the GError for that case, because
> it shouldn't happen normally should it?

Such a change in the ontology should result in an error passed on to
the initialization of the db, so that that part can decide to remove
meta.db and restart it.

And/or do abort() instead.

Kind regards,

Philip


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (MingW32)

iQEcBAEBAgAGBQJUKW1RAAoJEEP2NSGEz4aDTwEH/jLlKICpB+15sR/HF+pPdHBc
AQRiZJFkuwu+K4dAwd3617YA/J1ixMRJa8UaZK5c9Z1yh0s1/knr9SAbgZNLTqo8
grLO3uE+hakM70SC6cHbZGPLwt9Y3x/Bx4dM91kCy2gaoxQxI9uj87zG77/PT7tu
ZB4Cyfph5xDmyYek9KH0W8NWeT9sCB8gpdfaDqYh9beL5cBVzfPIr4JoUKttlX/H
NVUEcwuYgvnxSDRh6bIPgB3xfIfLVQAj5/jKMQq85Va+iYHX8pxJqMSGaB3GGQai
+6tMddMtbWvomdFiYAakDPb+8AUSXxWzMQ1R5XmWOfLk9P6UY06sj5ED1Q5lOYE=
=Qlw6
-----END PGP SIGNATURE-----