DateTime payload index by xzfc · Pull Request #3395 · qdrant/qdrant

xzfc · 2024-01-15T16:58:24Z

This PR adds datetime payload index. /claim #3320

Open for discussion/feedback:

Internal representation. Current format: milliseconds in i64. Possible alternatives: nanoseconds in u64 or seconds in f64.
Timestamp representation. Current format: RFC3339, e.g., 2019-01-01T00:00:00.000Z.
Currently datetime_range is a separate key. Need to investigate if this could be merged with the range key in a straightforward way.

All Submissions:

Contributions should target the dev branch. Did you create your branch from dev?
Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

Does your submission pass tests?
Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
Have you checked your code using cargo clippy --all --all-features command?

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

algora-pbc · 2024-01-15T16:58:34Z

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe/Alipay.

coszio

Great work here @xzfc!
My only concern is the at the api layer. We should keep the Range type at this level to not break backward compatibility in the clients. A newtype instead of generic for datetime range may fix this.

Also, I would love the tests to include some non-UTC timezone examples as a sanity check.

docs/redoc/master/openapi.json

lib/api/build.rs

lib/segment/src/index/field_index/field_index_base.rs

openapi/tests/openapi_integration/test_payload_indexing.py

timvisee · 2024-01-16T09:51:37Z

Some comments on your open questions, but I haven't gone through your code yet:

Internal representation. Current format: milliseconds in i64. Possible alternatives: nanoseconds in u64 or seconds in f64.

I think this is a good choice. This way we have a constant resolution of 1 millisecond, while we'd loose precision with f64.

Though, what's the maximum value we can represent here if milliseconds and i64?

Later we can add parameterization to this to support different resolutions, seconds or nanoseconds for example. Each could have an optimized index variant.

I do wonder whether we'd somehow want to make this timezone aware as well. Maybe we shouldn't care for the index and always use UTC.

Timestamp representation. Current format: RFC3339, e.g., 2019-01-01T00:00:00.000Z.

Perfect. Following a solid standard.

xzfc · 2024-01-16T12:12:37Z

@timvisee

Though, what's the maximum value we can represent here if milliseconds and i64?

I've checked the supported datetime ranges for chrono library and i64 values.

unit	range in years
seconds	584,942,417,355
milliseconds	584,942,417
microseconds	584,942
nanoseconds	584

Microseconds in i64 is enough to cover the whole range of dates supported by chrono (~524288 years). So if we stick to chrono, no reason to use milliseconds instead of microseconds.
Nanoseconds in i64 would give us a range of about 584 years, from 1677 to 2262.
Chrono doesn't support sub-nanosecond accuracy.

Seconds in f64 would give us sub-microsecond precision for dates in the current year, and sub-millisecond precision for dates in the max year supported by chrono (262143).

calculations

$ python3 -c 'import math,time;t=time.time();print(f"{(math.nextafter(t, t+1)-t) * 1e6:.10f} µs")'

0.2384185791 µs

$ python3 -c 'import math,time;t=8210298412799.0;print(f"{(math.nextafter(t, t+1)-t) * 1e6:.10f} µs")'

976.5625000000 µs

I'll switch to microseconds for now if you don't have objections.

timvisee · 2024-01-17T09:41:28Z

@xzfc Thank you so much for diving into this.

I'll switch to microseconds for now if you don't have objections.

No objections, we can extend/change this later if desired.

coszio

Thanks for improving this @xzfc, looks much better now.

I made a few improvements to push this to the endline:

restored the enum variant approach
other minor stuff

I'm not sure how much I like approving macros, which are hard to maintain, but we can refactor that later.

coszio · 2024-01-23T13:00:04Z

lib/segment/src/types.rs

+#[macro_rules_attribute::macro_rules_derive(crate::common::macros::schemars_rename_generics)]
+#[derive_args(<FloatPayloadType> => "Range", <DateTimePayloadType> => "DatetimeRange")]


openapi/tests/openapi_integration/test_payload_indexing.py

lib/segment/src/index/field_index/index_selector.rs

lib/segment/src/index/field_index/field_index_base.rs

lib/api/Cargo.toml

lib/api/src/grpc/conversions.rs

lib/segment/src/index/field_index/index_selector.rs

timvisee

@xzfc Thanks a lot for your effort on implementing this! ✔️

This reverts commit d55f251.

