Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Inconsistent validation of multiple restrictions on the same bound #18690

Open
nyh opened this issue May 15, 2024 · 0 comments
Open

Inconsistent validation of multiple restrictions on the same bound #18690

nyh opened this issue May 15, 2024 · 0 comments
Labels
area/cql P4 Low Priority symptom/ux Concerns regarding the user experience in working with Scylla.

Comments

@nyh
Copy link
Contributor

nyh commented May 15, 2024

Cassandra does not allow a WHERE restriction to restrict the same key from the same side twice. For example, if b is a clustering key, the restriction b < 1 AND b < 2 is not allowed. Although such restrictions are somewhat silly (since one of the terms in the restriction makes the other term superfluous), in issue #12472 we decided those are valid expressions and Scylla will support them. We have a cql-pytest test test_restrictions.py::test_multiple_restrictions_on_ck that checks that in Scylla these sort of restrictions work correctly (while they are refused in Cassandra).

However, it turns out that our support for such "superfluous" expression terms is only partial. We allow them for single-column restrictions, but for multi-column restrictions we have separate validation code, and this one insists - as Cassandra - that the same column must not be restricted twice from the same side (start or end) in the expression. For example, the restriction (b) < (1) AND (b,c) < (2,2) returns the error More than one restriction was found for the end bound on b - exactly the same error message as Cassandra does.

To add even more confusion, it turns out that the validation for multi-column restrictions is buggy and as shown by @michoecho in #18688, the restriction (b) < (1) AND (b) >= (0) AND (b, c) > (0,0) the validation code gets misled by the first term and misses that the second and third terms both give a lower bound for b, so that validation code wanted to generate an error but didn't.

We should consider fixing these inconsistencies - restricting the same column twice from the same side should either be allowed in all cases or forbidden in all cases - not the current confusing mess where in some cases it's allowed and some cases it isn't.
@nyh nyh added symptom/ux Concerns regarding the user experience in working with Scylla. area/cql P4 Low Priority labels May 15, 2024
nyh added a commit to nyh/scylla that referenced this issue May 15, 2024
@nyh
cql: fix hang during certain SELECT statements
2f6cd04 
The function intersection(r1,r2) in statement_restrictions.cc is used
when several WHERE restrictions were applied to the same column.
For example, for "WHERE b<1 AND b<2" the intersection of the two ranges
is calculated to be b<1.

As noted in issue scylladb#18690, Scylla is inconsistent in where it allows or
doesn't allow these intersecting restrictions. But where they are
allowed they must be implemented correctly. And it turns out the
function intersection() had a bug that caused it to sometimes enter
an infinite loop - when the intent was only to call itself once with
swapped parameters.

This patch includes a test reproducing this bug, and a fix for the
bug. The test hangs before the fix, and passes after the fix.

While at it, I carefully reviewed the entire code used to implement
the intersection() function to try to make sure that the bug we found
was the only one. I also added a few more comments where I thought they
were needed to understand complicated logic of the code.

The bug, the fix and the test were originally discovered by
Michał Chojnowski.

Fixes scylladb#18688
Refs scylladb#18690

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@nyh nyh mentioned this issue May 15, 2024
Closed
denesb pushed a commit that referenced this issue May 16, 2024
@nyh @denesb
cql: fix hang during certain SELECT statements
27ab560 
The function intersection(r1,r2) in statement_restrictions.cc is used
when several WHERE restrictions were applied to the same column.
For example, for "WHERE b<1 AND b<2" the intersection of the two ranges
is calculated to be b<1.

As noted in issue #18690, Scylla is inconsistent in where it allows or
doesn't allow these intersecting restrictions. But where they are
allowed they must be implemented correctly. And it turns out the
function intersection() had a bug that caused it to sometimes enter
an infinite loop - when the intent was only to call itself once with
swapped parameters.

This patch includes a test reproducing this bug, and a fix for the
bug. The test hangs before the fix, and passes after the fix.

