-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Inconsistent validation of multiple restrictions on the same bound #18690
Labels
Comments
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
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>
denesb
pushed a commit
that referenced
this issue
May 16, 2024
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
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 bot
pushed a commit
that referenced
this issue
May 16, 2024
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)
denesb
pushed a commit
that referenced
this issue
May 21, 2024
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
Labels
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 restrictionb < 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 testtest_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 errorMore 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.
The text was updated successfully, but these errors were encountered: