Last Comment Bug 709962 - Eliminate required column references in browser content provider query interface
: Eliminate required column references in browser content provider query interface
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P2 normal (vote)
: ---
Assigned To: Lucas Rocha (:lucasr)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 14:15 PST by Richard Newman [:rnewman]
Modified: 2012-01-09 11:38 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Eliminate required table references in BrowserProvider's query interface (17.01 KB, patch)
2011-12-12 16:53 PST, Lucas Rocha (:lucasr)
blassey.bugs: review+
rnewman: feedback+
Details | Diff | Splinter Review

Description Richard Newman [:rnewman] 2011-12-12 14:15:11 PST
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) 2011-12-12 16:53:38 PST
Created attachment 581104 [details] [diff] [review]
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?) 2011-12-12 23:20:28 PST
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] 2011-12-13 08:30:33 PST
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) 2011-12-13 10:42:56 PST
(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) 2011-12-13 11:49:30 PST
Pushed: http://hg.mozilla.org/mozilla-central/rev/50b3a3a55715

Note You need to log in before you can comment on or make changes to this bug.