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

Elasticsearch time_zone setting does not work for cast datetime expressions #16726

Closed
aniaan opened this issue Sep 17, 2021 · 2 comments
Closed
Assignees
Labels
#bug Bug report need:followup Requires followup

Comments

@aniaan
Copy link
Contributor

aniaan commented Sep 17, 2021

My time zone is CST, not UTC. Based on some best practices of ES, when we store data, we convert CST time to UTC time for storage, and the corresponding conversion is done by time zone when querying.
When I was using superset, I found two problems

  1. elasticsearch-dbapi does not support passing the time_zone parameter, I tried to fix it and submitted a PR feat(query): add time_zone param preset-io/elasticsearch-dbapi#69, currently waiting for a review, but I can fix it temporarily by pip install <my_github_repo>. It's equivalent to hitting a patch to circumvent the issue.
  2. Once the problem was solved, I found another problem, the time_zone I set didn't work for filtering datetime type fields, but it worked for HISTOGRAM("@timestamp", INTERVAL 1 DAY), I found the reason by checking the ES-SQL documentation, https://www.elastic.co/guide/en/elasticsearch/reference/current/sql-search-api.html#sql-search-api-request-body

(Optional, string) ISO-8601 time zone ID for the search. Several SQL date/time functions use this time zone. Defaults to Z (UTC).

Several SQL date/time functions use this time zone very important!

superset on ES datatime field query, is the use of CAST ('date' AS DATETIME),

def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
if target_type.upper() == utils.TemporalType.DATETIME:
return f"""CAST('{dttm.isoformat(timespec="seconds")}' AS DATETIME)"""
return None

time_zone on CAST this way of conversion does not work, only date related functions work, to solve this problem, should use DATETIME_PARSE ('date', '{pattern}') to deal with, time_zone on this function works.

Here are some examples of my steps to demonstrate this in kibana

PUT /library/_bulk?refresh
{"index":{"_id": "Leviathan Wakes"}}
{"name": "Leviathan Wakes", "author": "James S.A. Corey", "release_date": "2021-09-17T13:24:19Z", "page_count": 561}
{"index":{"_id": "Hyperion"}}
{"name": "Hyperion", "author": "Dan Simmons", "release_date": "2021-09-17T19:24:19Z", "page_count": 482}
{"index":{"_id": "Dune"}}
{"name": "Dune", "author": "Frank Herbert", "release_date": "2021-09-18T09:24:19Z", "page_count": 604}

# UTC -> CST
# 2021-09-17T13:24:19Z => 2021-09-17T21:24:19+08:00
# 2021-09-17T19:24:19Z => 2021-09-18T03:24:19+08:00
# 2021-09-18T09:24:19Z => 2021-09-18T17:24:19+08:00


# should be return 2 ,but 1, **because time_zone does not work for CAST**
GET /_sql?format=txt
{
  "query": """
  SELECT * FROM "library" where release_date > CAST('2021-09-17T21:24:19' AS DATETIME)
  """,
  "time_zone": "Asia/Shanghai"
}

    author     |     name      |  page_count   |        release_date         
---------------+---------------+---------------+-----------------------------
Frank Herbert  |Dune           |604            |2021-09-18T17:24:19.000+08:00


## return 2, work!!

GET /_sql?format=txt
{
  "query": """
  SELECT * FROM "library" where release_date > DATETIME_PARSE('2021-09-17 21:24:19', 'yyyy-MM-dd HH:mm:ss')
  """,
  "time_zone": "Asia/Shanghai"
}

    author     |     name      |  page_count   |        release_date         
---------------+---------------+---------------+-----------------------------
Dan Simmons    |Hyperion       |482            |2021-09-18T03:24:19.000+08:00
Frank Herbert  |Dune           |604            |2021-09-18T17:24:19.000+08:00

The above example should have illustrated the problem, the essence is that CAST this way for the time zone settings do not take effect, this is important for non-UTC users, I think we need to solve it, at present I can think of is

  1. change to DATETIME_PARSE, but there is no such function before ES 7.8.
  2. superset database add global time_zone parameter, the datetime object for filtering should carry the corresponding time zone information, and the format should also carry the time zone information. This way I found by looking at the code that the scope is too wide and will affect other data sources.

If we have to be compatible with users before ES7.8, a compromise solution is to create a separate db_engine_spec for ES7.8 and above, which can be implemented in elasticsearch-dbapi as a separate sqlalchemy ESDialect, the name of which may require some careful thought.

new db_engine_spec

@classmethod
    def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
        if target_type.upper() == utils.TemporalType.DATETIME:
            return f"""DATETIME_PARSE('2019-10-13 00:08:00', 'yyyy-MM-dd HH:mm:ss')"""
        return None

This is the solution I have thought of so far, what do you think?

cc @dpgaspar

@aniaan aniaan added the #bug Bug report label Sep 17, 2021
@junlincc
Copy link
Member

thank you for the contribution! @aniaan
I'm not sure if you opened the PR in the correct repo? @dpgaspar can you confirm?

@dpgaspar
Copy link
Member

I can, looking good, thank you @aniaan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#bug Bug report need:followup Requires followup
Projects
None yet
Development

No branches or pull requests

3 participants