-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix(elasticsearch): time_zone setting does not work for cast datetime expressions #17048
Changes from 3 commits
1c21505
a97af18
356dbe3
2e36740
71ed931
82ed2cd
e2d6109
a54147f
5e43a5c
d96aec6
cca6a6f
6441f4d
aa600a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,8 @@ | |
# specific language governing permissions and limitations | ||
# under the License. | ||
from datetime import datetime | ||
from typing import Dict, Optional, Type | ||
from distutils.version import StrictVersion | ||
from typing import Any, Dict, Optional, Type | ||
|
||
from superset.db_engine_specs.base import BaseEngineSpec | ||
from superset.db_engine_specs.exceptions import ( | ||
|
@@ -59,10 +60,20 @@ def get_dbapi_exception_mapping(cls) -> Dict[Type[Exception], Type[Exception]]: | |
} | ||
|
||
@classmethod | ||
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 | ||
def convert_dttm( | ||
cls, target_type: str, dttm: datetime, **kwargs: Any | ||
) -> Optional[str]: | ||
|
||
if target_type.upper() != utils.TemporalType.DATETIME: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the previous logic of not short circuiting is clearer, i.e., if |
||
return None | ||
|
||
es_version = kwargs.get("version") | ||
villebro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the f-string as previously? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
return f"""CAST('{dttm.isoformat(timespec="seconds")}' AS DATETIME)""" | ||
|
||
|
||
class OpenDistroEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method | ||
|
@@ -87,7 +98,9 @@ class OpenDistroEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method | |
engine_name = "ElasticSearch (OpenDistro SQL)" | ||
|
||
@classmethod | ||
def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]: | ||
def convert_dttm( | ||
cls, target_type: str, dttm: datetime, **kwargs: Any | ||
) -> Optional[str]: | ||
if target_type.upper() == utils.TemporalType.DATETIME: | ||
return f"""'{dttm.isoformat(timespec="seconds")}'""" | ||
return None | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is
**kwargs: Any
and notdb_extra: Optional[Dict[str, Any]]
like has been done inget_column_spec
? Unpacking into kwargs will make more difficult to add new parameters to this method going forwardThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 retainkwargs
, so that later if a data source needs non-db_extra information, this way is also compatible, what do you thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@villebro I have updated it, you can review it when you have time