fix(elasticsearch): time_zone setting does not work for cast datetime expressions #17048
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| cls, target_type: str, dttm: datetime, **kwargs: Any | ||
| ) -> Optional[str]: | ||
|
|
||
| if target_type.upper() != utils.TemporalType.DATETIME: |
There was a problem hiding this comment.
I think the previous logic of not short circuiting is clearer, i.e., if DATETIME then ….
|
|
||
| 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')""" |
There was a problem hiding this comment.
Why not use the f-string as previously?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
superset/db_engine_specs/base.py
Outdated
| @@ -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, | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@villebro I have updated it, you can review it when you have time
…GH16726-merge-test
| cls, target_type: str, dttm: datetime, db_extra: Optional[Dict[str, Any]] = None | ||
| ) -> Optional[str]: | ||
|
|
||
| db_extra = db_extra if db_extra else {} |
There was a problem hiding this comment.
nit: db_extra = db_extra or {} should do the job, too
| 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')""" | ||
| ) |
There was a problem hiding this comment.
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 objectJust 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:
...There was a problem hiding this comment.
@villebro I have updated it, you can review it when you have time
mistercrunch
added
🏷️ bot
SUMMARY
The elasticsearch
CASTfunction does not take effect for thetime zonesetting. In ES7.8 and above, we can use theDATETIME_PARSEfunction to solve this problem.In the current master code, the
extradict in the database contains theversionkey. We can use version to solve this problem. According to the instructions, version is set forprestoat the beginning, and ES should also reuse this field.TESTING INSTRUCTIONS
pip install elasticsearch-dbapi==0.2.6ADDITIONAL INFORMATION