Added datetime support #1396

evanxg852000 · 2022-06-22T22:15:37Z

Implements the new DateTime field supporting feature in Tantivy.
Closes #1408

evanxg852000 · 2022-06-23T15:14:22Z

I will handle Date field deprecation in a following PR. The idea is to make use of the new DateTime field underneath with defaults that correspond to deprecating Date field. WDT?
I currently have this DateTimeOptions::set_default_input_timezone(mut self, timezone: Tz) -> DateTimeOptions . I would like to replace Tz with &str so that we don't have to expose chrono_tz, unfortunately Tz::from_str(&time_zone_name) returns a result, it looks kind of weird in the builder pattern.
please see https://github.com/quickwit-oss/tantivy/pull/1396/files#diff-d23fd0e9872657894b5ebb4f510cfbd6d099eee1e200c96e1c727f7322af0016R168-R173 what's you take?
Currently, I am implementing serialisation/deserialisation for types like DateTimePrecison and DateTimeOptions I feel like we can remove those and let tantivy users decide how they want to construct, and what mnemonic they want to use. Right?
See: https://github.com/quickwit-oss/tantivy/pull/1396/files#diff-d23fd0e9872657894b5ebb4f510cfbd6d099eee1e200c96e1c727f7322af0016R425-R464
https://github.com/quickwit-oss/tantivy/pull/1396/files#diff-d230b222363da840c9989a567adc9d16a32bcafaafd6e62fb85afcd94d4189c6R235-R245

codecov-commenter · 2022-06-23T15:23:17Z

Codecov Report

Merging #1396 (e61a59f) into main (2406d92) will decrease coverage by 0.00%.
The diff coverage is 90.53%.

@@            Coverage Diff             @@
##             main    #1396      +/-   ##
==========================================
- Coverage   94.23%   94.22%   -0.01%     
==========================================
  Files         236      237       +1     
  Lines       43685    43939     +254     
==========================================
+ Hits        41165    41400     +235     
- Misses       2520     2539      +19

Impacted Files	Coverage Δ
fastfield_codecs/src/bitpacked.rs	`100.00% <ø> (ø)`
src/fastfield/multivalued/mod.rs	`97.01% <ø> (ø)`
src/query/more_like_this/more_like_this.rs	`67.54% <0.00%> (ø)`
src/schema/flags.rs	`47.36% <0.00%> (-8.89%)`	⬇️
src/schema/mod.rs	`100.00% <ø> (ø)`
src/lib.rs	`96.48% <85.48%> (-1.14%)`	⬇️
src/schema/field_type.rs	`80.75% <90.00%> (-0.14%)`	⬇️
src/fastfield/mod.rs	`94.41% <90.90%> (-0.10%)`	⬇️
src/schema/date_time_options.rs	`91.33% <91.33%> (ø)`
src/aggregation/intermediate_agg_result.rs	`98.23% <100.00%> (+0.01%)`	⬆️
... and 29 more

Continue to review full report at Codecov.

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

fulmicoton · 2022-06-24T08:39:02Z

I will handle Date field deprecation in a following PR. The idea is to make use of the new DateTime field underneath with defaults that correspond to deprecating Date field. WDT?

Yes if it is possible that would be great. If it is not possible let me know.

I currently have this DateTimeOptions::set_default_input_timezone(mut self, timezone: Tz) -> DateTimeOptions . I would like to replace Tz with &str so that we don't have to expose chrono_tz, unfortunately Tz::from_str(&time_zone_name) returns a result, it looks kind of weird in the builder pattern.
please see https://github.com/quickwit-oss/tantivy/pull/1396/files#diff-d23fd0e9872657894b5ebb4f510cfbd6d099eee1e200c96e1c727f7322af0016R168-R173 what's you take?

If possible, I'd keep DateTime without timezone in tantivy and have a notion of timezone in quickwit... but I might not have thought this through so let me know what would be the obstacle. (Query parsing I assume?)

evanxg852000 · 2022-06-24T08:43:44Z

If possible, I'd keep DateTime without timezone in tantivy and have a notion of timezone in quickwit... but I might not have thought this through so let me know what would be the obstacle. (Query parsing I assume?)

We can leave timezone thing for quickwit, this just means we will let the Tantivy users know that when a timezone is not specified in the input, we assume UTC. I think that's also good for a library

fulmicoton · 2022-06-28T00:56:04Z

examples/date_time_field.rs

