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

Add new `default` option for timestamp field #7036

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@dadoonet
Copy link
Member

commented Jul 25, 2014

Index process fails when having _timestamp enabled and path option is set.
It fails with a TimestampParsingException[failed to parse timestamp [null]] message.

Reproduction:

DELETE test
PUT  test
{
    "mappings": {
        "test": {
            "_timestamp" : {
                "enabled" : "yes",
                "path" : "post_date"
            }
        }
    }
}
PUT test/test/1
{
  "foo": "bar"
}

You can now define a default value for when timestamp is not provided
within the index request or in the _source document.

By default, the default value is now which means the date the document was processed by the indexing chain.

You can disable that default value by setting default to null. It means that timestamp is mandatory:

{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "default" : null
        }
    }
}

If you don't provide any timestamp value, indexation will fail.

You can also set the default value to any date respecting timestamp format:

{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "format" : "YYYY-MM-dd",
            "default" : "1970-01-01"
        }
    }
}

If you don't provide any timestamp value, indexation will fail.

Closes #4718.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2014

Should the default be null rather than now when a path is specified? I don't like the fact that a malformed document would get the current time as a timestamp instead of raising an error?

@jpountz

View changes

src/main/java/org/elasticsearch/cluster/metadata/MappingMetaData.java Outdated
@@ -528,6 +541,7 @@ public static void writeTo(MappingMetaData mappingMd, StreamOutput out) throws I
out.writeBoolean(false);
}
out.writeString(mappingMd.timestamp().format());
out.writeString(mappingMd.timestamp().defaultTimestamp());

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 28, 2014

Contributor

This breaks the stream backward compatibility. The default timestamp should only be written if the output version is >= 1.4.0

This comment has been minimized.

Copy link
@dadoonet

dadoonet Jul 28, 2014

Author Member

thanks!

@jpountz

View changes

