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

Botched up "token function arguments must be in partition key order" message #13468

Closed
nyh opened this issue Apr 10, 2023 · 2 comments
Closed

Botched up "token function arguments must be in partition key order" message #13468

nyh opened this issue Apr 10, 2023 · 2 comments
Labels
area/cql symptom/ux Concerns regarding the user experience in working with Scylla. type/bug
Milestone
5.3

Comments

@nyh
Copy link
Contributor

nyh commented Apr 10, 2023

When the token() function is used on the left-hand side of an expression, it requires all the partition key columns to be listed in order. If we have the partition key a and use token(b), Scylla currently generates the following error message:

The token function arguments must be in the partition key order: [0x60000506d928]

It appears that we intended to print a list of column names, but instead printed the address of the list :-)

The following failing test (it causes the error to be shown), inserted into test/cql-pytest/test_filtering.py, reproduces this bug:

def test_filter_token(cql, table1):
    cql.execute(f'SELECT a FROM {table1} WHERE token(b) = 0 ALLOW FILTERING')

CC @cvybhu (I also think this may be a bug in wrong array formatting, the kind of which @tchaikov looked into recently).
@nyh nyh added type/bug symptom/ux Concerns regarding the user experience in working with Scylla. area/cql labels Apr 10, 2023
@tchaikov
Copy link
Contributor

i think we should print out the partition keys of the table instead of the ones passed to token() , so user can compare his/her CQL statement with the feedback, and fix it accordingly.

this is not a bug in array formatting. this is just how we print arrays, see

scylladb/utils/to_string.hh

Line 30 in fd817e1

oss << *begin;
. if we were using fmtlib here, it won't compile at all. but operator<< just prints out the value of the pointer, which is the referenced by the begin.

tchaikov added a commit to tchaikov/scylladb that referenced this issue Apr 11, 2023
@tchaikov
cql3: expr: return more accurate error message for invalidated token(…
5c4fd96 
…) args

before this change, we just print out the addresses of the elements
in `column_defs`, if the argument passed to `token()` function are
not valid. this is not quite helpful from user's perspective, as
user would be more interested in the values. also, we could have
more accurate error message for different error.

in this change, following how Cassandra 4.1's behavior, three cases
are identified, and corresponding errors are returned respectively:

* duplicated partition keys
* wrong order of partition key
* missing keys

where, if the partition key order is wrong, instead of the keys specified
by user, the correct order is printed in the error message for helping
user to correct the `token()` function.

for better performance, the checks are performed only if the keys
do not match, based on the assumption that the error handling path
is not likely to be executed.

tests are added accordingly. they tested with Canssandra 4.1.1 also.

Fixes scylladb#13468
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
@tchaikov tchaikov mentioned this issue Apr 11, 2023
Closed
tchaikov added a commit to tchaikov/scylladb that referenced this issue Apr 11, 2023
@tchaikov
cql3: expr: return more accurate error message for invalidated token(…
6fedc00 
…) args

before this change, we just print out the addresses of the elements
in `column_defs`, if the argument passed to `token()` function are
not valid. this is not quite helpful from user's perspective, as
user would be more interested in the values. also, we could have
more accurate error message for different error.

in this change, following how Cassandra 4.1's behavior, three cases
are identified, and corresponding errors are returned respectively:

* duplicated partition keys
* wrong order of partition key
* missing keys

where, if the partition key order is wrong, instead of the keys specified
by user, the correct order is printed in the error message for helping
user to correct the `token()` function.

for better performance, the checks are performed only if the keys
do not match, based on the assumption that the error handling path
is not likely to be executed.

tests are added accordingly. they tested with Canssandra 4.1.1 also.

Fixes scylladb#13468
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov added a commit to tchaikov/scylladb that referenced this issue Apr 11, 2023
@tchaikov
cql3: expr: return more accurate error message for invalidated token(…
d10125c 
…) args

before this change, we just print out the addresses of the elements
in `column_defs`, if the argument passed to `token()` function are
not valid. this is not quite helpful from user's perspective, as
user would be more interested in the values. also, we could have
more accurate error message for different error.

in this change, following how Cassandra 4.1's behavior, three cases
are identified, and corresponding errors are returned respectively:

* duplicated partition keys
* wrong order of partition key
* missing keys

where, if the partition key order is wrong, instead of the keys specified
by user, the correct order is printed in the error message for helping
user to correct the `token()` function.

for better performance, the checks are performed only if the keys
do not match, based on the assumption that the error handling path
is not likely to be executed.

tests are added accordingly. they tested with Canssandra 4.1.1 also.

Fixes scylladb#13468
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov added a commit to tchaikov/scylladb that referenced this issue Apr 11, 2023
@tchaikov
cql3: expr: return more accurate error message for invalidated token(…
5c26acd 
…) args

before this change, we just print out the addresses of the elements
in `column_defs`, if the argument passed to `token()` function are
not valid. this is not quite helpful from user's perspective, as
user would be more interested in the values. also, we could have
more accurate error message for different error.

in this change, following how Cassandra 4.1's behavior, three cases
are identified, and corresponding errors are returned respectively:

* duplicated partition keys
* wrong order of partition key
* missing keys

where, if the partition key order is wrong, instead of the keys specified
by user, the correct order is printed in the error message for helping
user to correct the `token()` function.

for better performance, the checks are performed only if the keys
do not match, based on the assumption that the error handling path
is not likely to be executed.

tests are added accordingly. they tested with Canssandra 4.1.1 also.

Fixes scylladb#13468
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov added a commit to tchaikov/scylladb that referenced this issue Apr 12, 2023
@tchaikov
cql3: expr: return more accurate error message for invalidated token(…
02a1037 
…) args

before this change, we just print out the addresses of the elements
in `column_defs`, if the argument passed to `token()` function are
not valid. this is not quite helpful from user's perspective, as
user would be more interested in the values. also, we could have
more accurate error message for different error.

in this change, following how Cassandra 4.1's behavior, three cases
are identified, and corresponding errors are returned respectively:

* duplicated partition keys
* wrong order of partition key
* missing keys

where, if the partition key order is wrong, instead of the keys specified
by user, the correct order is printed in the error message for helping
user to correct the `token()` function.

for better performance, the checks are performed only if the keys
do not match, based on the assumption that the error handling path
is not likely to be executed.

tests are added accordingly. they tested with Canssandra 4.1.1 also.

Fixes scylladb#13468
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov added a commit to tchaikov/scylladb that referenced this issue Apr 12, 2023
@tchaikov
cql3: expr: return more accurate error message for invalidated token(…
775ad49 
…) args

before this change, we just print out the addresses of the elements
in `column_defs`, if the arguments passed to `token()` function are
not valid. this is not quite helpful from the user's perspective. as
user would be more interested in the values. also, we could print
more accurate error message for different error.

in this change, following Cassandra 4.1's behavior, three cases are
identified, and corresponding errors are returned respectively:

* duplicated partition keys
* wrong order of partition key
* missing keys

where, if the partition key order is wrong, instead of printing the
keys specified by user, the correct order is printed in the error
message for helping user to correct the `token()` function.

for better performance, the checks are performed only if the keys
do not match, based on the assumption that the error handling path
is not likely to be executed.

tests are added accordingly. they tested with Canssandra 4.1.1 also.

Fixes scylladb#13468
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov added a commit to tchaikov/scylladb that referenced this issue Apr 12, 2023
@tchaikov
cql3: expr: return more accurate error message for invalidated token(…
6da7f16 
…) args

before this change, we just print out the addresses of the elements
in `column_defs`, if the arguments passed to `token()` function are
not valid. this is not quite helpful from the user's perspective. as
user would be more interested in the values. also, we could print
more accurate error message for different error.

in this change, following Cassandra 4.1's behavior, three cases are
identified, and corresponding errors are returned respectively:

* duplicated partition keys
* wrong order of partition key
* missing keys

where, if the partition key order is wrong, instead of printing the
keys specified by user, the correct order is printed in the error
message for helping user to correct the `token()` function.

for better performance, the checks are performed only if the keys
do not match, based on the assumption that the error handling path
is not likely to be executed.

tests are added accordingly. they tested with Canssandra 4.1.1 also.

Fixes scylladb#13468
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
@avikivity
Copy link
Member

Minor, not backporting.

@DoronArazii DoronArazii added this to the 5.3 milestone May 1, 2023
cvybhu added a commit to cvybhu/scylladb that referenced this issue May 9, 2023
@cvybhu
cql3/prepare_expr: force token() receiver name to be partition key token
383f4e4 
Let's say that we have a prepared statement with a token restriction:
```cql
SELECT * FROM some_table WHERE token(p1, p2) = ?
```

After calling `prepare` the drivers receives some information
about the prepared statment, including names of values bound
to each bind marker.

In case of a partition token restriction (`token(p1, p2) = ?`)
there's an expectation that the name assigned to this bind marker
will be `"partition key token"`.

In a recent change the code handling `token()` expressions has been
unified with the code that handles generic function calls,
and as a result the name has changed to `token(p1, p2)`.

It turns out that the Java driver relies on the name being
`"partition key token"`, so a change to `token(p1, p2)`
broke some things.

This patch sets the name back to `"partition key token"`.
To achieve this we detect any restrictions that match
the pattern `token(p1, p2, p3) = X` and set the receiver
name for X to `"partition key token"`.

Fixes: scylladb#13468

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
@cvybhu cvybhu mentioned this issue May 9, 2023
Merged
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 symptom/ux Concerns regarding the user experience in working with Scylla. type/bug
Projects
None yet
Milestone
5.3
Development

Successfully merging a pull request may close this issue.

cql3: expr: return more accurate error message for invalidate token(… tchaikov/scylladb
5 participants
@tchaikov @nyh @avikivity @scylladb-promoter @DoronArazii