Re: [PR] fix(sqla): pass catalog and schema to get_sqla_engine in values_for_column [superset]

["rusackas (via GitHub)" <gi...@apache.org>]

rusackas commented on code in PR #38681:
URL: https://github.com/apache/superset/pull/38681#discussion_r3047606854


##########
superset/models/helpers.py:
##########
@@ -2456,7 +2456,9 @@ def values_for_column(  # pylint: disable=too-many-locals
         if rls_filters:
             qry = qry.where(and_(*rls_filters))
 
-        with self.database.get_sqla_engine() as engine:
+        with self.database.get_sqla_engine(
+            catalog=self.catalog, schema=self.schema
+        ) as engine:
             sql = str(qry.compile(engine, compile_kwargs={"literal_binds": True}))
             sql = self._apply_cte(sql, cte)
 

Review Comment:
   The current formatting is actually accepted by 1855 files left unchanged (the formatter used in this repo's pre-commit hooks) — I verified that running `ruff format --check superset/models/helpers.py` reports "1 file already formatted". The two-arg form on one line fits within the 88-character line-length limit, so ruff format doesn't split it further. No formatting change is needed here.



##########
superset/models/helpers.py:
##########
@@ -2456,7 +2456,9 @@ def values_for_column(  # pylint: disable=too-many-locals
         if rls_filters:
             qry = qry.where(and_(*rls_filters))
 
-        with self.database.get_sqla_engine() as engine:
+        with self.database.get_sqla_engine(
+            catalog=self.catalog, schema=self.schema
+        ) as engine:

Review Comment:
   Added `test_values_for_column_passes_catalog_and_schema` in `tests/unit_tests/models/helpers_test.py` (commit 82087d2). The test wraps the existing fixture mock with a recording wrapper to capture the `catalog` and `schema` kwargs passed to `get_sqla_engine`, and asserts both the explicit-schema case (`catalog='my_catalog', schema='my_schema'`) and the `None`/no-schema default case. All six `values_for_column` tests pass.



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