-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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(redshift): convert_dttm method for redshift dataset and tests #26283
Changes from 1 commit
23590b5
8c7ff26
e1de101
a52de05
65dc201
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 |
---|---|---|
|
@@ -14,8 +14,11 @@ | |
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
from __future__ import annotations | ||
|
||
import logging | ||
import re | ||
from datetime import datetime | ||
from re import Pattern | ||
from typing import Any, Optional | ||
|
||
|
@@ -29,6 +32,7 @@ | |
from superset.models.core import Database | ||
from superset.models.sql_lab import Query | ||
from superset.sql_parse import Table | ||
from sqlalchemy.types import Date, DateTime | ||
|
||
logger = logging.getLogger() | ||
|
||
|
@@ -147,6 +151,19 @@ def _mutate_label(label: str) -> str: | |
""" | ||
return label.lower() | ||
|
||
@classmethod | ||
def convert_dttm( | ||
cls, target_type: str, dttm: datetime, db_extra: dict[str, Any] | None = None | ||
) -> str | None: | ||
sqla_type = cls.get_sqla_column_type(target_type) | ||
|
||
if isinstance(sqla_type, Date): | ||
return f"TO_DATE('{dttm.date().isoformat()}', 'YYYY-MM-DD')" | ||
if isinstance(sqla_type, DateTime): | ||
dttm_formatted = dttm.isoformat(sep=" ", timespec="microseconds") | ||
return f"""TO_TIMESTAMP('{dttm_formatted}', 'YYYY-MM-DD HH24:MI:SS.US')""" | ||
return None | ||
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 hit me, we should consider moving this method from 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. +1 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. yes sure, doing that 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. done @john-bodley @villebro , redshift test is also running, i have ran 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. @john-bodley i think ci is not started yet, can you help |
||
|
||
@classmethod | ||
def get_cancel_query_id(cls, cursor: Any, query: Query) -> Optional[str]: | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
from datetime import datetime | ||
from typing import Any, Optional | ||
|
||
import pytest | ||
|
||
from tests.unit_tests.db_engine_specs.utils import assert_convert_dttm | ||
from tests.unit_tests.fixtures.common import dttm | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"target_type,expected_result", | ||
[ | ||
("Date", "TO_DATE('2019-01-02', 'YYYY-MM-DD')"), | ||
( | ||
"DateTime", | ||
"TO_TIMESTAMP('2019-01-02 03:04:05.678900', 'YYYY-MM-DD HH24:MI:SS.US')", | ||
), | ||
( | ||
"TimeStamp", | ||
"TO_TIMESTAMP('2019-01-02 03:04:05.678900', 'YYYY-MM-DD HH24:MI:SS.US')", | ||
), | ||
("UnknownType", None), | ||
], | ||
) | ||
def test_convert_dttm( | ||
target_type: str, expected_result: Optional[str], dttm: datetime | ||
) -> None: | ||
from superset.db_engine_specs.redshift import RedshiftEngineSpec as spec | ||
|
||
assert_convert_dttm(spec, target_type, expected_result, dttm) | ||
|
||
|
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.
This seems to be identical with the
PostgresEngineSpec.convert_dttm()
method which it inherits and thus this isn't needed.Keeping the unit tests makes sense.
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.
but same postgres method is not getting called, when i use it?
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.
that's why if i remove method, test is getting failed as well
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.
i understand it inherits postgres, but somehow mixin is not working @john-bodley
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.
@gaurav7261 you're correct - @john-bodley my bad,
convert_dttm
is actually defined inPostgresEngineSpec
, notPostgresBaseEngineSpec
like I said a moment ago. So this change LGTM.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.
@john-bodley @villebro can you help why precommit failed,what i need to do
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.
Did you check the details of the CI workflow? https://github.com/apache/superset/actions/runs/7226045710/job/19691266350?pr=26283
It specifies which changes are needed. Tip: you can configure
pre-commit
to do this automatically for you. Check here for details: https://github.com/apache/superset/blob/master/CONTRIBUTING.md#git-hooksThere 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.
thanks, got it, actually i was confused when it highlight the line on another function
get_cancel_query_id
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.
@john-bodley please approve the run