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

datetime string parsing difference between rqlite #1658

Open
bwarminski opened this issue Jan 31, 2024 · 8 comments
Open

datetime string parsing difference between rqlite #1658

bwarminski opened this issue Jan 31, 2024 · 8 comments

Comments

@bwarminski
Copy link

What version are you running?

version 8, SQLite 3.44.0, commit unknown, branch unknown, compiler gc

Are you using Docker or Kubernetes to run your system?

No

Are you running a single node or a cluster?

Single in this case

What did you do?

I am running some of the sqlite jdbc tests against an rqlite jdbc implementation and noticed a difference in datetime handling behavior. Here is the test case:

127.0.0.1:4001> create table sample (start_time datetime);
1 row affected
127.0.0.1:4001> insert into sample values('2024-01-31-05:00');
1 row affected
127.0.0.1:4001> select * from sample;

What did you expect to happen?

I expected the result of this query to be:

2024-01-31-05:00

That is what sqlite will return:

 $ sqlite3
SQLite version 3.43.2 2023-10-10 13:08:14
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> create table sample (start_time datetime);
sqlite> insert into sample values('2024-01-31-05:00');
sqlite> select * from sample;
2024-01-31-05:00

What happened instead?

0001-01-01T00:00:00Z was returned

Example from CLI:

127.0.0.1:4001> select * from sample;
+----------------------+
| start_time           |
+----------------------+
| 0001-01-01T00:00:00Z |
+----------------------+

Example from REST endpoint:

$ curl -XPOST 'localhost:4001/db/query?pretty&timings&timeout=30s' -H "Content-Type: application/json" -d @jsonextract.json
{
    "results": [
        {
            "columns": [
                "start_time"
            ],
            "types": [
                "datetime"
            ],
            "values": [
                [
                    "0001-01-01T00:00:00Z"
                ]
            ],
            "time": 0.000045588
        }
    ],
    "time": 0.000529417
}
@bwarminski bwarminski changed the title datetime string parsing difference between rqli datetime string parsing difference between rqlite Jan 31, 2024
@otoolep
Copy link
Member

otoolep commented Jan 31, 2024

Thank you, looks like a bug. I will look into this shortly, I don't think I have query test coverage with that type.

@otoolep
Copy link
Member

otoolep commented Feb 1, 2024

Well, this is odd. I have unit tests related to this: https://github.com/rqlite/rqlite/blob/v8.18.5/db/db_common_test.go#L129

@otoolep
Copy link
Member

otoolep commented Feb 1, 2024

OK, from the docs for the underlying driver at https://pkg.go.dev/github.com/mattn/go-sqlite3

var SQLiteTimestampFormats = [][string](https://pkg.go.dev/builtin#string){

	"2006-01-02 15:04:05.999999999-07:00",
	"2006-01-02T15:04:05.999999999-07:00",
	"2006-01-02 15:04:05.999999999",
	"2006-01-02T15:04:05.999999999",
	"2006-01-02 15:04:05",
	"2006-01-02T15:04:05",
	"2006-01-02 15:04",
	"2006-01-02T15:04",
	"2006-01-02",
}

SQLiteTimestampFormats is timestamp formats understood by both this module and SQLite. The first format in the slice will be used when saving time values into the database. When parsing a string from a timestamp or datetime column, the formats are tried in order.

The timestamp you supplied is not actually in any commonly accepted standard, that I know of -- it doesn't appear to be ISO 8601. Not sure what SQLite itself is doing, but the format you're supplying is not valid as per https://www.sqlite.org/lang_datefunc.html

A time value can be in any of the following formats shown below. The value is usually a string, though it can be an integer or floating point number in the case of format 12.

YYYY-MM-DD
YYYY-MM-DD HH:MM
YYYY-MM-DD HH:MM:SS
YYYY-MM-DD HH:MM:SS.SSS
YYYY-MM-DDTHH:MM
YYYY-MM-DDTHH:MM:SS
YYYY-MM-DDTHH:MM:SS.SSS
HH:MM
HH:MM:SS
HH:MM:SS.SSS
now
DDDDDDDDDD 