While at it, I carefully reviewed the entire code used to implement
the intersection() function to try to make sure that the bug we found
was the only one. I also added a few more comments where I thought they
were needed to understand complicated logic of the code.

The bug, the fix and the test were originally discovered by
Michał Chojnowski.

Fixes #18688
Refs #18690

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes #18694
mergify bot pushed a commit that referenced this issue May 16, 2024
@nyh @mergify
cql: fix hang during certain SELECT statements
f81ffc9 
The function intersection(r1,r2) in statement_restrictions.cc is used
when several WHERE restrictions were applied to the same column.
For example, for "WHERE b<1 AND b<2" the intersection of the two ranges
is calculated to be b<1.

As noted in issue #18690, Scylla is inconsistent in where it allows or
doesn't allow these intersecting restrictions. But where they are
allowed they must be implemented correctly. And it turns out the
function intersection() had a bug that caused it to sometimes enter
an infinite loop - when the intent was only to call itself once with
swapped parameters.

This patch includes a test reproducing this bug, and a fix for the
bug. The test hangs before the fix, and passes after the fix.

While at it, I carefully reviewed the entire code used to implement
the intersection() function to try to make sure that the bug we found
was the only one. I also added a few more comments where I thought they
were needed to understand complicated logic of the code.

The bug, the fix and the test were originally discovered by
Michał Chojnowski.

Fixes #18688
Refs #18690

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
(cherry picked from commit 2f6cd04)

# Conflicts:
#	test/cql-pytest/test_restrictions.py
@mergify mergify bot mentioned this issue May 16, 2024
Draft
mergify bot pushed a commit that referenced this issue May 16, 2024
@nyh @mergify
cql: fix hang during certain SELECT statements
3a6e37a 
The function intersection(r1,r2) in statement_restrictions.cc is used
when several WHERE restrictions were applied to the same column.
For example, for "WHERE b<1 AND b<2" the intersection of the two ranges
is calculated to be b<1.

As noted in issue #18690, Scylla is inconsistent in where it allows or
doesn't allow these intersecting restrictions. But where they are
allowed they must be implemented correctly. And it turns out the
function intersection() had a bug that caused it to sometimes enter
an infinite loop - when the intent was only to call itself once with
swapped parameters.

This patch includes a test reproducing this bug, and a fix for the
bug. The test hangs before the fix, and passes after the fix.

While at it, I carefully reviewed the entire code used to implement
the intersection() function to try to make sure that the bug we found
was the only one. I also added a few more comments where I thought they
were needed to understand complicated logic of the code.

The bug, the fix and the test were originally discovered by
Michał Chojnowski.

Fixes #18688
Refs #18690

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
(cherry picked from commit 2f6cd04)
@mergify mergify bot mentioned this issue May 16, 2024
Closed
denesb pushed a commit that referenced this issue May 21, 2024
@nyh @denesb
cql: fix hang during certain SELECT statements
0d4e22e 
The function intersection(r1,r2) in statement_restrictions.cc is used
when several WHERE restrictions were applied to the same column.
For example, for "WHERE b<1 AND b<2" the intersection of the two ranges
is calculated to be b<1.

As noted in issue #18690, Scylla is inconsistent in where it allows or
doesn't allow these intersecting restrictions. But where they are
allowed they must be implemented correctly. And it turns out the
function intersection() had a bug that caused it to sometimes enter
an infinite loop - when the intent was only to call itself once with
swapped parameters.

This patch includes a test reproducing this bug, and a fix for the
bug. The test hangs before the fix, and passes after the fix.

While at it, I carefully reviewed the entire code used to implement
the intersection() function to try to make sure that the bug we found
was the only one. I also added a few more comments where I thought they
were needed to understand complicated logic of the code.

The bug, the fix and the test were originally discovered by
Michał Chojnowski.

Fixes #18688
Refs #18690

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
(cherry picked from commit 27ab560)

Closes #18717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/cql P4 Low Priority symptom/ux Concerns regarding the user experience in working with Scylla.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@nyh