sadpandajoe commented on code in PR #43640: URL: https://github.com/apache/superset/pull/43640#discussion_r3878831481
########## requirements/development.in: ########## @@ -16,7 +16,7 @@ # specific language governing permissions and limitations # under the License. # --e .[development,bigquery,cockroachdb,crate,druid,duckdb,elasticsearch,fastmcp,gevent,gsheets,mssql,mysql,oracle,postgres,presto,prophet,trino,thumbnails] +-e .[development,bigquery,cockroachdb,crate,druid,duckdb,elasticsearch,fastmcp,gevent,gsheets,monetdb,mongodb,mssql,mysql,oracle,postgres,presto,prophet,trino,thumbnails] Review Comment: These dialect dependencies are installed by the Testcontainers workflow, but that workflow only runs for its own file and `tests/testcontainers/**`. Could its `pull_request.paths` also include the dependency manifests (and the engine-spec paths they exercise) so a driver or lockfile-only change cannot bypass this coverage until the nightly run? ########## tests/testcontainers/db_engine_specs/test_mongodb.py: ########## @@ -0,0 +1,104 @@ +# 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. +""" +Tests db_engine_specs.mongodb against a real MongoDB instance, spun up on +demand via testcontainers. Run via .github/workflows/testcontainers.yml. + +MongoDB is schemaless, and Superset talks to it via `pymongosql`, a +SQL-to-MongoDB translation layer (dialect requires a `?mode=superset` query +param -- not part of testcontainers' own MongoDbContainer.get_connection_url()). +Documents get inserted via the native pymongo driver, not SQL INSERT, +matching how Superset actually encounters MongoDB in practice and avoiding +any assumption about pymongosql's own INSERT/DDL support. +""" + +from collections.abc import Iterator + +import pytest +from sqlalchemy import create_engine, inspect, text +from sqlalchemy.engine import Engine + +from superset.db_engine_specs.mongodb import MongoDBEngineSpec +from superset.sql.parse import Table + +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.mongodb") +require_driver("pymongosql") + +from testcontainers.community.mongodb import MongoDbContainer # noqa: E402 + +COLLECTION = "pilot_pagination" + + [email protected](scope="module") +def engine() -> Iterator[Engine]: + with MongoDbContainer("mongo:7.0.7") as container: + client = container.get_connection_client() + client[container.dbname][COLLECTION].insert_many([{"id": i} for i in range(10)]) + # MongoDbContainer.get_connection_url() has no database path segment + # or query string at all (it only builds user:pass@host:port), so + # naively appending "&mode=superset" glues it straight onto the port + # number instead of starting a query string. Build the full URL + # ourselves instead of relying on string concatenation. + host = container.get_container_host_ip() + port = container.get_exposed_port(container.port) + # authSource=admin is required: MongoDbContainer creates its root + # user via MONGO_INITDB_ROOT_USERNAME, which lives in the `admin` + # database, not in `dbname` -- without it, auth fails against + # whatever database is in the URL path. + yield create_engine( + f"mongodb://{container.username}:{container.password}@{host}:{port}" + f"/{container.dbname}?mode=superset&authSource=admin" + ) + + +def test_paginated_query_returns_correct_rows_in_order(engine: Engine) -> None: + """ + A plain LIMIT/OFFSET query, compiled and executed against a real + instance. Mocked tests cannot catch a dialect compiling this + incorrectly (see apache/superset#42899, where Trino emitted OFFSET + before LIMIT) -- only real execution can. Unlike Elasticsearch's SQL + layer (which has no OFFSET support at all), pymongosql maps OFFSET to + MongoDB's native `skip`, so this dialect supports it. + """ + with engine.connect() as conn: + rows = conn.execute( + text( + f"SELECT id FROM {COLLECTION} ORDER BY id LIMIT 3 OFFSET 4" # noqa: S608 Review Comment: This literal SQL bypasses SQLAlchemy compilation, so an engine-spec regression in LIMIT/OFFSET ordering can leave this test green despite the stated purpose. Could this execute a Core `select(...).limit(3).offset(4)` and keep the row-order assertion? ########## tests/testcontainers/db_engine_specs/test_monetdb.py: ########## @@ -0,0 +1,111 @@ +# 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. +""" +Tests db_engine_specs.monetdb against a real MonetDB instance, spun up on +demand via testcontainers. Run via .github/workflows/testcontainers.yml. + +monetdb/monetdb publishes an amd64-only image, so this needs Rosetta/QEMU +emulation on Apple Silicon -- unlike CrateDB's x86-64-v3 CPU requirement, +this one actually runs fine under emulation (verified locally). No native +testcontainers module exists for MonetDB, so this uses a generic +DockerContainer with the documented MDB_* environment variables and waits +for the daemon's own startup log line. +""" + +import re +from collections.abc import Iterator + +import pytest +from sqlalchemy import ( + Column, + create_engine, + inspect, + Integer, + MetaData, + Table as SATable, +) +from sqlalchemy.engine import Engine + +from superset.db_engine_specs.monetdb import MonetDbEngineSpec +from superset.sql.parse import Table + +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.core.container") +require_driver("sqlalchemy_monetdb") + +from testcontainers.core.container import DockerContainer # noqa: E402 +from testcontainers.core.wait_strategies import LogMessageWaitStrategy # noqa: E402 + +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) + +PORT = 50000 +PASSWORD = "monetdb" # noqa: S105 -- fixed test-fixture password, not a secret +DBNAME = "test" + + [email protected](scope="module") +def engine() -> Iterator[Engine]: + container = DockerContainer("monetdb/monetdb:latest") + container.with_exposed_ports(PORT) + container.with_env("MDB_DB_ADMIN_PASS", PASSWORD) + container.with_env("MDB_CREATE_DBS", DBNAME) + container.waiting_for(LogMessageWaitStrategy(re.compile("Starting MonetDB daemon"))) Review Comment: This log line is emitted before the image runs `monetdbd start -n`, so the fixture can attempt DDL before the database accepts connections and make the CI leg flaky. Could the container wait for the exposed port or a successful SQL connection instead? ########## tests/testcontainers/db_engine_specs/test_mariadb.py: ########## @@ -0,0 +1,112 @@ +# 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. +""" +Tests db_engine_specs.mariadb against a real MariaDB instance, spun up on +demand via testcontainers. Run via .github/workflows/testcontainers.yml. + +MariaDB is a MySQL fork implementing the same wire protocol: connects via +the plain "mysql" dialect with mysqlclient, same as vanilla MySQL, just +pointed at the mariadb image instead of mysql:latest. + +Could not be verified locally in this environment: mysqlclient (MySQLdb) +has a pre-existing, unrelated native-library linking issue against this +machine's Homebrew-installed libmysqlclient. CI installs it via apt on +Linux, where this does not occur. +""" + +from collections.abc import Iterator + +import pytest +from sqlalchemy import ( + Column, + create_engine, + inspect, + Integer, + MetaData, + Table as SATable, +) +from sqlalchemy.engine import Engine + +from superset.db_engine_specs.mariadb import MariaDBEngineSpec +from superset.sql.parse import Table +from superset.utils.core import GenericDataType + +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.community.mysql") + +from testcontainers.community.mysql import MySqlContainer # noqa: E402 + +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) + + [email protected](scope="module") +def engine() -> Iterator[Engine]: + with MySqlContainer("mariadb:11") as container: + # get_connection_url() has no host override and defaults to + # get_container_host_ip(), which is the literal string "localhost" + # on native Linux Docker (e.g. GitHub Actions runners). MySQLdb + # (mysqlclient) treats a "localhost" host specially and attempts a + # Unix socket connection instead of TCP, which fails since there's + # no local MySQL socket -- the container is reached over the + # network. Forcing 127.0.0.1 keeps it on TCP. + port = container.get_exposed_port(container.port) + yield create_engine( + f"mysql://{container.username}:{container.password}" Review Comment: This forces the connection to the pytest host, so the MariaDB suite cannot reach a remote Docker daemon even though Testcontainers resolved the published port from that daemon. Could this preserve `get_container_host_ip()` when it is remote and only rewrite the local `localhost` case to `127.0.0.1`? ########## tests/testcontainers/db_engine_specs/test_monetdb.py: ########## @@ -0,0 +1,111 @@ +# 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. +""" +Tests db_engine_specs.monetdb against a real MonetDB instance, spun up on +demand via testcontainers. Run via .github/workflows/testcontainers.yml. + +monetdb/monetdb publishes an amd64-only image, so this needs Rosetta/QEMU +emulation on Apple Silicon -- unlike CrateDB's x86-64-v3 CPU requirement, +this one actually runs fine under emulation (verified locally). No native +testcontainers module exists for MonetDB, so this uses a generic +DockerContainer with the documented MDB_* environment variables and waits +for the daemon's own startup log line. +""" + +import re +from collections.abc import Iterator + +import pytest +from sqlalchemy import ( + Column, + create_engine, + inspect, + Integer, + MetaData, + Table as SATable, +) +from sqlalchemy.engine import Engine + +from superset.db_engine_specs.monetdb import MonetDbEngineSpec +from superset.sql.parse import Table + +pytestmark = pytest.mark.testcontainers + +from ._driver import require_driver # noqa: E402 + +require_driver("testcontainers.core.container") +require_driver("sqlalchemy_monetdb") + +from testcontainers.core.container import DockerContainer # noqa: E402 +from testcontainers.core.wait_strategies import LogMessageWaitStrategy # noqa: E402 + +from ._pagination import ( # noqa: E402 + assert_paginated_query_returns_correct_rows_in_order, +) + +PORT = 50000 +PASSWORD = "monetdb" # noqa: S105 -- fixed test-fixture password, not a secret +DBNAME = "test" + + [email protected](scope="module") +def engine() -> Iterator[Engine]: + container = DockerContainer("monetdb/monetdb:latest") + container.with_exposed_ports(PORT) + container.with_env("MDB_DB_ADMIN_PASS", PASSWORD) + container.with_env("MDB_CREATE_DBS", DBNAME) + container.waiting_for(LogMessageWaitStrategy(re.compile("Starting MonetDB daemon"))) + + with container: + host = container.get_container_host_ip() + port = container.get_exposed_port(PORT) + yield create_engine(f"monetdb://monetdb:{PASSWORD}@{host}:{port}/{DBNAME}") + + +def test_paginated_query_returns_correct_rows_in_order(engine: Engine) -> None: + """ + A plain SQLAlchemy Core LIMIT/OFFSET query, compiled and executed against + a real instance. Mocked tests cannot catch a dialect compiling this + incorrectly (see apache/superset#42899, where Trino emitted OFFSET + before LIMIT) -- only real execution can. + """ + assert_paginated_query_returns_correct_rows_in_order(engine) + + +def test_get_columns_maps_native_types(engine: Engine) -> None: + """ + MonetDbEngineSpec.get_columns wraps a real SQLAlchemy Inspector; this + exercises that against actual server-reported column metadata rather + than a mocked Inspector. + """ + metadata = MetaData() + SATable( + "pilot_types", + metadata, + Column("id", Integer, primary_key=True), + Column("amount", Integer), + ) + metadata.create_all(engine) + + inspector = inspect(engine) + columns = MonetDbEngineSpec.get_columns(inspector, Table("pilot_types")) + + by_name = {col["column_name"]: col for col in columns} + assert set(by_name) == {"id", "amount"} + for col in by_name.values(): + spec = MonetDbEngineSpec.get_column_spec(str(col["type"])) Review Comment: A wrong native-type mapping still passes here whenever it maps to any recognized type; for example, an integer reported as `VARCHAR` yields a non-null spec. Could this assert the numeric generic type and integer SQLAlchemy type, as the MariaDB and TimescaleDB tests do (and apply the same check to the other new weak mapping tests)? -- 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]
