Run one instead of n queries #2982
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (b7c5b76f) is created by Prow via merging commits: 90efb98 58a179c. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (b7c5b76f) is created by Prow via merging commits: 90efb98 58a179c. |
schmidt-sebastian
force-pushed
the
mrschmidt/querybuilder
branch
from
eff0bd7 to
4110173
Compare
|
/test device-check-changed |
There was a problem hiding this comment.
I want to see how you feel about an alternative: using SQL 'UNION' instead.
There are some advantages with UNION:
- It is less sql text manipulating, the old code can be modified to simply concat the sql with UNION.
- This would also be mentally close to IndexedDb and LevelDb.
- My playing with sqlite query plan suggests UNION might be more efficient..
It looks like SQLite is not doing all optimization with connected OR filters, it will simply do a filter on index_id.
The query plan for using OR
sqlite> select * from index_entries_test where (index_id=1 and index_value >= 'x' and index_value <'z') or (index_id = 1 and index_value > 'a' and index_value <'c');
QUERY PLAN
`--SEARCH TABLE index_entries_test USING COVERING INDEX sqlite_autoindex_index_entries_test_1 (index_id=?)
The query plan for using UNION:
sqlite> select * from index_entries_test where (index_id = 1 and index_value > 'a' and index_value <'c')
...> union
...> select * from index_entries_test where (index_id=1 and index_value >= 'x' and index_value <'z')
...> order by index_value
...> limit 10
...> ;
QUERY PLAN
`--MERGE (UNION)
|--LEFT
| `--SEARCH TABLE index_entries_test USING COVERING INDEX sqlite_autoindex_index_entries_test_1 (index_id=? AND index_value>? AND index_value<?)
`--RIGHT
`--SEARCH TABLE index_entries_test USING COVERING INDEX sqlite_autoindex_index_entries_test_1 (index_id=? AND index_value>? AND index_value<?)
| @Nullable Bound upperBound = target.getUpperBound(fieldIndex); | ||
|
|
||
| if (Logger.isDebugEnabled()) { | ||
| Logger.warn( | ||
| Logger.debug( |
There was a problem hiding this comment.
The if check above is not neccessary.
There was a problem hiding this comment.
It is meant to reduce computation complexity as it avoids stringifying the protos.
schmidt-sebastian
force-pushed
the
mrschmidt/querybuilder
branch
from
33b3666 to
8c8993c
Compare
| @Nullable Bound upperBound = target.getUpperBound(fieldIndex); | ||
|
|
||
| if (Logger.isDebugEnabled()) { | ||
| Logger.warn( | ||
| Logger.debug( |
There was a problem hiding this comment.
It is meant to reduce computation complexity as it avoids stringifying the protos.
| @Nullable Bound upperBound = target.getUpperBound(fieldIndex); | ||
|
|
||
| if (Logger.isDebugEnabled()) { | ||
| Logger.warn( | ||
| Logger.debug( |
Instead of running the same query a bunch of times (e.g. for IN queries), this PR now runs a query with multiple OR clauses. This allows SQLite to dedupe results.
I think this should never hit a statement limit since our query complexity is quite limited.