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

feat: Add DateTime custom scalars #931

Merged
merged 27 commits into from
Nov 19, 2022
Merged

feat: Add DateTime custom scalars #931

merged 27 commits into from
Nov 19, 2022

Conversation

jsimnz
Copy link
Member

@jsimnz jsimnz commented Nov 4, 2022

Relevant issue(s)

Resolves #676

Description

Adds support for DateTime custom scalar types in schemas and filter query arguments. At the moment this doesn't support NonNull DateTimes, as I wanted to implement as a follow-up PR in case the core design had push back.

There's a few points of attention, identified by @TODO asking for design input. Most notably on the schema object access within a CollectionAPI call, which has to use a type cast from the parser, and the parser schema manager being public.

There was also a need to update 1) int64 to int and 2) gql.NullValue export for input argument value parsing due to the use of the new GQL value coercion.

The general implementation revolves around a newly exposed API on the GQL lib called ValueFromAST and GetFieldType, which will convert an AST type to a Go native type when provided the cooresponding gql.Input type from the Schema.

Commits are fairly clean and in order of work.

PS. Theres a decent number of code changes which I would usually like to avoid just to implement DateTime. But this requires a new way to parse gql input objects which includes any custom scalars. Theres just the addition of glue code for defining the FieldKinds and the connor evaluation types.

Tasks

How has this been tested?

  • Local / Manual
  • New integration tests

Specify the platform(s) on which this was tested:

  • Ubuntu (WSL2)

@jsimnz jsimnz added feature New feature or request area/query Related to the query component area/schema Related to the schema system area/parser Related to the parser components action/no-benchmark Skips the action that runs the benchmark. labels Nov 4, 2022
@jsimnz jsimnz added this to the DefraDB v0.4 milestone Nov 4, 2022
@jsimnz jsimnz self-assigned this Nov 4, 2022
@jsimnz jsimnz requested a review from a team November 4, 2022 09:39
Copy link
Member Author

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

questions...

connor/eq.go Outdated Show resolved Hide resolved
connor/time/equality.go Show resolved Hide resolved
db/collection_update.go Outdated Show resolved Hide resolved
@jsimnz
Copy link
Member Author

jsimnz commented Nov 4, 2022

Note: I haven't officially looked yet, but the Change Detection tests are likely failing because there was a schema change of one of the integration test objects. Not 100% sure how to best define that issue on the change detection file since its not related to the core function of the database, but just breaks a test from one version to another.

cc: @AndrewSisley

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Looks good minus some localized issues (and the inherent awkwardness of the solution which has been discussed already and is out of scope).

client/descriptions.go Outdated Show resolved Hide resolved
connor/eq.go Show resolved Hide resolved
connor/eq.go Outdated Show resolved Hide resolved
connor/eq.go Outdated Show resolved Hide resolved
connor/ge.go Show resolved Hide resolved
query/graphql/parser/query.go Outdated Show resolved Hide resolved
query/graphql/parser/query.go Outdated Show resolved Hide resolved
query/graphql/schema/descriptions.go Show resolved Hide resolved
@AndrewSisley
Copy link
Contributor

Note: I haven't officially looked yet, but the Change Detection tests are likely failing because there was a schema change of one of the integration test objects. Not 100% sure how to best define that issue on the change detection file since its not related to the core function of the database, but just breaks a test from one version to another.

There should be a few examples of this in the change-documentation directory already. Just be really sure you havent actually changed anything first (suggest doing this right before merging, just in case)

@AndrewSisley
Copy link
Contributor

Just a quick sanity check - you are not persisting Time as strings in the datastore correct?

@jsimnz
Copy link
Member Author

jsimnz commented Nov 4, 2022

Just a quick sanity check - you are not persisting Time as strings in the datastore correct?

Realized I haven't looked that persistence w.r.t DateTime. Its very possible that it actually is off the top of my head, would need to confirm.

The options for DB persistence is 1) RFC3339 formatted stings (as its doing now), 2) convert to unixtime (millis).

Strings are obvi far more space, so we should prob do unixtime serialize uint64 as I think your likely suggesting here.

@jsimnz
Copy link
Member Author

jsimnz commented Nov 7, 2022

and the inherent awkwardness of the solution

Can you expand on this comment, missed it during my first review @AndrewSisley

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #931 (d310396) into develop (4b14839) will decrease coverage by 0.15%.
The diff coverage is 60.36%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #931      +/-   ##
===========================================
- Coverage    60.35%   60.19%   -0.16%     
===========================================
  Files          164      165       +1     
  Lines        17732    17884     +152     
===========================================
+ Hits         10702    10766      +64     
- Misses        6082     6155      +73     
- Partials       948      963      +15     
Impacted Files Coverage Δ
client/descriptions.go 84.61% <ø> (ø)
connor/in.go 58.33% <ø> (ø)
connor/ge.go 58.33% <48.27%> (-14.40%) ⬇️
connor/le.go 58.33% <48.27%> (-14.40%) ⬇️
connor/gt.go 60.00% <51.72%> (-16.20%) ⬇️
connor/lt.go 61.11% <51.72%> (-16.17%) ⬇️
connor/time/equality.go 59.09% <59.09%> (ø)
query/graphql/parser/mutation.go 80.21% <60.00%> (-5.15%) ⬇️
query/graphql/parser/subscription.go 74.46% <60.00%> (-9.75%) ⬇️
query/graphql/parser/commit.go 78.57% <62.50%> (-2.45%) ⬇️
... and 9 more

@AndrewSisley
Copy link
Contributor

Can you expand on this comment, missed it during my first review

This is RE the convo we had before, where other alternative solutions involved changing the gql parsing logic (currently the gql-fork), or doing this somewhere a little more isolated (a func that would have to re-traverse the tree - doing roughly what this PR does, but in a more isolated code block at the cost of some performance and the ugliness of iterating/parsing it in an extra step)

client/descriptions.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
connor/ge.go Outdated Show resolved Hide resolved
core/parser.go Outdated Show resolved Hide resolved
db/collection_update.go Show resolved Hide resolved
query/graphql/parser/mutation.go Outdated Show resolved Hide resolved
query/graphql/parser/query.go Outdated Show resolved Hide resolved
query/graphql/parser/mutation.go Outdated Show resolved Hide resolved
query/graphql/parser/query.go Outdated Show resolved Hide resolved
@jsimnz jsimnz dismissed stale reviews from AndrewSisley and shahzadlone November 19, 2022 11:53

Finished

@jsimnz jsimnz merged commit 82e0d2f into develop Nov 19, 2022
@jsimnz jsimnz deleted the jsimnz/feat/date-time branch November 19, 2022 11:57
@jsimnz jsimnz mentioned this pull request Nov 28, 2023
4 tasks
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Adds support for DateTime custom scalar types in schemas and filter query arguments. At the moment this doesn't support NonNull DateTimes yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/parser Related to the parser components area/query Related to the query component area/schema Related to the schema system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete support for scalar DateTime types
4 participants