-
Notifications
You must be signed in to change notification settings - Fork 105
feat(zero): add support for 'time' datatype #4671
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
Conversation
|
@xfh is attempting to deploy a commit to the Rocicorp Team on Vercel. A member of the Team first needs to authorize it. |
|
Curious why you chose Seems easier to deal with time as "number of nanoseconds since the beginning of the day" rather than as a string. |
|
I think it is a lot more natural to work with strings, because of the browser support: Numeric values are a bit odd for |
e893e42 to
f721687
Compare
|
Hi @tantaman, please let me know what the idea of the pipeline-driver test is, regarding this column definition:
Should the test reflect how it deals with unsupported datatypes? In that case, the "ignored" type can simply change to Based on the values that are set later on and the git history, it looks like typical timestamp values.
I can change the type to |
f721687 to
3c99e04
Compare
|
Pulling this down to check |
|
To convert to a string it should be: function microsecondsToTimeString(microseconds: number): string {
// Convert microseconds to seconds
const totalSeconds = Math.floor(microseconds / 1_000_000);
// Get the fractional microseconds (remainder that does not fit into a whole second)
const fractionalMicroseconds = microseconds % 1_000_000;
// Calculate hours, minutes, and seconds
const hours = Math.floor(totalSeconds / 3600);
const minutes = Math.floor((totalSeconds % 3600) / 60);
const seconds = totalSeconds % 60;
// Format with leading zeros
const hoursStr = hours.toString().padStart(2, '0');
const minutesStr = minutes.toString().padStart(2, '0');
const secondsStr = seconds.toString().padStart(2, '0');
const microsecondsStr = fractionalMicroseconds.toString().padStart(6, '0');
return `${hoursStr}:${minutesStr}:${secondsStr}.${microsecondsStr}`;
} |
|
the problem with the tests was that all |
|
Thanks for taking this on. I'm out camping for a few days, but I am happy to help with whatever is left to do.
|
da03c56 to
515ffd5
Compare
|
ok, this should be good to go now. Waiting on tests then will merge today. |
e681660 to
60bedcd
Compare
…ine-driver.test.ts, lite-tables
60bedcd to
3ef453c
Compare
| ['float4', 'number'], | ||
| ['float8', 'number'], | ||
| ['date', 'number'], | ||
| ['time', 'string'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should change to number, because you changed zero's format to milliseconds since 00.
|
|
||
| // Date/Time types | ||
| 'date': 'number', | ||
| 'time': 'string', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. This should now be type number
|
Good catch. Thanks
…On Fri, Aug 8, 2025 at 7:05 AM Fabio ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/zero-cache/src/types/pg.test.ts
<#4671 (comment)>:
> @@ -46,6 +371,7 @@ describe('dataTypeToZqlValueType', () => {
['float4', 'number'],
['float8', 'number'],
['date', 'number'],
+ ['time', 'string'],
I think this should change to number, because you changed zero's format
to milliseconds since 00.
------------------------------
In packages/zero-cache/src/types/pg.ts
<#4671 (comment)>:
> @@ -257,6 +362,7 @@ export const pgToZqlTypeMap = Object.freeze({
// Date/Time types
'date': 'number',
+ 'time': 'string',
same here. This should now be type number
—
Reply to this email directly, view it on GitHub
<#4671 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHWK22EJE3XIVAIZYJILL33MR76FAVCNFSM6AAAAACCPJZCQOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCMBQGM2DMMBTGM>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
|
fixed in #4709 |
|
This PR didn't work, at least with custom mutators. When a mutator runs I get: This appears to be happening because this CREATE TABLE "message" (
"id" VARCHAR PRIMARY KEY,
"sender_id" VARCHAR REFERENCES "user"(id),
"medium_id" VARCHAR REFERENCES "medium"(id),
"body" VARCHAR NOT NULL,
"timestamp" TIMESTAMP not null,
"t" TIME not null
);leads to this datatype: user@127.0.0.1:postgres> describe message;
+-----------+-----------------------------+-----------+
| Column | Type | Modifiers |
|-----------+-----------------------------+-----------|
| id | character varying | not null |
| sender_id | character varying | |
| medium_id | character varying | |
| body | character varying | not null |
| timestamp | timestamp without time zone | not null |
| t | time without time zone | not null |
+-----------+-----------------------------+-----------+which is not listed in export const pgToZqlTypeMap = Object.freeze({
// Numeric types
...pgToZqlNumericTypeMap,
// Date/Time types
'date': 'number',
'time': 'number',
'timestamp': 'number',
'timestamptz': 'number',
'timestamp with time zone': 'number',
'timestamp without time zone': 'number', |
|
I am not sure what all tests need to be fixed so leaving this for one of you two. |
|
Also, I don't know if you came out at microseconds since 00:00 or milliseconds, but can you please make sure it matches whatever we do for |
|
Here is a PR against one of our sample apps that you can use to replicate the problem: rocicorp/hello-zero-solid#25 |
|
I reproduce, thanks for the setup! Adding There is some conversion of ZQL types to Postgres types happening here in Lines 182 to 232 in d981ea6
But there is also the serialization & parsing logic in mono/packages/zero-cache/src/types/pg.ts Lines 193 to 247 in d981ea6
I don't understand, why there are two different approaches used, to convert to native postgres types. The placeholder approach in I've tested a @tantaman, maybe you could reconsider using a In the meantime, I have found a way to add support for time in case 'time':
case 'time without time zone':
return `(INTERVAL '1 millisecond' * ${valuePlaceholder})::TIME`;Let me know which direction you want to go and I'll implement tests and a PR. |
|
Sorry I meant that I would have assumed millis since midnight. (matt and I
spoke about the string/number thing and I agree with him -- I don't like
the idea of transmitting time as a string)
Matt is on vacation due to the US holiday and will be back next week and
I'm sure he can sort this out.
…On Fri, Aug 29, 2025 at 9:51 AM Fabio ***@***.***> wrote:
*xfh* left a comment (rocicorp/mono#4671)
<#4671 (comment)>
I reproduce, thanks for the setup!
Adding time without time zone to the pgToZqlTypeMap does help with the
"unsupported error", but it is not enough. I am afraid I don't understand
the architectural design yet.
There is some conversion of ZQL types to Postgres types happening here in
sql.ts:
https://github.com/rocicorp/mono/blob/d981ea6721a1247d1a8f73839e61bb0b33b6327a/packages/z2s/src/sql.ts#L182-L232
But there is also the serialization & parsing logic in pg.ts:
https://github.com/rocicorp/mono/blob/d981ea6721a1247d1a8f73839e61bb0b33b6327a/packages/zero-cache/src/types/pg.ts#L193-L247
I don't understand, why there are two different approaches used, to
convert to native postgres types. The placeholder approach in sql.ts is
assuming that the ZQL value can be easily converted to postgres, mostly
through casting. The postgresTypeConfig in pg.ts makes of typescript to
serialize or parse values.
I've tested a time column in the hello-zero project (without custom
mutators) - there the time values are saved and synced as they should.
With custom mutators it doesn't work, because the value that is passed to
postgres is still milliseconds since midnight. It seems a bit fragile, to
implement the conversion twice. Wouldn't it be saver convert the value that
is given to the query as an argument using the serializer functions
implemented in pg.ts? I am probably not seeing the big picture here,
sorry.
@tantaman <https://github.com/tantaman>, maybe you could reconsider using
a string representation of TIME? It would remove all the complexity of
converting to and from milliseconds since midnight. Note also @aboodman
<https://github.com/aboodman>'s assumption that it was millis since
epoch. Since a time is independent of a date, it cannot be since epoch. In
my opinion millis are rather confusing for TIME values. Having a string
representation instead of a numeric one could indicate that time and date
values are fundamentally different.
In the meantime, I have found a way to add support for time in sql.ts
with the current millis since midnight approach:
case 'time':case 'time without time zone':
return `(INTERVAL '1 millisecond' * ${valuePlaceholder})::TIME`;
Let me know which direction you want to go and I'll implement tests and a
PR.
—
Reply to this email directly, view it on GitHub
<#4671 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATUBGMXTYOLC7T2AURHT33QCVKRAVCNFSM6AAAAACCPJZCQOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMZYGA4DINJZHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi
I have some existing database schema that uses postgres'
timedatatype.Zero doesn't allow me to sync these tables.
This is an attempt to add support for
time, relying on postgres' parser for string formats.https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME-INPUT-TIMES
I don't know how I can build a version of zero that I can test locally. Some test are failing, but unrelated to my changes.
Any guidance is welcome.