This reverts commit c5255f6.

* Datetime payload index * Introduce IndexMapItem * Drop FieldIndex::DatetimeIndex * Rename OpenAPI struct names * Switch to microseconds * Validate and serialize grpc timestamps * Add tests with different timezones * minor review fixes * Revert "Drop FieldIndex::DatetimeIndex" This reverts commit d55f251. * Revert "Introduce IndexMapItem" This reverts commit c5255f6. * fix: back to microseconds after reverts * extract range conversion from boxed checker fn * add log to deps * don't run macro doctest * no_run -> ignore * remove prost-types in favor of prost-wkt-types * better assertion on test_payload_indexing.py * propagate unparsable datetime --------- Co-authored-by: Luis Cossío <luis.cossio@outlook.com>

algora-pbc bot mentioned this pull request Jan 15, 2024

Implement datetime support for the payload index #3320

Closed

algora-pbc bot added the 🙋 Bounty claim label Jan 15, 2024

coszio requested changes Jan 15, 2024

View reviewed changes

generall requested a review from coszio January 22, 2024 13:54

coszio force-pushed the 3320-datetime-index branch 2 times, most recently from 62381ba to 41d5134 Compare January 22, 2024 20:26

coszio approved these changes Jan 23, 2024

View reviewed changes

coszio requested a review from generall January 23, 2024 15:20

xzfc commented Jan 23, 2024

View reviewed changes

openapi/tests/openapi_integration/test_payload_indexing.py Outdated Show resolved Hide resolved

timvisee force-pushed the 3320-datetime-index branch 2 times, most recently from d4c8d81 to 03d3a9a Compare January 25, 2024 15:09

timvisee changed the title ~~Datetime payload index~~ DateTime payload index Jan 25, 2024

timvisee requested changes Jan 25, 2024

View reviewed changes

IvanPleshkov self-requested a review January 26, 2024 13:28

coszio requested a review from timvisee January 26, 2024 13:45

timvisee approved these changes Jan 29, 2024

View reviewed changes

timvisee force-pushed the 3320-datetime-index branch from 12b3ac6 to 469c060 Compare January 30, 2024 09:57

xzfc and others added 9 commits January 31, 2024 10:20

Datetime payload index

c445ab1

Introduce IndexMapItem

897c79c

Drop FieldIndex::DatetimeIndex

9abeea6

Rename OpenAPI struct names

f3be2f6

Switch to microseconds

fbe3180

Validate and serialize grpc timestamps

36fe4d1

Add tests with different timezones

63aebe9

minor review fixes

9cc601e

Revert "Drop FieldIndex::DatetimeIndex"

6ff02c3

This reverts commit d55f251.

coszio added 9 commits January 31, 2024 10:22

Revert "Introduce IndexMapItem"

869ae7e

This reverts commit c5255f6.

fix: back to microseconds after reverts

d872a4b

extract range conversion from boxed checker fn

668315d

add log to deps

0c574ba

don't run macro doctest

044bc25

no_run -> ignore

8570c73

remove prost-types in favor of prost-wkt-types

88c792d

better assertion on test_payload_indexing.py

3c0bb01

propagate unparsable datetime

11788ad

timvisee force-pushed the 3320-datetime-index branch from 469c060 to 11788ad Compare January 31, 2024 09:44

timvisee merged commit 41784a2 into qdrant:dev Jan 31, 2024

This was referenced Feb 5, 2024

Extend datetime formats in REST API #3529

Closed

Better error response for wrong datetime format in REST filter #3531

Closed

xzfc mentioned this pull request Mar 3, 2026

Remove shard dependency on api #8284

Merged

		#[macro_rules_attribute::macro_rules_derive(crate::common::macros::schemars_rename_generics)]
		#[derive_args(<FloatPayloadType> => "Range", <DateTimePayloadType> => "DatetimeRange")]

Conversation

xzfc commented Jan 15, 2024

All Submissions:

New Feature Submissions:

Changes to Core Features:

Uh oh!

algora-pbc bot commented Jan 15, 2024

Uh oh!

coszio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timvisee commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xzfc commented Jan 16, 2024

Uh oh!

timvisee commented Jan 17, 2024

Uh oh!

coszio left a comment

Choose a reason for hiding this comment

Uh oh!

coszio Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timvisee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timvisee commented Jan 16, 2024 •

edited

Loading