fix: order normalization with descending query #788
Conversation
product-auto-label
bot
added
size: m
tests/unit/v1/test_base_query.py
Outdated
|
|
||
| normalized_w_snapshot = query_w_snapshot._normalize_orders() | ||
| expected_w_snapshot = expected + [ | ||
| query._make_order("b", "DESCENDING"), |
There was a problem hiding this comment.
let me know if this test case looks right.
Previously we were always using ASCENDING for all added orders. My understanding is we should always use the previous direction?
There was a problem hiding this comment.
Nit: IMO it would be more readable just to spell out the whole expected list here instead of appending the previous expected.
| orders.append(self._make_order("__name__", direction)) | ||
| # skip equality filters and filters on fields already ordered | ||
| if ( | ||
| filter_.op not in _EQUALITY_OPERATORS |
There was a problem hiding this comment.
In my opinion, it would be better to create a list of INEQUALITY filters such that a new filter is default opted out rather than default opted in. This matches how other SDKs handle this more closely. I also think it's more likely someone will notice that an inequality is not having the order-by normalized rather than an equality filter is accidentally sneaking into the order-by normalization (Since it's an equality, the ordering is not easily noticed in most situations.)
There was a problem hiding this comment.
makes sense, fixed
tests/unit/v1/test_base_query.py
Outdated
| _make_base_query(collection) | ||
| .where(filter=FieldFilter("a", "==", 1)) | ||
| .where(filter=FieldFilter("b", "in", [1, 2, 3])) | ||
| .where(filter=FieldFilter("b", "not-in", [4, 5, 6])) |
There was a problem hiding this comment.
Nit: We have two 'b' values here, an equality and inequality on the same field, it would probably make more sense to have this be a separate property value.
This PR fixes a bug in order normalization logic, to be more in line with nodejs: