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.

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

Maybe inconsistent handling of implicit timestamp<->bigint conversion #14319

Open
nyh opened this issue Jun 20, 2023 · 3 comments
Open

Maybe inconsistent handling of implicit timestamp<->bigint conversion #14319

nyh opened this issue Jun 20, 2023 · 3 comments
Labels
Milestone

Comments

@nyh
Copy link
Contributor

nyh commented Jun 20, 2023

This is a spinoff of https://github.com/scylladb/scylla-dtest/pull/3247, where we noticed that SELECT mintimeuuid(writetime(x)) works in Cassandra, but on Scylla it fails saying:

A WRITETIME produces a org.apache.cassandra.db.marshal.LongType value, which doesn't match the type: org.apache.cassandra.db.marshal.TimestampType of arg0(system.mintimeuuid)"

It's actually nice that mintimeuuid(writetime(x)) doesn't work - writetime(x) returns a USING TIMESTAMP value, typically in microseconds, while mintimeuuid() expects a timestamp-type value, which is in milliseconds, and the two don't match, so it's good we don't accept it although Cassandra does. We can consider this a Cassandra bug, and so far so good.

But something still worries me... If g is a bigint (64-bit) number, the following select does work on both Scylla and Cassandra:

SELECT mintimeuuid(g)

And yet, the following select does not work in Scylla (it does work on Cassandra):

SELECT mintimeuuid(writetime(x)) 

How can this be? both g and writetime(x) have the same type - bigint. Either bigint can be implicitly converted to timestamp, or it can't. So how come this conversion works for the expression g but not for writetime(x)? Maybe we have a bug in the implicit conversion code?

Again, I'm not too worried about this specific example, where I even like the fact that the implicit conversion didn't work. But I'm worried that it indicates a bug that we did something about implicit conversions inconsistently or wrongly, and it might bite us in the future in other scenarios.

CC @avikivity @cvybhu

@nyh nyh added the area/cql label Jun 20, 2023
@cvybhu
Copy link
Contributor

cvybhu commented Jun 21, 2023

The code that prepares expressions is often inconsistent in how it checks type compatibility - sometimes it's a direct type comparison, and sometimes test_assignment (which allows implicit conversions). I hope to unify this soon, because it leads to inconsistencies like this.

Although this is selector code, so maybe the issue is somewhere there.

@nyh
Copy link
Contributor Author

nyh commented Jul 18, 2023

I found another example of an implicit conversion that Scylla does and Cassandra does:
If you have a counter column c, you can run SELECT bigintasblob(c) FROM tbl WHERE p=1.
In Cassandra this doesn't work - bigintasblob() insists on getting an actual bigint, not a counter, even though they are the same size.

Suprisingly, the opposite direction is rejected in Scylla not just in Cassandra - SELECT counterasblob(i) where i is a bigint column.

So I don't understand when Scylla allows, and when it doesn't, these implicit conversions of function arguments.

nyh added a commit to nyh/scylla that referenced this issue Jul 18, 2023
Code in functions.cc creates the different TYPEasblob() and blobasTYPE()
functions for all type names TYPE. The functions for the "counter" type
were skipped, supposedly because "counters are not supported yet". But
counters are supported, so let's add the missing functions.

The code fix is trivial, the tests that verify that the result behaves
like Cassandra took more work.

After this patch, unimplemented::cause::COUNTERS is no longer used
anywhere in the code. I wanted to remove it, but noticed that
unimplemented::cause is a graveyard of unused causes, so decided not
to remove this one either. We should clean it up in a separate patch.

Fixes scylladb#14742

Also includes tests for tangently-related issues:
Refs scylladb#12607
Refs scylladb#14319

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
nyh added a commit to nyh/scylla that referenced this issue Jul 30, 2023
Code in functions.cc creates the different TYPEasblob() and blobasTYPE()
functions for all type names TYPE. The functions for the "counter" type
were skipped, supposedly because "counters are not supported yet". But
counters are supported, so let's add the missing functions.

The code fix is trivial, the tests that verify that the result behaves
like Cassandra took more work.

After this patch, unimplemented::cause::COUNTERS is no longer used
anywhere in the code. I wanted to remove it, but noticed that
unimplemented::cause is a graveyard of unused causes, so decided not
to remove this one either. We should clean it up in a separate patch.

Fixes scylladb#14742

Also includes tests for tangently-related issues:
Refs scylladb#12607
Refs scylladb#14319

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@DoronArazii DoronArazii added this to the Backlog milestone Aug 22, 2023
@nyh
Copy link
Contributor Author

nyh commented Jan 30, 2024

Interestingly, the issue #5552 was about mintimeuuid(writetime(g)) being accepted but then crashing Scylla. Commit 1078c86 by @bhalevy fixed it by detecting out-of-range timestamps (as it turns out in #17035 this forgot to exclude negative timestamps), but this detection is no longer needed to solve the original problem of mintimeuuid(writetime(g)), which is now rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants