Added support for 2 date formats:

[viglia]

This PR addresses #22 by adding support for 2 date formats:

1. epoch_millis
2. epoch_seconds

When the mapping are fetched each date type will carry additional format information.
Ex. date[epoch_millis], date[epoch_second], etc.

This will later on be converted to pandas types according to their format: datetime64[ms], datetime64[s]

[stevedodson]

A large number of existing tests fail with this PR - please always make sure all tests pass before submitting (via ci or manually).

No tests are supplied with this PR - please add tests.

[viglia]

ok. checking it, thanks!

[stevedodson]

As described in previous comment, we should try to write tests first and then develop code against these tests.

Also, can we not map the es_dtype/format to the appropriate pd.to_datetime arguments in mappings, so we can avoid this code in flatten.

[stevedodson]

Another thing we may want to consider is if we should parse doc_values for datetime rather than _source. For example, how would we resolve this:

PUT my_index
{
  "mappings": {
    "properties": {
      "date": {
        "type": "date" 
      }
    }
  }
}

PUT my_index/_doc/1
{ "date": "2015-01-01" } 

PUT my_index/_doc/2
{ "date": "2015-01-01T12:10:30Z" } 

PUT my_index/_doc/3
{ "date": 1420070400001 } 

[viglia]

@stevedodson another question I have is: I noticed that by default date is mapped to datetime64[ns] is there a specific reason for mapping to that specific dtype? Should we keep it that way?

[stevedodson]

@viglia - for now I'd look to implement the simplest way to supported epoch_millis as a format + test against other common formats (e.g. the ones that contains date and time).

Having the default map to datetime64[ns] is ok.

[stevedodson]

FYI - It is good to test multiple time formats in a single index alongside a single time format.

• Happy for you to write all the tests if you want, but I'd be satisfied for now if the test above passes when a eland.DataFrame is constructed from the index and all the results are consistent with the indexed values. We can then move on to other issues.

[stevedodson]

@blaklaybul don't worry about review at this stage - I clicked incorrect reviewer in UI.

[stevedodson]

From slack:

The implementation of this should be very simple. In elasticsearch_date_to_pandas_datetime simply add the correct conversions for all date formats. E.g.

    if date_format == "epoch_millis":
        return pd.to_datetime(x, unit='ms')
    elif date_format == "epoch_second":
        return pd.to_datetime(x, unit='s')
    elif date_format == "basic_ordinal_date":
        return pd.to_datetime(x, format='%Y%j')
    ...

etc.
In general, vanilla pd.to_datetime will work (and we should use it where it does as it will probably be more efficient). The cases it doesn’t work use an explicit format.

[stevedodson]

@viglia + please merge with master. Thanks!

[stevedodson]

Francesco Vigliaturo 8:58 AM
@steve.dodson that is actually the “hack” I was thinking of 😅
I think it’s hacky because the actual dtypes differ between the eland dtype (datetime64[ns]) and the pandas one (depending on the date object/datetime64[ns, UTC]). There is a mismatch.

Steve Dodson 9:02 AM
There is no one-to-one mapping between pandas datetime dtypes and Elasticsearch dates. For instance, basic_ordinal_date doesn’t map to a pandas datetime. So whatever you do there is require translation. Assuming all eland dtypes map to datetime64[ns] or datetime64[s] is not a hacky as we have to make an informed decision about datetime conversion. (edited)
Adding this as a comment would be useful to justify the decision.

[stevedodson]

@viglia - please add support for all non 'strict_' formats.

[viglia]

Hi @stevedodson

I've addressed all the feedback left. Let me know if I missed anything.

Regarding this one:

Also, do we need to add a new member variable for date_fields_format that is separate from _mappings_capabilities

I did not merged them and I've kept date_fields_formatseparate as it seems that that info is very specific to only date field so I wasn't sure it made sense having in the matrix along with searchable, aggregatable, etc. this info

Ps. I've also run all the tests and everything run fine. I'm going to check what Jenkins complains about

[stevedodson]

Please merge into master!

[stevedodson]

@viglia - next time please wait until the PR is approved before merging.

[viglia]

@stevedodson sorry for the misunderstanding!

I've read your comment above "Please merge into master!" and I thought it was a green light to rebase on master and merge