fix(elasticsearch): time_zone setting does not work for cast datetime expressions #17048

aniaan · 2021-10-09T08:56:03Z

SUMMARY

The elasticsearch CAST function does not take effect for the time zone setting. In ES7.8 and above, we can use the DATETIME_PARSE function to solve this problem.

In the current master code, the extra dict in the database contains the version key. We can use version to solve this problem. According to the instructions, version is set for presto at the beginning, and ES should also reuse this field.

TESTING INSTRUCTIONS

Install the latest elasticsearch-dbapi version, 0.2.6, which supports setting the time_zone parameter pip install elasticsearch-dbapi==0.2.6
Follow the steps in Elasticsearch time_zone setting does not work for cast datetime expressions #16726 to create an ES data source, create a data source, set the time_zone parameter and version

Follow the operation steps in Elasticsearch time_zone setting does not work for cast datetime expressions #16726 to analyze, and you can see that the DATETIME_PARSE function and time_zone parameter play a correct role

ADDITIONAL INFORMATION

Has associated issue: Elasticsearch time_zone setting does not work for cast datetime expressions #16726
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2021-10-09T09:15:02Z

Codecov Report

Merging #17048 (aa600a6) into master (3ee9e11) will increase coverage by 0.08%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master   #17048      +/-   ##
==========================================
+ Coverage   76.86%   76.95%   +0.08%     
==========================================
  Files        1042     1042              
  Lines       56254    56271      +17     
  Branches     7785     7785              
==========================================
+ Hits        43242    43301      +59     
+ Misses      12754    12712      -42     
  Partials      258      258

Flag	Coverage Δ
hive	`81.54% <94.11%> (+0.01%)`	⬆️
mysql	`81.96% <94.11%> (+0.01%)`	⬆️
postgres	`81.97% <94.11%> (+0.01%)`	⬆️
presto	`81.83% <95.58%> (?)`
python	`82.47% <95.58%> (+0.15%)`	⬆️
sqlite	`81.65% <94.11%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/base.py	`88.54% <ø> (-0.05%)`	⬇️
superset/models/sql_types/base.py	`52.63% <50.00%> (ø)`
superset/db_engine_specs/databricks.py	`90.00% <66.66%> (ø)`
superset/db_engine_specs/mssql.py	`95.83% <66.66%> (ø)`
superset/connectors/sqla/models.py	`88.42% <100.00%> (+1.40%)`	⬆️
superset/connectors/sqla/utils.py	`92.15% <100.00%> (ø)`
superset/db_engine_specs/athena.py	`89.28% <100.00%> (ø)`
superset/db_engine_specs/bigquery.py	`84.84% <100.00%> (ø)`
superset/db_engine_specs/clickhouse.py	`66.03% <100.00%> (ø)`
superset/db_engine_specs/crate.py	`92.30% <100.00%> (ø)`
... and 22 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 3ee9e11...aa600a6. Read the comment docs.

john-bodley · 2021-10-11T14:20:20Z

superset/db_engine_specs/elasticsearch.py

+        cls, target_type: str, dttm: datetime, **kwargs: Any
+    ) -> Optional[str]:
+
+        if target_type.upper() != utils.TemporalType.DATETIME:


I think the previous logic of not short circuiting is clearer, i.e., if DATETIME then ….

john-bodley · 2021-10-11T14:21:21Z

superset/db_engine_specs/elasticsearch.py

+
+        if es_version and StrictVersion(es_version) >= StrictVersion("7.8"):
+            datetime_formatted = dttm.isoformat(sep=" ", timespec="seconds")
+            return f"""DATETIME_PARSE('{datetime_formatted}', 'yyyy-MM-dd HH:mm:ss')"""


Why not use the f-string as previously?

It was first written according to the way of not short-circuiting, so there will be nested if. If the f-string is written in one line, then the length of this line will be too long, so the expression is extracted. This way of writing refers to drill .py#convert_dttm method

superset/db_engine_specs/elasticsearch.py

villebro

Thanks for the contribution! First pass observation: one concern regarding the changed signature of convert_dttm, other than that the changes here make sense. Curious to hear your thoughts on the signature, happy to take a second pass + do some testing later.

villebro · 2021-11-11T08:26:20Z

superset/db_engine_specs/base.py

@@ -686,13 +685,14 @@ def df_to_sql(

    @classmethod
    def convert_dttm(  # pylint: disable=unused-argument
-        cls, target_type: str, dttm: datetime,
+        cls, target_type: str, dttm: datetime, **kwargs: Any,


Is there a reason this is **kwargs: Any and not db_extra: Optional[Dict[str, Any]] like has been done in get_column_spec? Unpacking into kwargs will make more difficult to add new parameters to this method going forward

Indeed, the writing here is not strict enough, for db_extra, it is better to show the declaration type, I think, the method signature can be like this, def convert_dttm(cls, target_type: str, dttm: datetime, db_extra: Optional[Dict[str, Any]], **kwargs: Any) also continue to retain kwargs, so that later if a data source needs non-db_extra information, this way is also compatible, what do you think

@aniaan I'd prefer not to add **kwargs unless it's currently needed (it's easy enough to add later when the need comes up). And thinking more closely, I'd personally prefer to keep all arguments named so that all parameters in the method are explicit.

Yes, we can, then we will not add it for now, and we will consider it later if we have this situation.
I'll correct the PR later

@villebro I have updated it, you can review it when you have time

…GH16726-merge

…GH16726-merge-test

villebro

Some additional comments, but in general LGTM

villebro · 2021-11-16T10:57:52Z

superset/db_engine_specs/elasticsearch.py

+        cls, target_type: str, dttm: datetime, db_extra: Optional[Dict[str, Any]] = None
+    ) -> Optional[str]:
+
+        db_extra = db_extra if db_extra else {}


nit: db_extra = db_extra or {} should do the job, too

villebro · 2021-11-16T11:06:04Z

superset/db_engine_specs/elasticsearch.py

+            if es_version and StrictVersion(es_version) >= StrictVersion("7.8"):
+                datetime_formatted = dttm.isoformat(sep=" ", timespec="seconds")
+                return (
+                    f"""DATETIME_PARSE('{datetime_formatted}', 'yyyy-MM-dd HH:mm:ss')"""
+                )


I wonder if we should have a fallback/more clear error message if the version isn't parseable by StrictVersion (Since this is to be populated by the user, we can expect to bump into unparseable values here):

>>> from distutils.version import StrictVersion >>> StrictVersion("7.8.0.0") Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/ville/.pyenv/versions/3.8-dev/lib/python3.8/distutils/version.py", line 40, in __init__ self.parse(vstring) File "/Users/ville/.pyenv/versions/3.8-dev/lib/python3.8/distutils/version.py", line 137, in parse raise ValueError("invalid version number '%s'" % vstring) ValueError: invalid version number '7.8.0.0'

Same for non-string values (I expect someone may try to enter it as a number, not string):

>>> StrictVersion(7.8) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/ville/.pyenv/versions/3.8-dev/lib/python3.8/distutils/version.py", line 40, in __init__ self.parse(vstring) File "/Users/ville/.pyenv/versions/3.8-dev/lib/python3.8/distutils/version.py", line 135, in parse match = self.version_re.match(vstring) TypeError: expected string or bytes-like object

Just in case, perhaps we could do something as simple as

supports_dttm_parse = False try: if es_version: supports_dttm_parse = StrictVersion(es_version) >= StrictVersion("7.8") except ValueError: ...

@villebro I have updated it, you can review it when you have time

villebro

LGTM, thanks for your patience with all the iterations!

aniaan added 3 commits October 9, 2021 16:02

fix(elasticsearch): cast does not take effect for time zone settings

1c21505

test(elasticsearch): add test

a97af18

fix(test): fix typo

356dbe3

pull-request-size bot added the size/L label Oct 9, 2021

john-bodley reviewed Oct 11, 2021

View reviewed changes

aniaan added 3 commits October 11, 2021 23:44

docs(elasticsearch): add annotation

2e36740

docs(elasticsearch): add time_zone desc

71ed931

docs(elasticsearch): fix typo

82ed2cd

aniaan requested a review from john-bodley October 11, 2021 15:54

villebro reviewed Nov 11, 2021

View reviewed changes

aniaan added 3 commits November 11, 2021 17:37

Merge branch 'master' of https://github.com/apache/superset into fix/…

e2d6109

…GH16726-merge

refactor(db_engine): change convert_dttm signature

a54147f

fix(test): fix test

5e43a5c

aniaan requested a review from villebro November 11, 2021 11:39

Merge branch 'master' of https://github.com/apache/superset into fix/…

d96aec6

…GH16726-merge-test

villebro reviewed Nov 16, 2021

View reviewed changes

aniaan added 3 commits November 16, 2021 21:38

fix(es): add try catch

cca6a6f

fix(test): fix caplog

6441f4d

fix(test): fix typo

aa600a6

aniaan requested a review from villebro November 16, 2021 15:01

villebro approved these changes Nov 25, 2021

View reviewed changes

villebro merged commit 5a1c681 into apache:master Nov 25, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(elasticsearch): time_zone setting does not work for cast datetime expressions #17048

fix(elasticsearch): time_zone setting does not work for cast datetime expressions #17048

aniaan commented Oct 9, 2021

codecov bot commented Oct 9, 2021 •

edited

Loading

john-bodley Oct 11, 2021

john-bodley Oct 11, 2021

aniaan Oct 11, 2021

villebro left a comment

villebro Nov 11, 2021

aniaan Nov 11, 2021

villebro Nov 11, 2021

aniaan Nov 11, 2021

aniaan Nov 16, 2021

villebro left a comment

villebro Nov 16, 2021

villebro Nov 16, 2021

aniaan Nov 16, 2021 •

edited

Loading

villebro left a comment

fix(elasticsearch): time_zone setting does not work for cast datetime expressions #17048

fix(elasticsearch): time_zone setting does not work for cast datetime expressions #17048

Conversation

aniaan commented Oct 9, 2021

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 9, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a 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

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aniaan Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 9, 2021 •

edited

Loading

aniaan Nov 16, 2021 •

edited

Loading