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: Enable indexing for DateTime fields #2933

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

islamaliev
Copy link
Contributor

Relevant issue(s)

Resolves #2914

Description

Make indexes handle time.Time type as well.
For this encoding/decoding of time type is added to encoding package.

@islamaliev islamaliev changed the title Enable indexing for DateTime fields feat: Enable indexing for DateTime fields Aug 20, 2024
@islamaliev islamaliev self-assigned this Aug 20, 2024
@islamaliev islamaliev requested a review from a team August 20, 2024 07:57
@islamaliev islamaliev added the area/query Related to the query component label Aug 20, 2024
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 14.66667% with 64 lines in your changes missing coverage. Please review.

Project coverage is 79.33%. Comparing base (9422244) to head (311840f).
Report is 1 commits behind head on develop.

Files Patch % Lines
internal/db/fetcher/indexer_iterators.go 8.00% 23 Missing ⚠️
internal/encoding/time.go 23.33% 23 Missing ⚠️
internal/encoding/field_value.go 11.11% 15 Missing and 1 partial ⚠️
internal/encoding/type.go 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2933      +/-   ##
===========================================
- Coverage    79.50%   79.33%   -0.17%     
===========================================
  Files          325      326       +1     
  Lines        24781    24856      +75     
===========================================
+ Hits         19701    19718      +17     
- Misses        3670     3726      +56     
- Partials      1410     1412       +2     
Flag Coverage Δ
all-tests 79.33% <14.67%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
internal/encoding/encoding.go 100.00% <ø> (ø)
internal/encoding/type.go 88.24% <0.00%> (-11.76%) ⬇️
internal/encoding/field_value.go 64.36% <11.11%> (-11.55%) ⬇️
internal/db/fetcher/indexer_iterators.go 76.30% <8.00%> (-4.49%) ⬇️
internal/encoding/time.go 23.33% <23.33%> (ø)

... and 18 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9422244...311840f. Read the comment docs.

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.

LGTM thanks Islam :)

thought: I think (not in this PR) that the GQL code should change so that all field types are not supported by default, or we'll likely have this same issue in the future.

testUtils.CreateDoc{
Doc: `{
"name": "Fred",
"birthday": "2000-07-23T03:00:00-00:00"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Damn... I'm so much younger than I thought. I love this!

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

@islamaliev islamaliev merged commit 373f90b into develop Aug 20, 2024
39 of 41 checks passed
@islamaliev islamaliev deleted the feat/enable-indexing-on-time-field branch August 20, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/query Related to the query component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Composite unique index appears to ignore datetime components
3 participants