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 2022/10/26 18:08:17 UTC
[GitHub] [superset] hughhhh opened a new pull request, #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
hughhhh opened a new pull request, #21943:
URL: https://github.com/apache/superset/pull/21943
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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
[GitHub] [superset] hughhhh merged pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
hughhhh merged PR #21943:
URL: https://github.com/apache/superset/pull/21943
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21943:
URL: https://github.com/apache/superset/pull/21943#issuecomment-1315723883
Ephemeral environment shutdown and build artifacts deleted.
--
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
[GitHub] [superset] betodealmeida commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1022015430
##########
superset/db_engine_specs/base.py:
##########
@@ -894,17 +903,17 @@ def df_to_sql(
:param to_sql_kwargs: The kwargs to be passed to pandas.DataFrame.to_sql` method
"""
- engine = cls.get_engine(database)
to_sql_kwargs["name"] = table.table
if table.schema:
# Only add schema when it is preset and non empty.
to_sql_kwargs["schema"] = table.schema
- if engine.dialect.supports_multivalues_insert:
- to_sql_kwargs["method"] = "multi"
+ with cls.get_engine(database) as engine:
+ if engine.dialect.supports_multivalues_insert:
+ to_sql_kwargs["method"] = "multi"
Review Comment:
Ah, good point, I missed that.
--
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
[GitHub] [superset] hughhhh commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1021986669
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -340,8 +340,12 @@ def df_to_sql(
if not table.schema:
raise Exception("The table schema must be defined")
- engine = cls.get_engine(database)
- to_gbq_kwargs = {"destination_table": str(table), "project_id": engine.url.host}
+ to_gbq_kwargs = {}
+ with cls.get_engine(database) as engine:
+ to_gbq_kwargs = {
+ "destination_table": str(table),
+ "project_id": engine.url.host,
Review Comment:
created a ticket for this
--
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
[GitHub] [superset] betodealmeida commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1014735512
##########
superset/db_engine_specs/base.py:
##########
@@ -472,7 +472,10 @@ def get_engine(
schema: Optional[str] = None,
source: Optional[utils.QuerySource] = None,
) -> Engine:
- return database.get_sqla_engine(schema=schema, source=source)
+ with database.get_sqla_engine_with_context(
+ schema=schema, source=source
+ ) as engine:
+ return engine
Review Comment:
This won't work; the context manager will be exited before the functions returns. It's equivalent to:
```python
with database.get_sqla_engine_with_context(...) as engine:
a = engine
return a
```
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21943:
URL: https://github.com/apache/superset/pull/21943#issuecomment-1301122754
@hughhhh Ephemeral environment spinning up at http://34.222.64.144:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] hughhhh commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1014884869
##########
superset/db_engine_specs/base.py:
##########
@@ -472,7 +472,10 @@ def get_engine(
schema: Optional[str] = None,
source: Optional[utils.QuerySource] = None,
) -> Engine:
- return database.get_sqla_engine(schema=schema, source=source)
+ with database.get_sqla_engine_with_context(
+ schema=schema, source=source
+ ) as engine:
+ return engine
Review Comment:
so should i just `return database._ get_sqla_engine()`
--
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
[GitHub] [superset] betodealmeida commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1014735512
##########
superset/db_engine_specs/base.py:
##########
@@ -472,7 +472,10 @@ def get_engine(
schema: Optional[str] = None,
source: Optional[utils.QuerySource] = None,
) -> Engine:
- return database.get_sqla_engine(schema=schema, source=source)
+ with database.get_sqla_engine_with_context(
+ schema=schema, source=source
+ ) as engine:
+ return engine
Review Comment:
This won't work; before returning the context manager will be exited before the functions returns. It's equivalent to:
```python
with database.get_sqla_engine_with_context(...) as engine:
a = engine
return a
```
--
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
[GitHub] [superset] betodealmeida commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1019660134
##########
superset/databases/commands/test_connection.py:
##########
@@ -100,31 +99,36 @@ def ping(engine: Engine) -> bool:
with closing(engine.raw_connection()) as conn:
return engine.dialect.do_ping(conn)
- try:
- alive = func_timeout(
- int(app.config["TEST_DATABASE_CONNECTION_TIMEOUT"].total_seconds()),
- ping,
- args=(engine,),
- )
- except (sqlite3.ProgrammingError, RuntimeError):
- # SQLite can't run on a separate thread, so ``func_timeout`` fails
- # RuntimeError catches the equivalent error from duckdb.
- alive = engine.dialect.do_ping(engine)
- except FunctionTimedOut as ex:
- raise SupersetTimeoutException(
- error_type=SupersetErrorType.CONNECTION_DATABASE_TIMEOUT,
- message=(
- "Please check your connection details and database settings, "
- "and ensure that your database is accepting connections, "
- "then try connecting again."
- ),
- level=ErrorLevel.ERROR,
- extra={"sqlalchemy_uri": database.sqlalchemy_uri},
- ) from ex
- except Exception as ex: # pylint: disable=broad-except
- alive = False
- # So we stop losing the original message if any
- ex_str = str(ex)
+ with database.get_sqla_engine_with_context() as engine:
+ try:
+ alive = func_timeout(
+ int(
+ app.config[
+ "TEST_DATABASE_CONNECTION_TIMEOUT"
+ ].total_seconds()
+ ),
Review Comment:
Nit, I'd compute this outside the `with` block just to improve readability:
```python
timeout = app.config["TEST_DATABASE_CONNECTION_TIMEOUT"].total_seconds()
with database.get_sqla_engine_with_context() as engine:
try:
alive = func_timeout(int(timeout), ping, args=(engine,))
```
Also, `func_timeout` should take floats. :-P
##########
superset/db_engine_specs/base.py:
##########
@@ -894,17 +903,17 @@ def df_to_sql(
:param to_sql_kwargs: The kwargs to be passed to pandas.DataFrame.to_sql` method
"""
- engine = cls.get_engine(database)
to_sql_kwargs["name"] = table.table
if table.schema:
# Only add schema when it is preset and non empty.
to_sql_kwargs["schema"] = table.schema
- if engine.dialect.supports_multivalues_insert:
- to_sql_kwargs["method"] = "multi"
+ with cls.get_engine(database) as engine:
+ if engine.dialect.supports_multivalues_insert:
+ to_sql_kwargs["method"] = "multi"
Review Comment:
I think we need to improve this a little bit... here we're building an engine just to check an attribute in the dialect, which means we're setting up and tearing down an SSH connection just to read an attribute. :-(
Maybe we should add a `get_dialect` method to the DB engine spec, that builds the engine without the context manager:
```python
@classmethod
def get_dialect(database, schema, source):
engine = database.get_sqla_engine(schema=schema, source=source)
return engine.dialect
```
Then when we only need the dialect we can call this method, which is cheaper.
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -340,8 +340,12 @@ def df_to_sql(
if not table.schema:
raise Exception("The table schema must be defined")
- engine = cls.get_engine(database)
- to_gbq_kwargs = {"destination_table": str(table), "project_id": engine.url.host}
+ to_gbq_kwargs = {}
+ with cls.get_engine(database) as engine:
+ to_gbq_kwargs = {
+ "destination_table": str(table),
+ "project_id": engine.url.host,
Review Comment:
I wonder if we should add an attribute to DB engine specs annotating if they support SSH tunnel or not? BigQuery, eg, will probably never support it.
##########
superset/datasets/commands/importers/v1/utils.py:
##########
@@ -168,7 +168,8 @@ def load_data(
connection = session.connection()
else:
logger.warning("Loading data outside the import transaction")
- connection = database.get_sqla_engine()
+ with database.get_sqla_engine_with_context() as engine:
+ connection = engine
Review Comment:
Engine will be closed after line 172, making `df.to_sql` fail. You need to have `df.to_sql` inside the context manager, while the engine is open.
##########
superset/connectors/sqla/utils.py:
##########
@@ -137,13 +136,18 @@ def get_virtual_table_metadata(dataset: SqlaTable) -> List[ResultSetColumnType]:
# TODO(villebro): refactor to use same code that's used by
# sql_lab.py:execute_sql_statements
try:
- with closing(engine.raw_connection()) as conn:
- cursor = conn.cursor()
- query = dataset.database.apply_limit_to_sql(statements[0], limit=1)
- db_engine_spec.execute(cursor, query)
- result = db_engine_spec.fetch_data(cursor, limit=1)
- result_set = SupersetResultSet(result, cursor.description, db_engine_spec)
- cols = result_set.columns
+ with dataset.database.get_sqla_engine_with_context(
+ schema=dataset.schema
+ ) as engine:
+ with closing(engine.raw_connection()) as conn:
Review Comment:
We don't need to do this now, but eventually we could have a `database.get_raw_connection` context manager, to simplify things a little bit. This way we could rewrite this as:
```python
with dataset.database.get_raw_connection(schema=dataset.schema) as conn:
cursor = conn.cursor()
...
```
And the implementation of `get_raw_connection()` would take care of closing the connection:
```python
@contextmanager
def get_raw_connection(...):
with get_sqla_engine_with_context(...) as engine:
with closing(engine.raw_connection()) as conn:
yield conn
```
##########
tests/integration_tests/sqllab_tests.py:
##########
@@ -733,7 +733,7 @@ def test_execute_sql_statements(self, mock_execute_sql_statement, mock_get_query
mock_query = mock.MagicMock()
mock_query.database.allow_run_async = False
mock_cursor = mock.MagicMock()
- mock_query.database.get_sqla_engine.return_value.raw_connection.return_value.cursor.return_value = (
+ mock_query.database.get_sqla_engine_with_context.return_value.__enter__.return_value.raw_connection.return_value.cursor.return_value = (
Review Comment:
You can replace all intermediary `.return_value` with `()` (but not the last):
```suggestion
mock_query.database.get_sqla_engine_with_context().__enter__().raw_connection().cursor.return_value = (
```
##########
superset/examples/paris.py:
##########
@@ -28,29 +28,29 @@
def load_paris_iris_geojson(only_metadata: bool = False, force: bool = False) -> None:
tbl_name = "paris_iris_mapping"
database = database_utils.get_example_database()
- engine = database.get_sqla_engine()
- schema = inspect(engine).default_schema_name
- table_exists = database.has_table_by_name(tbl_name)
+ with database.get_sqla_engine_with_context() as engine:
Review Comment:
We really should convert these old examples into the new format (YAML + CSV)...
##########
superset/db_engine_specs/hive.py:
##########
@@ -205,7 +203,8 @@ def df_to_sql(
if table_exists:
raise SupersetException("Table already exists")
elif to_sql_kwargs["if_exists"] == "replace":
- engine.execute(f"DROP TABLE IF EXISTS {str(table)}")
+ with cls.get_engine(database) as engine:
+ engine.execute(f"DROP TABLE IF EXISTS {str(table)}")
Review Comment:
It's interesting that sometimes we use `engine.raw_connection().execute`, and others we use `engine.execute`. Ideally we should standardize in the latter wherever possible, since it's more concise.
##########
superset/databases/commands/validate.py:
##########
@@ -101,30 +101,30 @@ def run(self) -> None:
database.set_sqlalchemy_uri(sqlalchemy_uri)
database.db_engine_spec.mutate_db_for_connection_test(database)
- engine = database.get_sqla_engine()
- try:
- with closing(engine.raw_connection()) as conn:
- alive = engine.dialect.do_ping(conn)
- except Exception as ex:
- url = make_url_safe(sqlalchemy_uri)
- context = {
- "hostname": url.host,
- "password": url.password,
- "port": url.port,
- "username": url.username,
- "database": url.database,
- }
- errors = database.db_engine_spec.extract_errors(ex, context)
- raise DatabaseTestConnectionFailedError(errors) from ex
+ with database.get_sqla_engine_with_context() as engine:
+ try:
+ with closing(engine.raw_connection()) as conn:
+ alive = engine.dialect.do_ping(conn)
+ except Exception as ex:
+ url = make_url_safe(sqlalchemy_uri)
+ context = {
+ "hostname": url.host,
+ "password": url.password,
+ "port": url.port,
+ "username": url.username,
+ "database": url.database,
+ }
+ errors = database.db_engine_spec.extract_errors(ex, context)
+ raise DatabaseTestConnectionFailedError(errors) from ex
- if not alive:
- raise DatabaseOfflineError(
- SupersetError(
- message=__("Database is offline."),
- error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
- level=ErrorLevel.ERROR,
- ),
- )
+ if not alive:
+ raise DatabaseOfflineError(
+ SupersetError(
+ message=__("Database is offline."),
+ error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
+ level=ErrorLevel.ERROR,
+ ),
+ )
Review Comment:
We can dedent this block, no?
##########
superset/models/core.py:
##########
@@ -370,11 +370,11 @@ def get_sqla_engine_with_context(
source: Optional[utils.QuerySource] = None,
) -> Engine:
try:
- yield self.get_sqla_engine(schema=schema, nullpool=nullpool, source=source)
+ yield self._get_sqla_engine(schema=schema, nullpool=nullpool, source=source)
except Exception as ex:
- raise self.db_engine_spec.get_dbapi_mapped_exception(ex)
+ raise ex
Review Comment:
You don't need `try/except` here if you're raising all exceptions.
--
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
[GitHub] [superset] hughhhh commented on pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
hughhhh commented on PR #21943:
URL: https://github.com/apache/superset/pull/21943#issuecomment-1300922331
/testenv up
--
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
[GitHub] [superset] betodealmeida commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1015736288
##########
superset/db_engine_specs/base.py:
##########
@@ -472,7 +472,10 @@ def get_engine(
schema: Optional[str] = None,
source: Optional[utils.QuerySource] = None,
) -> Engine:
- return database.get_sqla_engine(schema=schema, source=source)
+ with database.get_sqla_engine_with_context(
+ schema=schema, source=source
+ ) as engine:
+ return engine
Review Comment:
> so should i just `return database._ get_sqla_engine()`
If you do that, then the SSH tunneling wouldn't be setup in places that call `DBEngineSpec.get_engine()`, defeating the purpose of the changes in this PR. You need to convert this into a context manager as well (and update everything downstream), so that the setup/teardown works as expected.
You can play with a simple Python script to understand how this works:
```python
from contextlib import contextmanager
@contextmanager
def get_sqla_engine_with_context():
print("enabling ssh tunnel")
yield 42
print("disabling ssh tunnel")
def get_engine():
with get_sqla_engine_with_context() as engine:
return engine
print("start")
engine = get_engine()
print("I have the engine, I can now work on it")
print("end")
```
Running the code above prints:
```before
start
enabling ssh tunnel
disabling ssh tunnel
I have the engine, I can now work on it
end
```
--
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
[GitHub] [superset] codecov[bot] commented on pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21943:
URL: https://github.com/apache/superset/pull/21943#issuecomment-1295332312
# [Codecov](https://codecov.io/gh/apache/superset/pull/21943?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21943](https://codecov.io/gh/apache/superset/pull/21943?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87c0d79) into [master](https://codecov.io/gh/apache/superset/commit/7f563cf92df82ab49590407adf6a7bce3eef1779?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7f563cf) will **decrease** coverage by `14.67%`.
> The diff coverage is `1.66%`.
```diff
@@ Coverage Diff @@
## master #21943 +/- ##
===========================================
- Coverage 66.96% 52.28% -14.68%
===========================================
Files 1807 1807
Lines 69221 69227 +6
Branches 7402 7402
===========================================
- Hits 46352 36195 -10157
- Misses 20964 31127 +10163
Partials 1905 1905
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `?` | |
| python | `51.09% <1.66%> (-30.40%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.09% <1.66%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `57.38% <0.00%> (-33.63%)` | :arrow_down: |
| [superset/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3V0aWxzLnB5) | `41.74% <0.00%> (-48.45%)` | :arrow_down: |
| [superset/databases/commands/validate.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3ZhbGlkYXRlLnB5) | `29.31% <0.00%> (-48.28%)` | :arrow_down: |
| [superset/datasets/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `55.29% <0.00%> (-3.04%)` | :arrow_down: |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `73.41% <0.00%> (-16.29%)` | :arrow_down: |
| [superset/examples/bart\_lines.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmFydF9saW5lcy5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `0.00% <0.00%> (-71.43%)` | :arrow_down: |
| [superset/examples/country\_map.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvY291bnRyeV9tYXAucHk=) | `0.00% <0.00%> (ø)` | |
| [superset/examples/energy.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvZW5lcmd5LnB5) | `0.00% <0.00%> (ø)` | |
| [superset/examples/flights.py](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvZmxpZ2h0cy5weQ==) | `0.00% <0.00%> (ø)` | |
| ... and [333 more](https://codecov.io/gh/apache/superset/pull/21943/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [superset] hughhhh commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1021984198
##########
superset/db_engine_specs/base.py:
##########
@@ -894,17 +903,17 @@ def df_to_sql(
:param to_sql_kwargs: The kwargs to be passed to pandas.DataFrame.to_sql` method
"""
- engine = cls.get_engine(database)
to_sql_kwargs["name"] = table.table
if table.schema:
# Only add schema when it is preset and non empty.
to_sql_kwargs["schema"] = table.schema
- if engine.dialect.supports_multivalues_insert:
- to_sql_kwargs["method"] = "multi"
+ with cls.get_engine(database) as engine:
+ if engine.dialect.supports_multivalues_insert:
+ to_sql_kwargs["method"] = "multi"
Review Comment:
in this case we still need the `engine` though, so it makes more sense to just use `get_engine` instead of just the dialect
https://github.com/apache/superset/pull/21943/files/7ce583678e1770d472527abb8270dd22e666b9c0#diff-2e62d64ef1113e48efdfeb2acbaa522fca13e49e6a00c2cfd4f74efc4ae1b45cR916
--
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
[GitHub] [superset] betodealmeida commented on a diff in pull request #21943: feat: refactor all `get_sqla_engine` to use contextmanager in codebase
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1022105826
##########
superset/datasets/commands/importers/v1/utils.py:
##########
@@ -168,15 +168,16 @@ def load_data(
connection = session.connection()
else:
logger.warning("Loading data outside the import transaction")
- connection = database.get_sqla_engine()
-
- df.to_sql(
- dataset.table_name,
- con=connection,
- schema=dataset.schema,
- if_exists="replace",
- chunksize=CHUNKSIZE,
- dtype=dtype,
- index=False,
- method="multi",
- )
+ with database.get_sqla_engine_with_context() as engine:
+ connection = engine
+
+ df.to_sql(
Review Comment:
This will only run in the `else` block, but it needs to run in the `if database.sqlalchemy_uri == current_app.config.get("SQLALCHEMY_DATABASE_URI"):` block as well.
--
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