rusackas commented on code in PR #43502:
URL: https://github.com/apache/superset/pull/43502#discussion_r3858343957
##########
pyproject.toml:
##########
@@ -142,7 +142,13 @@ bigquery = [
"google-cloud-bigquery>=3.42.3",
]
clickhouse = ["clickhouse-connect>=1.7.1, <2.0"]
-cockroachdb = ["cockroachdb>=0.3.5, <0.4"]
+# The `cockroachdb` PyPI package (last released 2021) is abandoned and its
+# SQLAlchemy dialect cannot even import under SQLAlchemy 2.0 (it references
+# sqlalchemy.dialects.postgresql.psycopg2.PGCompiler_psycopg2, removed in
+# 2.0). sqlalchemy-cockroachdb is the actively maintained replacement,
+# already linked from CockroachDbEngineSpec.metadata's docs_url, and
+# registers the same `cockroachdb` SQLAlchemy dialect entry point.
+cockroachdb = ["sqlalchemy-cockroachdb>=2.0.0, <3"]
Review Comment:
Good catch, pinned `psycopg2-binary` alongside it, same driver the
`postgres` extra already uses.
##########
.github/workflows/testcontainers.yml:
##########
@@ -0,0 +1,91 @@
+# db_engine_specs tests against real databases (testcontainers)
+name: Testcontainers
+
+# Spins up real Docker containers (see tests/testcontainers/ for the current
+# dialect list) via testcontainers-python, which catches real dialect/driver
+# regressions -- the kind mocked db_engine_specs unit tests structurally
+# cannot, e.g. apache/superset#42899 (Trino emitting OFFSET before LIMIT).
+# Runs on a nightly cron (catches drift from a driver's own releases, not
+# just from Superset's changes) and on pull_request, scoped via `paths` to
+# only PRs that actually touch this test suite or the workflow itself, so
+# unrelated PRs across the repo are never affected.
+permissions:
+ contents: read
+
+on:
+ schedule:
+ - cron: "0 5 * * *"
+ workflow_dispatch: {}
+ pull_request:
+ paths:
+ - ".github/workflows/testcontainers.yml"
+ - "tests/testcontainers/**"
+
+concurrency:
Review Comment:
Fixed, scoped the group by ref so a PR run and the nightly cron don't cancel
each other.
##########
tests/testcontainers/db_engine_specs/test_cockroachdb.py:
##########
@@ -0,0 +1,89 @@
+# 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.cockroachdb against a real CockroachDB instance,
+spun up on demand via testcontainers. Run nightly (see
+.github/workflows/nightly-testcontainers.yml), not on every merge -- these
+exercise real SQL execution and dialect introspection, which mocked unit
+tests structurally cannot.
+"""
+
+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.cockroachdb import CockroachDbEngineSpec
+from superset.sql.parse import Table
+
+pytest.importorskip("testcontainers.community.cockroachdb")
+
+from testcontainers.community.cockroachdb import CockroachDBContainer # noqa:
E402
+
+from ._pagination import assert_paginated_query_returns_correct_rows_in_order
+
+
[email protected](scope="module")
+def engine() -> Iterator[Engine]:
+ # sqlalchemy-cockroachdb registers its dialect under the plain
+ # "cockroachdb" name; the container's own default ("cockroachdb+psycopg2")
+ # matches the abandoned `cockroachdb` package instead (see #43501).
+ with CockroachDBContainer(dialect="cockroachdb") as container:
+ yield create_engine(container.get_connection_url())
+
+
+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:
+ """
+ CockroachDbEngineSpec.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 = CockroachDbEngineSpec.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 = CockroachDbEngineSpec.get_column_spec(str(col["type"]))
+ assert spec is not None
Review Comment:
Fixed, matching the Trino test now: asserts `generic_type == NUMERIC` and
`isinstance(sqla_type, Integer)`.
##########
tests/testcontainers/db_engine_specs/test_cockroachdb.py:
##########
@@ -0,0 +1,89 @@
+# 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.cockroachdb against a real CockroachDB instance,
+spun up on demand via testcontainers. Run nightly (see
+.github/workflows/nightly-testcontainers.yml), not on every merge -- these
+exercise real SQL execution and dialect introspection, which mocked unit
+tests structurally cannot.
+"""
+
+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.cockroachdb import CockroachDbEngineSpec
+from superset.sql.parse import Table
+
+pytest.importorskip("testcontainers.community.cockroachdb")
Review Comment:
Fixed. The CI job now sets `SUPERSET_TESTCONTAINERS_STRICT`, so a
broken/missing driver import fails the job instead of skipping quietly.
##########
requirements/development.in:
##########
@@ -16,5 +16,8 @@
# specific language governing permissions and limitations
# under the License.
#
--e
.[development,bigquery,druid,duckdb,fastmcp,gevent,gsheets,mysql,postgres,presto,prophet,trino,thumbnails]
+-e
.[development,bigquery,cockroachdb,crate,db2,druid,duckdb,elasticsearch,fastmcp,gevent,gsheets,mssql,mysql,oracle,postgres,presto,prophet,trino,thumbnails]
-e ./superset-extensions-cli[test]
+# testcontainers-backed db_engine_specs tests (tests/testcontainers/) --
+# see .github/workflows/testcontainers.yml
+testcontainers[cockroachdb,cratedb,db2,mssql,oracle,trino]>=4.15.0,<5
Review Comment:
Fixed, added a `testcontainers` pytest marker excluded by default in
`pytest.ini`. A plain `pytest` run skips these now, CI opts back in with `-m
testcontainers`.
##########
UPDATING.md:
##########
@@ -25,6 +25,7 @@ assists people when migrating to a new version.
## Next
- `SAMPLES_ROW_LIMIT` is now the default for `/datasource/samples` requests
without a valid explicit `per_page`, rather than a hard per-request ceiling;
explicit limits are honored up to the existing global row-limit ceiling,
matching `/chart/data` SAMPLES requests.
+- The `cockroachdb` extra (`pip install apache-superset[cockroachdb]`) now
installs `sqlalchemy-cockroachdb` instead of the abandoned `cockroachdb`
package, whose SQLAlchemy dialect could not be imported under SQLAlchemy 2.0.
Existing environments with the old package installed should `pip uninstall
cockroachdb && pip install sqlalchemy-cockroachdb` (or simply reinstall the
extra) to restore CockroachDB connectivity.
Review Comment:
Fixed, the note now says to uninstall the old package outright instead of
presenting reinstalling the extra as sufficient on its own.
--
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]
