eschutho commented on code in PR #35540:
URL: https://github.com/apache/superset/pull/35540#discussion_r2755684786
##########
superset/models/core.py:
##########
@@ -833,17 +833,17 @@ def select_star( # pylint: disable=too-many-arguments
cols: list[ResultSetColumnType] | None = None,
) -> str:
"""Generates a ``select *`` statement in the proper dialect"""
- with self.get_sqla_engine(catalog=table.catalog, schema=table.schema)
as engine:
- return self.db_engine_spec.select_star(
- self,
- table,
- engine=engine,
- limit=limit,
- show_cols=show_cols,
- indent=indent,
- latest_partition=latest_partition,
- cols=cols,
- )
+ dialect = self.get_dialect()
+ return self.db_engine_spec.select_star(
+ self,
+ table,
+ dialect=dialect,
+ limit=limit,
+ show_cols=show_cols,
+ indent=indent,
+ latest_partition=latest_partition,
+ cols=cols,
+ )
Review Comment:
From Claude:
Why the review comment is wrong:
There is no connection to leak. The select_star method only builds a SQL
string. It never calls engine.connect(), engine.execute(), or anything that
would open a database connection. The only thing it used from the engine was
engine.dialect.
The old code was wasteful, not the new code. The original code created an
entire engine (and potentially an SSH tunnel) just to access engine.dialect for
string quoting. That's the whole point of this refactor — the PR title says
"use Dialect instead of Engine in select_star to avoid SSH tunnels."
get_dialect() creates no resources that need cleanup. It just parses the URI
string and instantiates a dialect object. There's nothing to leak.
The Bito reviewer appears to have pattern-matched on "removed context
manager" → "resource leak" without actually analyzing what resources were being
used inside the context manager. The refactor is correct and actually improves
resource management by not creating unnecessary engines and SSH tunnels.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