src/main/java/org/elasticsearch/cluster/metadata/MappingMetaData.java Outdated
@@ -565,7 +579,7 @@ public static MappingMetaData readFrom(StreamInput in) throws IOException {
// routing
Routing routing = new Routing(in.readBoolean(), in.readBoolean() ? in.readString() : null);
// timestamp
Timestamp timestamp = new Timestamp(in.readBoolean(), in.readBoolean() ? in.readString() : null, in.readString());
Timestamp timestamp = new Timestamp(in.readBoolean(), in.readBoolean() ? in.readString() : null, in.readString(), in.readString());

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 28, 2014

Contributor

same here

@jpountz jpountz removed the review label Jul 28, 2014

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2014

About default value it was more about not to break bwc. Before this change, when you activate _timestamp in mapping, if you don't set any timestamp in the request, "now" is applied by default.

I think we should keep it as it was previously.

dadoonet added some commits Jul 25, 2014

Generate timestamp when path is null
Index process fails when having `_timestamp` enabled and `path` option is set.
It fails with a `TimestampParsingException[failed to parse timestamp [null]]` message.

Reproduction:

```
DELETE test
PUT  test
{
    "mappings": {
        "test": {
            "_timestamp" : {
                "enabled" : "yes",
                "path" : "post_date"
            }
        }
    }
}
PUT test/test/1
{
  "foo": "bar"
}
```

You can define a default value for when timestamp is not provided
within the index request or in the `_source` document.

By default, the default value is `now` which means the date the document was processed by the indexing chain.

You can disable that default value by setting `default` to `null`. It means that `timestamp` is mandatory:

```
{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "default" : null
        }
    }
}
```

If you don't provide any timestamp value, indexation will fail.

You can also set the default value to any date respecting timestamp format:

```
{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "format" : "YYYY-MM-dd",
            "default" : "1970-01-01"
        }
    }
}
```

If you don't provide any timestamp value, indexation will fail.

Closes #4718.
assertThat(request.timestamp(), notNullValue());

// TODO compute that generated date is closed to current date

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 29, 2014

Contributor

Should you address these TODOs?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Jul 29, 2014

Author Member

Yeah! I just saw that I forgot to code that part :(

@@ -528,6 +542,9 @@ public static void writeTo(MappingMetaData mappingMd, StreamOutput out) throws I
out.writeBoolean(false);
}
out.writeString(mappingMd.timestamp().format());
if (out.getVersion().onOrAfter(Version.V_1_4_0)) {
out.writeString(mappingMd.timestamp().defaultTimestamp());

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 29, 2014

Contributor

writeString would fail if the default timestamp is null.

So I think we would also need to write a boolean to tell whether it is not null? (and an integration test that would exercise serialization)

This comment has been minimized.

Copy link
@dadoonet

dadoonet Jul 29, 2014

Author Member

Argh! Right!

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2014

@dadoonet Left one comment about the null case

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2014

@jpountz PR updated. Let me know

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2014

LGTM

@jpountz jpountz removed the review label Jul 30, 2014

dadoonet added a commit that referenced this pull request Jul 31, 2014

Generate timestamp when path is null
Index process fails when having `_timestamp` enabled and `path` option is set.
It fails with a `TimestampParsingException[failed to parse timestamp [null]]` message.

Reproduction:

```
DELETE test
PUT  test
{
    "mappings": {
        "test": {
            "_timestamp" : {
                "enabled" : "yes",
                "path" : "post_date"
            }
        }
    }
}
PUT test/test/1
{
  "foo": "bar"
}
```

You can define a default value for when timestamp is not provided
within the index request or in the `_source` document.

By default, the default value is `now` which means the date the document was processed by the indexing chain.

You can disable that default value by setting `default` to `null`. It means that `timestamp` is mandatory:

```
{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "default" : null
        }
    }
}
```

If you don't provide any timestamp value, indexation will fail.

You can also set the default value to any date respecting timestamp format:

```
{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "format" : "YYYY-MM-dd",
            "default" : "1970-01-01"
        }
    }
}
```

If you don't provide any timestamp value, indexation will fail.

Closes #4718.
Closes #7036.

(cherry picked from commit 85eb0ea)

@dadoonet dadoonet closed this in 85eb0ea Jul 31, 2014

@dadoonet dadoonet changed the title Add new `default` option for timestamp field Mapping: add new `default` option for timestamp field Aug 4, 2014

@dadoonet dadoonet deleted the dadoonet:issue/4718-timestamp-master branch Aug 12, 2014

dadoonet added a commit that referenced this pull request Sep 8, 2014

Generate timestamp when path is null
Index process fails when having `_timestamp` enabled and `path` option is set.
It fails with a `TimestampParsingException[failed to parse timestamp [null]]` message.

Reproduction:

```
DELETE test
PUT  test
{
    "mappings": {
        "test": {
            "_timestamp" : {
                "enabled" : "yes",
                "path" : "post_date"
            }
        }
    }
}
PUT test/test/1
{
  "foo": "bar"
}
```

You can define a default value for when timestamp is not provided
within the index request or in the `_source` document.

By default, the default value is `now` which means the date the document was processed by the indexing chain.

You can disable that default value by setting `default` to `null`. It means that `timestamp` is mandatory:

```
{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "default" : null
        }
    }
}
```

If you don't provide any timestamp value, indexation will fail.

You can also set the default value to any date respecting timestamp format:

```
{
    "tweet" : {
        "_timestamp" : {
            "enabled" : true,
            "format" : "YYYY-MM-dd",
            "default" : "1970-01-01"
        }
    }
}
```

If you don't provide any timestamp value, indexation will fail.

Closes #4718.
Closes #7036.

@clintongormley clintongormley changed the title Mapping: add new `default` option for timestamp field Mapping: Add new `default` option for timestamp field Sep 8, 2014

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Dec 23, 2014

New timestamp default option changed the default behavior for missing…
… paths

PR elastic#7036 changed the behavior for timestamp when provided as a parameter or within the document using `path` attribute.

This PR now considers that:

* when using timestamp as a parameter, we use a default value of `now`. Which means that if no timestamp is provided, the current time is used when the index operation is performed.
* when getting the timestamp from `path`, we use a default value of `null`. Which means that if no value is provided within the document, indexation will fail.

If users want to set the default value for `timestamp`, they can explicitly set one or set `"default":  "now"`.

Closes elastic#8882.

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Jan 26, 2015

Explicit _timestamp default null is set to now
When creating an index with:

```
PUT new_index
{
    "mappings": {
        "power": {
            "_timestamp" : {
                "enabled" : true,
                "default": null
            }
        }
    }
}
```

When restarting the cluster, `now` is applied instead of `null`. So index become:

```
{
    "mappings": {
        "power": {
            "_timestamp" : {
                "enabled" : true,
                "default": "now"
            }
        }
    }
}
```

This PR fix that and applies `null` when it was explicitly set.

Note that this won't happen anymore in 1.5 as `null` is not allowed anymore as a `default` value. See elastic#9104.

See also:

* elastic#7036
* elastic#9049
* elastic#9426#issuecomment-71534871

@clintongormley clintongormley changed the title Mapping: Add new `default` option for timestamp field Add new `default` option for timestamp field Jun 6, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Explicit _timestamp default null is set to now
When creating an index with:

```
PUT new_index
{
    "mappings": {
        "power": {
            "_timestamp" : {
                "enabled" : true,
                "default": null
            }
        }
    }
}
```

When restarting the cluster, `now` is applied instead of `null`. So index become:

```
{
    "mappings": {
        "power": {
            "_timestamp" : {
                "enabled" : true,
                "default": "now"
            }
        }
    }
}
```

This PR fix that and applies `null` when it was explicitly set.

Note that this won't happen anymore in 1.5 as `null` is not allowed anymore as a `default` value. See elastic#9104.

See also:

* elastic#7036
* elastic#9049
* elastic#9426#issuecomment-71534871
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.