709962 - Eliminate required column references in browser content provider query interface

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Richard Newman [:rnewman]]

Follow up from email and IRC: the browser content provider LEFT OUTER JOINs bookmarks to images, which requires all use of the sync columns (guid, last modified, date created) to be qualified, which is confusing for callers.

---
Check this out:

    12-09 15:05:07.770: E/DatabaseUtils(737): android.database.sqlite.SQLiteException: ambiguous column name: guid: , while compiling: SELECT bookmarks.guid AS guid, position, folder, title, thumbnail, bookmarks._id AS _id, bookmarks.created AS created, favicon, parent, url, bookmarks.modified AS modified FROM bookmarks LEFT OUTER JOIN images ON bookmarks.url = images.url_key WHERE (guid in ('G4uul34cGNiO')) ORDER BY folder DESC, position ASC, _id ASC


This comes from

    context.getContentResolver().query(getUri(), null, where, null, null);


The WHERE applies to the

    bookmarks LEFT OUTER JOIN images ON bookmarks.url = images.url_key

so obviously the AS clauses in the projection don't apply by the time the WHERE is processed.


It looks like there are a few options here:

  1. Add column qualifiers to the .query() call. This means we can't just use BookmarksContract.SyncColumns.<column>, and we need to know the inner table names. Queries will break if you alter the structure of the query. This is the approach we're taking right now to keep jvoll moving.

  2. Adjust the query to apply the WHERE to the entire joined and projected query. That might put more burden on the query optimizer (which sucks), potentially generating more intermediate rows. sqlite might be smart enough to pull the clauses inside; I don't know. This is probably the correct solution if that doesn't apply.

  3. Adjust the meaning of the WHERE clause to apply to the bookmarks table in a subquery, then join the result with images, and project. This means that callers can't apply the WHERE clause to images, but it means the bookmark column names can be used directly.

  4. Maybe there's some option here that uses the projection argument to .query()…?

  5. It strikes me that until we sync favicons, we don't need to join to images to get thumbnail -- that's something that only Fennec needs to do. It seems that having two separate query URIs here would allow you to avoid that join entirely; faster and cleaner in this case.

---


Final solution might be to LEFT OUTER JOIN to a subquery on images. This might be more efficiently phrased as a view.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Eliminate required table references in BrowserProvider's query interface

Creates enhanced views for bookmarks and history tables that include all columns from original table plus the image columns (favicon and thumbnail). This simplifies a bunch of parts of the code that required explicit casting of columns with table names. The view are only used in queries with a projection that includes image columns. Use the table directly otherwise.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 581104 [details] [diff] [review]
Eliminate required table references in BrowserProvider's query interface

Review of attachment 581104 [details] [diff] [review]:
-----------------------------------------------------------------

this is a rubberstamp from me. If rnewman likes it, it is fine.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 581104 [details] [diff] [review]
Eliminate required table references in BrowserProvider's query interface

Review of attachment 581104 [details] [diff] [review]:
-----------------------------------------------------------------

Bootiful.

::: mobile/android/base/db/BrowserProvider.java
@@ +245,5 @@
>  
>          return result;
>      }
>  
> +    private boolean hasImagesInProjection(String[] projection) {

This can be static.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #3)
> Comment on attachment 581104 [details] [diff] [review]
> Eliminate required table references in BrowserProvider's query interface
> 
> Review of attachment 581104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bootiful.
> 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ +245,5 @@
> >  
> >          return result;
> >      }
> >  
> > +    private boolean hasImagesInProjection(String[] projection) {
> 
> This can be static.

Done.

Comment 5

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/50b3a3a55715