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.

Sign up for GitHub

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: create virtual table with exotic type #19714

Merged
merged 2 commits into from
Apr 14, 2022
Merged

fix: create virtual table with exotic type #19714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
918d35d
fix: create virtual table with exotic type
villebro Apr 14, 2022
3c99eba
fix test
villebro Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions superset/connectors/sqla/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2374,11 +2374,10 @@ def write_shadow_dataset( # pylint: disable=too-many-locals
referenced_tables = parsed.tables
tables = load_or_create_tables(
session,
dataset.database_id,
database,
dataset.schema,
referenced_tables,
conditional_quote,
engine,
)

# create the new dataset
Expand Down
36 changes: 12 additions & 24 deletions superset/connectors/sqla/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@
# specific language governing permissions and limitations
# under the License.
from contextlib import closing
from datetime import date, datetime, time, timedelta
from typing import Callable, Dict, List, Optional, Set, TYPE_CHECKING
from typing import Any, Callable, Dict, List, Optional, Set, TYPE_CHECKING

import sqlparse
from flask_babel import lazy_gettext as _
from sqlalchemy import and_, inspect, or_
from sqlalchemy.engine import Engine
from sqlalchemy import and_, or_
from sqlalchemy.exc import NoSuchTableError
from sqlalchemy.orm import Session
from sqlalchemy.sql.type_api import TypeEngine
Expand All @@ -42,14 +40,11 @@
from superset.connectors.sqla.models import SqlaTable


TEMPORAL_TYPES = {date, datetime, time, timedelta}


def get_physical_table_metadata(
database: Database,
table_name: str,
schema_name: Optional[str] = None,
) -> List[Dict[str, str]]:
) -> List[Dict[str, Any]]:
"""Use SQLAlchemy inspector to get table metadata"""
db_engine_spec = database.db_engine_spec
db_dialect = database.get_dialect()
Expand Down Expand Up @@ -86,7 +81,7 @@ def get_physical_table_metadata(
col.update(
{
"type": "UNKNOWN",
"generic_type": None,
"type_generic": None,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a bycatch (see line 79/76 above with the correct key) when I worked on adding proper TypedDicts for these objects (opening up separate PR for that one, but I thought it worthwhile to fix this error right away).

"is_dttm": None,
}
)
Expand Down Expand Up @@ -173,20 +168,12 @@ def validate_adhoc_subquery(
return ";\n".join(str(statement) for statement in statements)


def is_column_type_temporal(column_type: TypeEngine) -> bool:
try:
return column_type.python_type in TEMPORAL_TYPES
except NotImplementedError:
return False


def load_or_create_tables( # pylint: disable=too-many-arguments
session: Session,
database_id: int,
database: Database,
default_schema: Optional[str],
tables: Set[Table],
conditional_quote: Callable[[str], str],
engine: Engine,
) -> List[NewTable]:
"""
Load or create new table model instances.
Expand All @@ -206,7 +193,7 @@ def load_or_create_tables( # pylint: disable=too-many-arguments
predicate = or_(
*[
and_(
NewTable.database_id == database_id,
NewTable.database_id == database.id,
NewTable.schema == table.schema,
NewTable.name == table.table,
)
Expand All @@ -220,9 +207,10 @@ def load_or_create_tables( # pylint: disable=too-many-arguments
for table in tables:
if (table.schema, table.table) not in existing:
try:
inspector = inspect(engine)
column_metadata = inspector.get_columns(
table.table, schema=table.schema
column_metadata = get_physical_table_metadata(
database=database,
table_name=table.table,
schema_name=table.schema,
)
except Exception: # pylint: disable=broad-except
continue
Expand All @@ -231,7 +219,7 @@ def load_or_create_tables( # pylint: disable=too-many-arguments
name=column["name"],
type=str(column["type"]),
expression=conditional_quote(column["name"]),
is_temporal=is_column_type_temporal(column["type"]),
is_temporal=column["is_dttm"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous version checked for date, datetime, time and timedelta. Is this equivalent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - this one is actually based on what the db engine spec says is temporal, so it's much more comprehensive (=can be customized per database).

is_aggregation=False,
is_physical=True,
is_spatial=False,
Expand All @@ -245,7 +233,7 @@ def load_or_create_tables( # pylint: disable=too-many-arguments
name=table.table,
schema=table.schema,
catalog=None,
database_id=database_id,
database_id=database.id,
columns=columns,
)
)
Expand Down