+fn main() -> tantivy::Result<()> {
+    // # Defining the schema
+    let mut schema_builder = Schema::builder();
+    let mut date_formats = HashSet::new();


surely this needs to be a ordered. HashSet is a very bad idea here.

fulmicoton · 2022-06-28T01:00:01Z

src/schema/field_type.rs

@@ -58,13 +65,14 @@ pub enum Type {
    Json = b'j',
 }

-const ALL_TYPES: [Type; 9] = [
+const ALL_TYPES: [Type; 10] = [
    Type::Str,
    Type::U64,
    Type::I64,
    Type::F64,
    Type::Bool,
    Type::Date,


The old Date type was actually a DateTime (yes the name sucks).

Is the plan to add DateTime and deprecate the older Date, or to replace right away?

So all these two types DateTime and PreciseDateTime exist because my plan is to keep Type::Date working with a deprecated label. If we can drop the old DateTime type and accept the breaking change let me know your opinion.

fulmicoton · 2022-06-28T01:02:10Z

src/schema/date_time_options.rs

+
+    // Accepted input format for parsing input as DateTime.
+    #[serde(default = "default_input_formats")]
+    input_formats: HashSet<DateTimeFormat>,


This cannot be an HashSet. Order matters here.

fulmicoton · 2022-06-28T01:05:10Z

src/schema/date_time.rs

+}
+
+impl fmt::Debug for PreciseDateTime {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {


Suggested change

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {

fulmicoton · 2022-06-28T01:05:40Z

src/schema/date_time.rs

+            OffsetDateTime::from_unix_timestamp_nanos(timestamp as i128)
+        }
+    }
+    .map_err(|error| error.to_string())


Suggested change

.map_err(|error| error.to_string())

.map_err(ToString::to_string)

fulmicoton · 2022-06-28T01:06:33Z

src/schema/date_time.rs

+    }
+
+    /// Convert to UTC `OffsetDateTime`
+    pub fn into_utc(self) -> OffsetDateTime {


fulmicoton · 2022-06-28T01:07:02Z

src/schema/date_time.rs

+    }
+
+    /// Returns the underlying timestamp.
+    pub fn get_timestamp(&self) -> i64 {


The name does not make it clear that this is with the given precision...
get_truncated_timestamp get_inner_timestamp...

fulmicoton · 2022-06-28T01:08:58Z

src/schema/date_time.rs

+    /// Converts to the underlying timestamp.
+    /// TODO: kept for backward compatibility, should we remove it?
+    pub const fn into_timestamp(self) -> i64 {
+        let Self { timestamp, .. } = self;


self.timestamp would have worked too

fulmicoton · 2022-06-28T01:09:53Z

src/schema/date_time.rs

+}
+
+impl DateTime {
+    /// Create new instance from UNIX timestamp.


Suggested change

/// Create new instance from UNIX timestamp.

/// Create new instance from UNIX timestamp (in seconds).

(I agree UNIX timestamp generally suggests seconds, but we might as well be explicit here)

fulmicoton · 2022-06-28T01:13:09Z

src/schema/date_time_options.rs

+    input_formats: HashSet<DateTimeFormat>,
+
+    // Internal storage precision, used to avoid storing
+    // very large numbers when not needed. This optimizes compression.


Does it apply to

indexing

stored

fast

index sorting
or only

fast
or only
-fast
-indexing?

It is not an obvious question and it should be detailed here.

fulmicoton · 2022-06-28T01:14:54Z

src/schema/date_time_options.rs

+
+impl From<DateTimeOptions> for DateTimeParsersHolder {
+    fn from(opts: DateTimeOptions) -> Self {
+        let mut string_parsers: VecDeque<StringDateTimeParser> = VecDeque::new();


why not Vec?

fulmicoton · 2022-06-28T01:25:47Z

src/schema/date_time_options.rs

+        for (index, parser) in self.string_parsers.iter().enumerate() {
+            if let Ok(date_time) = parser(&value) {
+                // Move successful parser in front of queue.
+                self.string_parsers.swap(0, index);


Ah I see...
I think Vec has swap too.

I am not sure this is a good idea.

So with the HashSet defintion, the output of quickwit right now is not really deterministic at compile time.
Date format can be ambiguous ( think about MM-DD-YYY vs DD-MM-YYYY and 01/03/2021).
Which means that our output depends on the order of our date formats.

By using a HashSet, you make the output of quickwit dependent on the implementation of hashset.
(A change in the rust standard library fro instance, would break unit tests.)
We need to stay away from HashSet.

I would also stay away from the reordering logic, but I wonder...
So it has two functions I believe:

a) performance.

b) best effort to deal with the ambiguity described above.

a) can be partially addressed by extracting ez to check predicate for each format.
For instance on the date string length.

If the date is not exactly 10chrs. skip format XXXXX.

b) is tricky. With your solution we can "eventually" find the right format when we reach a date that is unambiguous (e.g. 24/02/2021). On the other hand, I think we should give all the tools to the user to tell use the right thing. I'd rather not do the swapping logic and just rely on the priority logic.

fulmicoton · 2022-06-28T01:30:09Z

src/schema/date_time_options.rs

+#[derive(Clone)]
+pub struct DateTimeParsersHolder {
+    /// Functions for parsing string input as datetime.
+    string_parsers: VecDeque<StringDateTimeParser>,


fulmicoton · 2022-06-28T01:39:13Z

src/schema/date_time.rs

+    pub DateTime,
+);
+
+impl Deref for PreciseDateTime {


But the inner DateTime is incorrect right? The precision here is arbitrary.
We don't want to expose it.

fulmicoton · 2022-06-28T01:39:43Z

src/schema/date_time.rs

+// It only exists to provide support the underlying fast
+// field value for both `[Value::Date]` and `[Value::DateTime]` variants.
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub struct PreciseDateTime(


Rename to TruncatedTimestamp maybe.

fulmicoton · 2022-06-28T01:40:24Z

src/schema/date_time.rs

+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("DateTime")
+            .field("timestamp", &self.0.timestamp)
+            .field("precision", &self.0.precision)


But the precision is wrong isn't it? Also we don't want to print DateTime here.

Precision is always correct, whether you use DateTime::from_unix_timestamp(u64) or DateTime::from_utc_with_precision(utc_dt, precison) constructor. Maybe the names should be:

DateTime -> DateTimeWithSecondPrecision (old DateTime)

PreciseDateTime -> DateTimeWithCustomPrecision (for new fast field)

fulmicoton · 2022-06-28T01:47:21Z

I don't understand the logic around PreciseDateTime.

In the FastField we store truncated values I assume? (e.g. a milliseconds precision timestamp for instance).
Then I understand that you want a type on which you can implement FastValue. This means you create the type by taking the int without really knowing the precision. The schema is then needed to deal with this.
Then why do we expose the inner DateTime? That seems error-prone.

evanxg852000 · 2022-06-28T06:33:16Z

So PreciseDateTime is the new date time with actual precision, currently, DateTime API was enhanced to conform to the old DateTime that's why I kept the old API and added few things. This allowed using DateTime like before and not breaking existing code. PreciseDateTime is the one that actually makes use of the new API. As I said in the comment it only exist because I could implement both fastfield types for the same DateTime. I don't want to rip something without us agreeing so DateTime behaves as the way it use to with a precison -> seconds and the timestamp is second. when using the DateTime field, precison can change to millis or ... .
We always store the computed timestamp value then when extracting the date the precision is used.
In both DateTime (seconds precision) and PreciseDateTime (any precision) the precison is always correct. I think I need to find a better name.
By exposing the inner DateTime, user only needs to worry about that type that already existed.

fulmicoton · 2022-06-28T07:37:02Z

In both DateTime (seconds precision) and PreciseDateTime (any precision) the precison is always correct.

@evanxg852000 I don't see how this is possible... In the FastValue of PreciseDateTime, from assumes that we have seconds in the fast field, and the conversion to a fastvalue is taking whatever truncated value we had.

evanxg852000 · 2022-07-07T16:23:42Z

src/indexer/json_term_writer.rs

@@ -325,7 +327,7 @@ impl<'a> JsonTermWriter<'a> {
        self.close_path_and_set_type(T::to_type());
        self.term_buffer
            .as_mut()
-            .extend_from_slice(val.to_u64().to_be_bytes().as_slice());
+            .extend_from_slice(val.to_u64(None).to_be_bytes().as_slice());


@fulmicoton I am not applying the precision here to DateTime. my reasoning is that this is all indexing though we are pushing fast values. Am I wrong?

Great question!

FastValue is a poorly named trait.
It is not only used for fast values, it is also used for the inverted index.

In reality it offers a strictly monotonic mapping from the type to U64 (and its reciprocal).

We could be named MonoMapU64 or something like that.
Can you fill a ticket to do this rename?

Back to your question... in the json_term_writer we should apply some truncation on dates.

I suggest we always truncate with second precision?

We should probably truncate to seconds always for regular (=not Json) date fields too; and only use the precision setting for fast fields.

Besides this is the current behavior of tantivy in a sense.

examples/date_time_field.rs

fastfield_codecs/src/bitpacked.rs

fulmicoton · 2022-07-08T06:31:15Z

src/lib.rs

+    }
+
+    /// Convert to UNIX timestamp in microseconds.
+    pub const fn into_timestamp_nanos(self) -> i64 {


i think we should ditch the nanos one. It could suggest we have nanos precision.

guilload · 2022-07-08T06:31:39Z

src/lib.rs

+    /// Create new from UNIX timestamp in nanoseconds.
+    pub const fn from_timestamp_nanos(nanoseconds: i128) -> Self {
+        Self {
+            timestamp_micros: (nanoseconds as i64 / 1_000),


We should divide into an i128 first and then convert to i64. See for yourself. Can you add the corresponding unit test?

inlined it and added a test for it.

fulmicoton · 2022-07-08T06:32:34Z

src/lib.rs

+    }
+
+    /// Create new from UNIX timestamp in nanoseconds.
+    pub const fn from_timestamp_nanos(nanoseconds: i128) -> Self {


I would remove that one (and inline the implem in from_utc)

fulmicoton · 2022-07-08T06:37:04Z

src/fastfield/mod.rs

@@ -62,7 +63,9 @@ pub trait FastValue: Clone + Copy + Send + Sync + PartialOrd + 'static {
    /// Converts a value to u64.
    ///
    /// Internally all fast field values are encoded as u64.
-    fn to_u64(&self) -> u64;


I understand you use the field type for truncation on datetime, but it seems too convoluted. Let's not change the to_u64.

Instead, let's put the truncation at the fast field writer level.

fulmicoton · 2022-07-08T06:39:47Z

src/fastfield/writer.rs

@@ -305,7 +335,7 @@ impl IntFastFieldWriter {
    pub fn add_document(&mut self, doc: &Document) {
        match doc.get_first(self.field) {
            Some(v) => {
-                self.add_val(super::value_to_u64(v));
+                self.add_val(super::value_to_u64(v, &self.field_type));


We can put the truncation logic here.

let mut val = super::value_to_u64(v, self.field_type); if let Some(precision) = self.precision_opt { val = precision.truncate(val); }

What's a bit lame is that we will have to do it elsewhere for indexing.

fulmicoton · 2022-07-08T06:57:29Z

src/indexer/segment_writer.rs

@@ -248,7 +248,7 @@ impl SegmentWriter {
                FieldType::Date(_) => {
                    for value in values {
                        let date_val = value.as_date().ok_or_else(make_schema_error)?;
-                        term_buffer.set_u64(date_val.to_u64());
+                        term_buffer.set_u64(date_val.to_u64(None));


See discussion in json_term_writer.
We want some truncation here.

I suggest we truncate to the second all of the time.

fulmicoton · 2022-07-08T07:04:58Z

src/schema/date_time_options.rs

+
+/// DateTime Precision
+#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
+pub enum DatePrecision {


"precision": "Milliseconds"
does not look great in a JSON.

`"precision": "milliseconds" would look better IMHO

can you add attriubte to make the serde serialization look nicer?
(serde lower should do the job.)

fulmicoton · 2022-07-08T07:07:11Z

src/collector/histogram_collector.rs

@@ -72,7 +72,7 @@ impl HistogramComputer {
            return;
        }
        let delta = value - self.min_value;
-        let delta_u64 = delta.to_u64();
+        let delta_u64 = delta.to_u64(None);


we can remove that line anyway I think.

delta_u64 = delta

fulmicoton

see comments inline

fulmicoton · 2022-07-11T06:22:57Z

src/schema/date_time_options.rs

+
+impl Default for DateOptions {
+    fn default() -> Self {
+        Self {


You have clippy warnings here. Either use derive(Default) or add an annotation to make sure Clippy keeps quiet.
I'm ok with both solutions.

used Default, shorter & the recommended Clippy way

… field precision configurable

fulmicoton · 2022-07-12T01:04:01Z

Good job!

twe4ked · 2022-09-17T03:07:14Z

Thanks for this! Any chance of a release with this included soon? 🙏

PSeitz · 2022-09-18T02:36:30Z

@twe4ked The next release will be a breaking release, so we try to add more stuff into it. Maybe a release in around 1 month

evanxg852000 force-pushed the support-datetime-field branch from c8a066d to d6f27bc Compare June 23, 2022 14:50

evanxg852000 requested a review from fulmicoton June 23, 2022 14:53

evanxg852000 force-pushed the support-datetime-field branch from 8a07c12 to 4b1ce74 Compare June 24, 2022 11:32

fulmicoton reviewed Jun 28, 2022

View reviewed changes

src/schema/date_time.rs Outdated

}

/// Convert to UTC `OffsetDateTime`

pub fn into_utc(self) -> OffsetDateTime {

Copy link

Collaborator

fulmicoton Jun 28, 2022

Choose a reason for hiding this comment

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

good idea

fulmicoton reviewed Jun 28, 2022

View reviewed changes

evanxg852000 commented Jul 7, 2022

View reviewed changes

evanxg852000 changed the title ~~Added datetime support [W.I.P]~~ Added datetime support Jul 7, 2022

evanxg852000 requested a review from guilload July 7, 2022 16:35

evanxg852000 marked this pull request as ready for review July 7, 2022 16:36

fulmicoton reviewed Jul 8, 2022

View reviewed changes

examples/date_time_field.rs Show resolved Hide resolved

fulmicoton reviewed Jul 8, 2022

View reviewed changes

fastfield_codecs/src/bitpacked.rs Show resolved Hide resolved

fulmicoton reviewed Jul 8, 2022

View reviewed changes

guilload reviewed Jul 8, 2022

View reviewed changes

fulmicoton reviewed Jul 8, 2022

View reviewed changes

fulmicoton requested changes Jul 8, 2022

View reviewed changes

evanxg852000 force-pushed the support-datetime-field branch 2 times, most recently from 506197e to 22df8d6 Compare July 10, 2022 16:01

fulmicoton reviewed Jul 11, 2022

View reviewed changes

fulmicoton approved these changes Jul 11, 2022

View reviewed changes

evanxg852000 force-pushed the support-datetime-field branch from a5929b4 to b4a80b0 Compare July 11, 2022 08:32

Updated DateTime to hold timestamp in microseconds, while making date…

0035969

… field precision configurable

evanxg852000 force-pushed the support-datetime-field branch from e61a59f to 0035969 Compare July 11, 2022 09:01

fulmicoton approved these changes Jul 12, 2022

View reviewed changes

fulmicoton merged commit a4be239 into main Jul 12, 2022

fulmicoton deleted the support-datetime-field branch July 12, 2022 01:04

waywardmonkeys mentioned this pull request Sep 26, 2022

Document that dates are truncated at the second precision. #1287

Closed

This was referenced Jan 13, 2023

truncation comment PSeitz/tantivy#30

Closed

use stats PSeitz/tantivy#31

Closed

PSeitz mentioned this pull request Jan 31, 2023

update lz4 flex PSeitz/tantivy#33

Closed

fmassot mentioned this pull request May 3, 2023

Add precision to datetime field and datetime field internally expressed in microsecs. #1408

Closed

	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {

	.map_err(\|error\| error.to_string())
	.map_err(ToString::to_string)

	/// Create new instance from UNIX timestamp.
	/// Create new instance from UNIX timestamp (in seconds).

Added datetime support #1396

Added datetime support #1396

Conversation

evanxg852000 commented Jun 22, 2022 • edited Loading

evanxg852000 commented Jun 23, 2022 • edited by fulmicoton Loading

codecov-commenter commented Jun 23, 2022 • edited Loading

Codecov Report

fulmicoton commented Jun 24, 2022

evanxg852000 commented Jun 24, 2022 • edited Loading

fulmicoton Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

fulmicoton Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

fulmicoton Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton commented Jun 28, 2022

evanxg852000 commented Jun 28, 2022 • edited Loading

fulmicoton commented Jun 28, 2022

Choose a reason for hiding this comment

fulmicoton Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton left a comment

Choose a reason for hiding this comment

fulmicoton Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton commented Jul 12, 2022

twe4ked commented Sep 17, 2022

PSeitz commented Sep 18, 2022

evanxg852000 commented Jun 22, 2022 •

edited

Loading

evanxg852000 commented Jun 23, 2022 •

edited by fulmicoton

Loading

codecov-commenter commented Jun 23, 2022 •

edited

Loading

evanxg852000 commented Jun 24, 2022 •

edited

Loading

fulmicoton Jun 28, 2022 •

edited

Loading

fulmicoton Jun 28, 2022 •

edited

Loading

fulmicoton Jun 28, 2022 •

edited

Loading

fulmicoton Jun 28, 2022 •

edited

Loading

fulmicoton Jun 28, 2022 •

edited

Loading

fulmicoton Jun 28, 2022 •

edited

Loading

evanxg852000 commented Jun 28, 2022 •

edited

Loading

fulmicoton Jul 8, 2022 •

edited

Loading

fulmicoton Jul 8, 2022 •

edited

Loading

fulmicoton Jul 8, 2022 •

edited

Loading

fulmicoton Jul 11, 2022 •

edited

Loading