You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/12/09 21:06:30 UTC
[GitHub] [superset] john-bodley commented on a change in pull request #17543: feat: new dataset/table/column models
john-bodley commented on a change in pull request #17543:
URL: https://github.com/apache/superset/pull/17543#discussion_r766145860
##########
File path: superset/columns/models.py
##########
@@ -0,0 +1,95 @@
+# 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.
+"""
+Column model.
+
+This model was introduced in SIP-68 (https://github.com/apache/superset/issues/14909),
+and represents a "column" in a table or dataset. In addition to a column, new models for
+tables, metrics, and datasets were also introduced.
+
+These models are not fully implemented, and shouldn't be used yet.
+"""
+
+import sqlalchemy as sa
+from flask_appbuilder import Model
+
+from superset.models.helpers import (
+ AuditMixinNullable,
+ ExtraJSONMixin,
+ ImportExportMixin,
+)
+
+
+class Column(
+ Model, AuditMixinNullable, ExtraJSONMixin, ImportExportMixin,
+):
+ """
+ A "column".
+
+ The definition of column here is overloaded: it can represent a physical column in a
+ database relation (table or view); a computed/derived column on a dataset; or an
+ aggregation expression representing a metric.
+ """
+
+ __tablename__ = "sl_columns"
Review comment:
Out of interest what does the `sl` prefix represent? Additionally is it necessary?
##########
File path: superset/columns/models.py
##########
@@ -0,0 +1,95 @@
+# 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.
+"""
+Column model.
+
+This model was introduced in SIP-68 (https://github.com/apache/superset/issues/14909),
+and represents a "column" in a table or dataset. In addition to a column, new models for
+tables, metrics, and datasets were also introduced.
+
+These models are not fully implemented, and shouldn't be used yet.
+"""
+
+import sqlalchemy as sa
+from flask_appbuilder import Model
+
+from superset.models.helpers import (
+ AuditMixinNullable,
+ ExtraJSONMixin,
+ ImportExportMixin,
+)
+
+
+class Column(
+ Model, AuditMixinNullable, ExtraJSONMixin, ImportExportMixin,
+):
+ """
+ A "column".
+
+ The definition of column here is overloaded: it can represent a physical column in a
+ database relation (table or view); a computed/derived column on a dataset; or an
+ aggregation expression representing a metric.
+ """
+
+ __tablename__ = "sl_columns"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+
+ # We use ``sa.Text`` for these attributes because (1) in modern databases the
+ # performance is the same as ``VARCHAR``[1] and (2) because some table names can be
+ # **really** long (eg, Google Sheets URLs).
+ #
+ # [1] https://www.postgresql.org/docs/9.1/datatype-character.html
+ name = sa.Column(sa.Text)
+ type = sa.Column(sa.Text)
+
+ # Columns are defined by expressions. For tables, these are the actual columns names,
+ # and should match the ``name`` attribute. For datasets, these can be any valid SQL
+ # expression. If the SQL expression is an aggregation the column is a metric,
+ # otherwise it's a computed column.
+ expression = sa.Column(sa.Text)
+
+ # Does the expression point directly to a physical column?
+ is_physical = sa.Column(sa.Boolean, default=True)
+
+ # Additional metadata describing the column.
+ description = sa.Column(sa.Text)
+ warning_text = sa.Column(sa.Text)
+ units = sa.Column(sa.Text)
+
+ # Is this a time column? Useful for plotting time series.
+ is_temporal = sa.Column(sa.Boolean, default=False)
+
+ # Is this a spatial column? This could be leveraged in the future for spatial
+ # visualizations.
+ is_spatial = sa.Column(sa.Boolean, default=False)
+
+ # Is this column a partition? Useful for scheduling queries and previewing the latest
+ # data.
+ is_partition = sa.Column(sa.Boolean, default=False)
+
+ # Is this column an aggregation (metric)?
+ is_aggregation = sa.Column(sa.Boolean, default=False)
+
+ # Assuming the column is an aggregation, is it additive? Useful for determining which
+ # aggregations can be done on the metric. Eg, ``COUNT(DISTINCT user_id)`` is not
+ # additive, so it shouldn't be used in a ``SUM``.
+ is_additive = sa.Column(sa.Boolean, default=False)
+
+ # Is an increase good? Useful for displaying the results of A/B tests, or setting up
+ # alerts. Eg, this is true for "revenue", but false for "latency".
+ increase_good = sa.Column(sa.Boolean, default=True)
Review comment:
Maybe "desired" as opposed to "good"? Also for boolean columns I find the `is_` or `has_` prefix is helpful so `is_increased_desired`.
##########
File path: superset/tables/models.py
##########
@@ -0,0 +1,88 @@
+# 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.
+"""
+Table model.
+
+This model was introduced in SIP-68 (https://github.com/apache/superset/issues/14909),
+and represents a "table" in a given database -- either a physical table or a view. In
+addition to a table, new models for columns, metrics, and datasets were also introduced.
+
+These models are not fully implemented, and shouldn't be used yet.
+"""
+
+from typing import List
+
+import sqlalchemy as sa
+from flask_appbuilder import Model
+from sqlalchemy.orm import backref, relationship
+from sqlalchemy.schema import UniqueConstraint
+
+from superset.columns.models import Column
+from superset.models.core import Database
+from superset.models.helpers import (
+ AuditMixinNullable,
+ ExtraJSONMixin,
+ ImportExportMixin,
+)
+
+association_table = sa.Table(
+ "sl_table_columns",
+ Model.metadata, # pylint: disable=no-member
+ sa.Column("table_id", sa.ForeignKey("sl_tables.id")),
+ sa.Column("column_id", sa.ForeignKey("sl_columns.id")),
+)
+
+
+class Table(Model, AuditMixinNullable, ExtraJSONMixin, ImportExportMixin):
+ """
+ A table/view in a database.
+ """
+
+ __tablename__ = "sl_tables"
+
+ # Note this uniqueness constraint is not part of the physical schema, i.e., it does
Review comment:
Thanks for including this comment. I know it's something which has tripped us up in the past.
##########
File path: tests/unit_tests/tables/test_models.py
##########
@@ -0,0 +1,53 @@
+# 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.
+
+# pylint: disable=import-outside-toplevel, unused-argument
+
+from sqlalchemy.orm.session import Session
+
+
+def test_table_model(app_context: None, session: Session) -> None:
+ """
+ Test basic attributes of a ``Table``.
+ """
+ from superset.columns.models import Column
+ from superset.models.core import Database
+ from superset.tables.models import Table
+
+ engine = session.get_bind()
+ Table.metadata.create_all(engine) # pylint: disable=no-member
+
+ table = Table(
+ name="my_table",
+ schema="my_schema",
+ catalog="my_catalog",
+ database=Database(database_name="my_database", sqlalchemy_uri="test://"),
+ columns=[Column(name="ds", type="TIMESTAMP", expression="ds",)],
+ )
+ session.add(table)
+ session.flush()
+
+ assert table.id == 1
+ assert table.uuid is not None
+
Review comment:
```suggestion
```
##########
File path: tests/unit_tests/tables/test_models.py
##########
@@ -0,0 +1,53 @@
+# 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.
+
+# pylint: disable=import-outside-toplevel, unused-argument
+
+from sqlalchemy.orm.session import Session
+
+
+def test_table_model(app_context: None, session: Session) -> None:
+ """
+ Test basic attributes of a ``Table``.
+ """
+ from superset.columns.models import Column
+ from superset.models.core import Database
+ from superset.tables.models import Table
+
+ engine = session.get_bind()
+ Table.metadata.create_all(engine) # pylint: disable=no-member
+
+ table = Table(
+ name="my_table",
+ schema="my_schema",
+ catalog="my_catalog",
+ database=Database(database_name="my_database", sqlalchemy_uri="test://"),
+ columns=[Column(name="ds", type="TIMESTAMP", expression="ds",)],
+ )
+ session.add(table)
+ session.flush()
+
+ assert table.id == 1
+ assert table.uuid is not None
+
+ assert table.database_id == 1
+
Review comment:
```suggestion
```
##########
File path: superset/connectors/sqla/models.py
##########
@@ -1790,23 +1806,448 @@ def before_update(
raise Exception(get_dataset_exist_error_msg(target.full_name))
@staticmethod
- def update_table(
- _mapper: Mapper, _connection: Connection, obj: Union[SqlMetric, TableColumn]
+ def update_table( # pylint: disable=unused-argument
+ mapper: Mapper, connection: Connection, target: Union[SqlMetric, TableColumn]
) -> None:
"""
Forces an update to the table's changed_on value when a metric or column on the
table is updated. This busts the cache key for all charts that use the table.
- :param _mapper: Unused.
- :param _connection: Unused.
- :param obj: The metric or column that was updated.
+ :param mapper: Unused.
+ :param connection: Unused.
+ :param target: The metric or column that was updated.
+ """
+ inspector = inspect(target)
+ session = inspector.session
+
+ # get DB-specific conditional quoter for expressions that point to columns or
+ # table names
+ database = (
+ target.table.database
+ or session.query(Database).filter_by(id=target.database_id).one()
+ )
+ engine = database.get_sqla_engine(schema=target.table.schema)
+ conditional_quote = engine.dialect.identifier_preparer.quote
+
+ session.execute(update(SqlaTable).where(SqlaTable.id == target.table.id))
+
+ # update ``Column`` model as well
+ dataset = (
+ session.query(NewDataset).filter_by(sqlatable_id=target.table.id).one()
+ )
+
+ if isinstance(target, TableColumn):
+ columns = [
+ column
+ for column in dataset.columns
+ if column.name == target.column_name
+ ]
+ if not columns:
+ return
+
+ column = columns[0]
+ extra_json = json.loads(target.extra or "{}")
+ for attr in {"groupby", "filterable", "verbose_name", "python_date_format"}:
+ value = getattr(target, attr)
+ if value:
+ extra_json[attr] = value
+
+ column.name = target.column_name
+ column.type = target.type or "Unknown"
+ column.expression = target.expression or conditional_quote(
+ target.column_name
+ )
+ column.description = target.description
+ column.is_temporal = target.is_dttm
+ column.is_physical = target.expression is None
+ column.extra_json = json.dumps(extra_json) if extra_json else None
+
+ else: # SqlMetric
+ columns = [
+ column
+ for column in dataset.columns
+ if column.name == target.metric_name
+ ]
+ if not columns:
+ return
+
+ column = columns[0]
+ extra_json = json.loads(target.extra or "{}")
+ for attr in {"verbose_name", "metric_type", "d3format"}:
+ value = getattr(target, attr)
+ if value:
+ extra_json[attr] = value
+
+ is_additive = (
+ target.metric_type
+ and target.metric_type.lower() in ADDITIVE_METRIC_TYPES
+ )
+
+ column.name = target.metric_name
+ column.expression = target.expression
+ column.warning_text = target.warning_text
+ column.description = target.description
+ column.is_additive = is_additive
+ column.extra_json = json.dumps(extra_json) if extra_json else None
+
+ @staticmethod
+ def after_insert( # pylint: disable=too-many-locals
+ mapper: Mapper, connection: Connection, target: "SqlaTable",
+ ) -> None:
+ """
+ Shadow write the dataset to new models.
Review comment:
I think the dual write approach is a good call.
##########
File path: tests/unit_tests/tables/test_models.py
##########
@@ -0,0 +1,53 @@
+# 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.
+
+# pylint: disable=import-outside-toplevel, unused-argument
+
+from sqlalchemy.orm.session import Session
+
+
+def test_table_model(app_context: None, session: Session) -> None:
+ """
+ Test basic attributes of a ``Table``.
+ """
+ from superset.columns.models import Column
+ from superset.models.core import Database
+ from superset.tables.models import Table
+
+ engine = session.get_bind()
+ Table.metadata.create_all(engine) # pylint: disable=no-member
+
+ table = Table(
+ name="my_table",
+ schema="my_schema",
+ catalog="my_catalog",
+ database=Database(database_name="my_database", sqlalchemy_uri="test://"),
+ columns=[Column(name="ds", type="TIMESTAMP", expression="ds",)],
+ )
+ session.add(table)
+ session.flush()
+
+ assert table.id == 1
+ assert table.uuid is not None
+
+ assert table.database_id == 1
+
+ assert table.catalog == "my_catalog"
+ assert table.schema == "my_schema"
+ assert table.name == "my_table"
+
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org