The underlying driver instead of returning the string you pushed in returns the zero time. See https://github.com/mattn/go-sqlite3/blob/4702d9b5d640f42488752f5cf70a195b748ffe96/sqlite3.go#L2261

While it might be nice to do exactly what SQLite does, since your example is using a time that is not officially supported by SQLite, I'm not planning to make any changes -- the action of inserting a non-supported timestamp format results in undefined behaviour. Hope that makes sense.

@otoolep otoolep closed this as completed Feb 1, 2024
@bwarminski
Copy link
Author

Sure, the reasoning makes sense. For context, that specific string was generated by a Java date time formatter. I'll push up the ignored test onto a the Java driver in a few days after tweaking my implementation to get the others to pass. I was basically taking the unit tests from the java sqlite driver and applying them here.

In the meantime, here's the code

    conn.createStatement().execute("create table sample (start_time datetime)");

    Date now = new Date();
    String date = DateTimeFormatter.ISO_DATE.format(now.toInstant().atZone(ZoneId.systemDefault()));

    conn.createStatement().execute("insert into sample values('" + date + "')");

At least according to Java, it's a valid ISO date string and could cause problems from a cross-language portability standpoint. At least in that language, you can request typed objects from the driver and it seems like a lot of drivers attempt to convert to the correct value. In this case, it's impossible. I'm personally ambivalent about it. Happy to take a look in more detail, though, if you'd like.

@otoolep
Copy link
Member

otoolep commented Feb 1, 2024

Interesting.

I wouldn't be surprised if SQLite did more than it explicitly spelled out in its docs, or the shell did some transformation for user happiness sake, but the SQLite docs are quite clear - the format you supplied is not supported. I could go into my fork of the driver and add support for this type, but I don't wish to make changes unless it's clear why. For example, if the SQLite docs were shown to be out of date relative to their source code.

@bwarminski
Copy link
Author

I've changed up the code to only use those specific formats and ran into the opposite problem with formatting consistency. I understand that the outcomes here are undefined when those formats aren't followed, but thought to raise this anyway since the underlying driver seems to be disobeying its own format constraints.

For context, I'm attempting to mimic this test https://github.com/xerial/sqlite-jdbc/blob/d76c9330d447cef58740c98abb3dec79c8849ed5/src/test/java/org/sqlite/QueryTest.java#L96 with an rqlite backed jdbc driver.

That statement inserts a long into a datetime column. Here's the test case:

127.0.0.1:4001> CREATE TABLE sample (start_time datetime);
1 row affected
127.0.0.1:4001> insert into sample values(1706842979630);
1 row affected
127.0.0.1:4001> select * from sample;
+-------------------------+
| start_time              |
+-------------------------+
| 2024-02-02T03:02:59.63Z |
+-------------------------+
127.0.0.1:4001> 

Note the "Z" at the end which is inconsistent with the formats it declares.

Raw sqlite just lets this stay as a long

SQLite version 3.43.2 2023-10-10 13:08:14
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> create table sample (start_time datetime);
sqlite> insert into sample values(1706842979630);
sqlite> select * from sample;
1706842979630
sqlite> 

I can build around the inconsistency, but wanted to report it in case it represented a surprise usability issue for someone. You're maintaining a fork of mattn for a reason, I'd imagine - is it because the upstream is unlikely to patch these types of issues?

@otoolep otoolep reopened this Feb 2, 2024
@otoolep
Copy link
Member

otoolep commented Feb 2, 2024

That Z at the end may be something rqlite is doing, I'll look into it. However the whole thing of "types" and "type affinities" is complex in SQLite, so it's easy for differences like this to emerge.

I maintain a fork because there was a couple of functions that upstream hadn't exposed at one point. They subsequently added them, and my fork my actually be unneeded, but I've yet to confirm.

@otoolep
Copy link
Member

otoolep commented Feb 2, 2024

https://www.sqlite.org/datatype3.html

While you're declaring the column to be a datetime, you're putting an integer in there. And what's in there is what SQLite actually returns. But rqlite might be looking at the declared type and converting the integer to a timestamp (which is closer to what other relational databases do).

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

No branches or pull requests

2 participants