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

feat(graph, graphql, store): Support new Timestamp scalar #4758

Merged
merged 1 commit into from
Mar 21, 2024
Merged

feat(graph, graphql, store): Support new Timestamp scalar #4758

merged 1 commit into from
Mar 21, 2024

Conversation

dotansimha
Copy link
Contributor

@dotansimha dotansimha commented Jul 17, 2023 •
edited by yash251
Loading

TODO

  • Add new scalar
  • Handle SQL
  • Decide on how to format the values
  • PR in graph-tooling (New Timestamp scalar graph-tooling#1522)
  • Make it compile
  • Fix CI and tests
  • Use i64 instead of string
  • Add integration tests with graph-tooling

@lutter
Copy link
Collaborator

lutter commented Oct 24, 2023

I am very interested in getting this merged! Would be great to discuss how to get this PR ready - more than happy to help.

I fixed some of this up and put it onto the branch lutter/ts2 - it compiles, but there are lots of test failures.

I am also not sure how best to expose timestamps to AS; the way I did it right now is as seconds since the epoch, but that ties AS to second granularity. That might be a little too coarse in the future. We could do microseconds, but we'd also need some support to deal with timestamps in graph-tooling, and for that it might be better to expose the timestamps not as an i64 but in a more structured way.

As I said: let's discuss.

@lutter
Copy link
Collaborator

lutter commented Oct 24, 2023

Thinking about this some more, we should probably expose it in microseconds - an i64 would cover roughly +/- 290k years in microseconds which I think is plenty of range for timestamps.

@dotansimha
Copy link
Contributor Author

Thinking about this some more, we should probably expose it in microseconds - an i64 would cover roughly +/- 290k years in microseconds which I think is plenty of range for timestamps.

Yeah, this makes sense. Thank you @lutter for the help. I'm back on this today, focused on graph-tooling PR.
Will ping you when I have some progress!

@dotansimha dotansimha mentioned this pull request Nov 29, 2023
Merged
@dotansimha dotansimha force-pushed the ts-scalar branch 5 times, most recently from 963b5bc to f0eb537 Compare November 30, 2023 15:02
@dotansimha dotansimha force-pushed the ts-scalar branch 6 times, most recently from 6ce19e4 to 2dcc020 Compare January 30, 2024 15:11
@lutter lutter mentioned this pull request Feb 5, 2024
Open
5 tasks
@dotansimha dotansimha force-pushed the ts-scalar branch 3 times, most recently from ed751dc to 22ed8f5 Compare March 6, 2024 07:03
@dotansimha dotansimha force-pushed the ts-scalar branch 2 times, most recently from f8b471a to 636d891 Compare March 17, 2024 09:42
@dotansimha dotansimha marked this pull request as ready for review March 17, 2024 09:55
lutter
lutter requested changes Mar 19, 2024
View reviewed changes
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! This looks good; the two main things I am not sure about are:

  • passing timestamps between mappings and graph-node as milliseconds. Should we use microseconds to make sure we never have to change how this works?
  • converting inputs to internal timestamp types earlier rather than passing strings around (r::Value::Timestamp)

graph/src/data/graphql/object_macro.rs Outdated Show resolved Hide resolved
graph/src/data/store/mod.rs Outdated Show resolved Hide resolved
graph/src/data/store/mod.rs Outdated Show resolved Hide resolved
graph/src/data/store/scalar.rs Outdated Show resolved Hide resolved
graphql/src/values/coercion.rs Show resolved Hide resolved
runtime/wasm/src/asc_abi/class.rs Outdated Show resolved Hide resolved
store/postgres/examples/layout.rs Outdated Show resolved Hide resolved
store/postgres/src/relational_queries.rs Outdated Show resolved Hide resolved
store/test-store/tests/graphql/query.rs Outdated Show resolved Hide resolved
@dotansimha dotansimha force-pushed the ts-scalar branch 3 times, most recently from 6947c8b to 11fdb13 Compare March 19, 2024 11:37
@dotansimha
Copy link
Contributor Author

@lutter I've applied all suggestions. Tests seems to pass :) It's also using microseconds now. Thank you for the feedback!

@dotansimha dotansimha requested a review from lutter March 19, 2024 11:37
lutter
lutter approved these changes Mar 21, 2024
View reviewed changes
Copy link
Collaborator

@lutter lutter left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! I am letting this do one more CI run and will then merge it

graph/src/data/graphql/object_macro.rs Outdated Show resolved Hide resolved
@lutter lutter merged commit 52f4929 into master Mar 21, 2024
7 checks passed
@lutter lutter deleted the ts-scalar branch March 21, 2024 22:18
@alex-pakalniskis alex-pakalniskis mentioned this pull request May 21, 2024
Open
@azf20 azf20 mentioned this pull request May 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lutter lutter lutter approved these changes

@incrypto32 incrypto32 Awaiting requested review from incrypto32

@leoyvens leoyvens Awaiting requested review from leoyvens

@azf20 azf20 Awaiting requested review from azf20

Assignees

@dotansimha dotansimha

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dotansimha @lutter