GNOME Bugzilla – Bug 740701
Filter priorities
Last modified: 2014-12-08 18:19:39 UTC
I've just spent a while wondering why my query, which is supposed to return only one entry, was returning instead 22958. (slightly more than expected :P) Printing the SQL command run by Gom, I see: SELECT COUNT('chars'.'id') FROM 'chars' WHERE 'chars'.'version' == ? AND 'chars'.'orientation' != ? AND 'chars'.'big5' == ? OR 'chars'.'hkscs' == ? OR 'chars'.'punctuation' == ? AND 'chars'.'code' == ? Putting in there the values, this becomes: SELECT COUNT('chars'.'id') FROM 'chars' WHERE 'chars'.'version' == 3 AND 'chars'.'orientation' != 1 AND 'chars'.'big5' == 1 OR 'chars'.'hkscs' == 1 OR 'chars'.'punctuation' == 1 AND 'chars'.'code' == 'ddd' (as an aside, it would be nice if Gom printed the values in the SQL queries, not just the '?') And sure enough, if I run this query in sqlite3, I get: sqlite> SELECT ... 22958 However, the query I really wanted was: SELECT COUNT('chars'.'id') FROM 'chars' WHERE 'chars'.'version' == 3 AND 'chars'.'orientation' != 1 AND ('chars'.'big5' == 1 OR 'chars'.'hkscs' == 1 OR 'chars'.'punctuation' == 1) AND 'chars'.'code' == 'ddd' Notice the parentheses! Running this new query in sqlite3: sqlite> SELECT ... 1 That's better. I'm building the filters as follows (well, slightly simplifying the code here, this comment is already long enough), and I was expecting it to respect priorities, that is, to use parentheses: v_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "version", &value); o_filter = gom_filter_new_neq (CANGJIE_TYPE_CHAR, "orientation", &value); v_and_o_filter = gom_filter_new_and (version_filter, orientation_filter); b_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "big5", &value); h_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "hkscs", &value); p_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "punctuation", &value); b_or_h_filter = gom_filter_new_or (b_filter, h_filter); f_filter = gom_filter_new_or (b_or_h_filter, p_filter); v_and_o_and_f_filter = gom_filter_new_and (v_and_o_filter, f_filter); code_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "code", &value); final_filter = gom_filter_new_and (v_and_o_and_f_filter, code_filter);
Created attachment 291487 [details] [review] Respect filter priorities This adds parentheses everywhere (probably more than strictly necessary), in order to respect the priorities intended by the user.
Not sure this is the right approach to fixing this, but here's a simple patch that helps in my case. I'm not excluding the fact that I'm actually doing something wrong with the way I build the filters. :)
Review of attachment 291487 [details] [review]: If the test suite passes, looks good.
(In reply to comment #0) > However, the query I really wanted was: > > SELECT COUNT('chars'.'id') > FROM 'chars' > WHERE 'chars'.'version' == 3 > AND 'chars'.'orientation' != 1 > AND ('chars'.'big5' == 1 > OR 'chars'.'hkscs' == 1 > OR 'chars'.'punctuation' == 1) > AND 'chars'.'code' == 'ddd' > > Notice the parentheses! This isn't exactly what your code does though. <snip> > v_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "version", &value); > o_filter = gom_filter_new_neq (CANGJIE_TYPE_CHAR, "orientation", &value); > v_and_o_filter = gom_filter_new_and (version_filter, orientation_filter); > > b_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "big5", &value); > h_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "hkscs", &value); > p_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "punctuation", &value); > b_or_h_filter = gom_filter_new_or (b_filter, h_filter); > f_filter = gom_filter_new_or (b_or_h_filter, p_filter); > > v_and_o_and_f_filter = gom_filter_new_and (v_and_o_filter, f_filter); > > code_filter = gom_filter_new_eq (CANGJIE_TYPE_CHAR, "code", &value); > final_filter = gom_filter_new_and (v_and_o_and_f_filter, code_filter); It does: SELECT COUNT('chars'.'id') FROM 'chars' WHERE (('chars'.'version' == 3 AND 'chars'.'orientation' != 1) AND (('chars'.'big5' == 1 OR 'chars'.'hkscs' == 1) OR 'chars'.'punctuation' == 1)) AND 'chars'.'code' == 'ddd' Which might not be really equivalent. Our filters don't handle more than 2 operands right now. But this is likely a separate problem.
I have a new, better patch for this coming, based on top of the ones from bug 740872. I'll attach it later, once we figure out what to do with the other bug. :)
Created attachment 292207 [details] [review] filter: Respect filter priorities This adds parenthesis to the generated SQL queries where appropriate.
Created attachment 292296 [details] [review] filter: Respect filter priorities This adds parenthesis to the generated SQL queries where appropriate.
Here's an updated patch after your IRL review from Friday. :)
Review of attachment 292296 [details] [review]: Looks good. Does it fix the original bug as well? Could you make a test case for that as well? Thinking of what the test set should be will be tricky.
Created attachment 292310 [details] [review] tests: Add more data to the DB This will help better ensuring some filter beheviours.
Created attachment 292311 [details] [review] tests: Ensure filter priority This runs 2 queries with the same filters, but "grouped" in 2 different ways, to ensure that we respect filtering priorities (i.e parentheses).
Review of attachment 292310 [details] [review]: Looks good otherwise. ::: tests/test-gom-find-specific.c @@ +237,3 @@ g_assert (ret); + for (i = 0; i < 3; i++) { i < G_N_ELEMENTS(values);
Review of attachment 292311 [details] [review]: Good stuff!
> Does it fix the original bug as well? Sure it does. My new query now looks like: SELECT COUNT('chars'.'id') FROM 'chars' WHERE ('chars'.'version' == 3 AND 'chars'.'orientation' != 1 AND ('chars'.'big5' == 1 OR 'chars'.'hkscs' == 1 OR 'chars'.'punctuation' == 1)) AND 'chars'.'code' == 'ddd' Which, given how I'm constructing the filters, is exactly what I was expecting. :)
Attachment 292296 [details] pushed as ac381b1 - filter: Respect filter priorities Attachment 292310 [details] pushed as 2d4e1db - tests: Add more data to the DB Attachment 292311 [details] pushed as 991dcc2 - tests: Ensure filter priority