counters: Fix filtering of counters #6277
Conversation
types.cc
Outdated
| @@ -1985,8 +1985,15 @@ struct compare_visitor { | |||
| int32_t operator()(const empty_type_impl&) { return 0; } | |||
| int32_t operator()(const tuple_type_impl& t) { return compare_aux(t, v1, v2); } | |||
| int32_t operator()(const counter_type_impl&) { | |||
| fail(unimplemented::cause::COUNTERS); | |||
| return 0; | |||
| if (v1.empty()) { | |||
There was a problem hiding this comment.
What does empty value mean? Is it deleted counter or is it counter before the first operation? The second would mean empty value == 0.
There was a problem hiding this comment.
Good catch. Deleted counters do not get into this comparison (they are excluded on higher level). But uninitialized ones do - even for non-counter types. That's why e.g. ints also suffer from empty values:
cqlsh> CREATE TABLE ks.tbl_int2 (pk int primary key, i1 int, i2 int);
cqlsh> INSERT INTO ks.tbl_int2 (pk, i1) VALUES (0, 0);
cqlsh> SELECT * FROM ks.tbl_int2 WHERE i2 < -999 ALLOW FILTERING;
pk | i1 | i2
----+----+------
0 | 0 | null
(1 rows)
My thinking is that uninitialized values should never get that deep.
There was a problem hiding this comment.
You're probably right. For counters fix would be easy (uninitialized == 0) but for ints it's not that easy. We probably should fix this in the upper layer. But it might be easier to treat empty for counters as 0 in this PR and merge it and fix the null thing in separate PR.
There was a problem hiding this comment.
Uninitialized int is NULL, not zero. NULL doesn't equal anything.
For counters, it is reasonable to use 0 for an uninitialized counter (there is no way to initialize a counter).
There was a problem hiding this comment.
What does Cassanra do? e.g. for a table with two counter columns, perform an update that increments just one of them.
There was a problem hiding this comment.
@avikivity Tested on C* 3.11:
- untouched counter doesn't equal anything;
- deleted counter doesn't equal anything.
I just updated this patch so Scylla treats untouched counters as zeros (and deleted ones as NULLs). Such a compatibility breach makes sense to me too, but we need a definitive opinion if it's OK.
|
LGTM in general. I have just one question and request for some additional test cases. Thanks. |
CQL `counter_type_impl` is now made comparable by deserializing it as an `int64_t`. It allows the use of counters in statement restrictions.
Tested predicates: IN, EQ, GE, GT, LE, LT. Untouched counters are expected to evaluate as 0. Deleted counters are expected not to appear at all.
jul-stas
force-pushed
the
5635-filtering-on-counters
branch
from
4e493d2
to
afee590
Compare
|
@avikivity @psarna could any of you merge this in, please? |
|
Looks good. Also, looks like the effort of adding all these |
|
I queued it as c32faee. It's debatable whether we should diverge from Cassandra's treatment of uninitialized counters, but our way seems to make more sense. |
Queries with
ALLOW FILTERINGand constraints on counter values used to be rejected as "unimplemented". The reason was a missing tri-comparator, which is added in this patch.Fixes #5635