[GitHub] [superset] ktmud commented on a change in pull request #19234: fix: adhoc column in legacy chart

[GitBox <gi...@apache.org>]

ktmud commented on a change in pull request #19234:
URL: https://github.com/apache/superset/pull/19234#discussion_r829485014



##########
File path: tests/integration_tests/model_tests.py
##########
@@ -578,6 +580,25 @@ def test_data_for_slices_with_query_context(self):
             "state",
         }
 
+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+    def test_data_for_slices_with_adhoc_column(self):
+        # should perform sqla.model.BaseDatasource.data_for_slices() with adhoc
+        # column and legacy chart
+        tbl = self.get_table(name="birth_names")
+        slc = (
+            metadata_db.session.query(Slice)
+            .filter_by(
+                datasource_id=tbl.id, datasource_type=tbl.type, slice_name="Boys",
+            )
+            .first()
+        )

Review comment:
       Can we create a new slice instead to reduce dependencies on other tests/data?

##########
File path: superset/connectors/base/models.py
##########
@@ -339,11 +339,14 @@ def data_for_slices(  # pylint: disable=too-many-locals
                     or []
                 )
             else:
-                column_names.update(
-                    column
+                _columns = [

Review comment:
       Is this extra variable really needed?




-- 
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