From d0a2514981d2c3b7e13c5b1b99488d8c15c8ba08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 12:16:30 +0100 Subject: [PATCH 01/20] test: add system tests --- .github/workflows/test_suite.yml | 24 ++++++++++++++++++++ noxfile.py | 30 ++++++++++++++++++++++++ test/system/test_basics.py | 39 ++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 test/system/test_basics.py diff --git a/.github/workflows/test_suite.yml b/.github/workflows/test_suite.yml index d79b89dd..0606d102 100644 --- a/.github/workflows/test_suite.yml +++ b/.github/workflows/test_suite.yml @@ -126,6 +126,30 @@ jobs: SPANNER_EMULATOR_HOST: localhost:9010 GOOGLE_CLOUD_PROJECT: appdev-soda-spanner-staging + system: + runs-on: ubuntu-latest + + services: + emulator-0: + image: gcr.io/cloud-spanner-emulator/emulator:latest + ports: + - 9010:9010 + + steps: + - name: Checkout code + uses: actions/checkout@v2 + - name: Setup Python + uses: actions/setup-python@v4 + with: + python-version: 3.12 + - name: Install nox + run: python -m pip install nox + - name: Run System Tests + run: nox -s system + env: + SPANNER_EMULATOR_HOST: localhost:9010 + GOOGLE_CLOUD_PROJECT: appdev-soda-spanner-staging + migration_tests: runs-on: ubuntu-latest diff --git a/noxfile.py b/noxfile.py index eff6bfc9..8370e702 100644 --- a/noxfile.py +++ b/noxfile.py @@ -252,6 +252,36 @@ def compliance_test_20(session): ) +@nox.session(python=DEFAULT_PYTHON_VERSION_FOR_SQLALCHEMY_20) +def system(session): + """Run SQLAlchemy dialect system test suite.""" + + # Sanity check: Only run tests if the environment variable is set. + if not os.environ.get("GOOGLE_APPLICATION_CREDENTIALS", "") and not os.environ.get( + "SPANNER_EMULATOR_HOST", "" + ): + session.skip( + "Credentials or emulator host must be set via environment variable" + ) + + session.install( + "pytest", + "pytest-cov", + "pytest-asyncio", + ) + + session.install("mock") + session.install(".[tracing]") + session.install("opentelemetry-api==1.27.0") + session.install("opentelemetry-sdk==1.27.0") + session.install("opentelemetry-instrumentation==0.48b0") + session.run("python", "create_test_database.py") + + session.install("sqlalchemy>=2.0") + + session.run("py.test", "--quiet", os.path.join("test", "system"), *session.posargs) + + @nox.session(python=DEFAULT_PYTHON_VERSION) def unit(session): """Run unit tests.""" diff --git a/test/system/test_basics.py b/test/system/test_basics.py new file mode 100644 index 00000000..6a53b234 --- /dev/null +++ b/test/system/test_basics.py @@ -0,0 +1,39 @@ +# Copyright 2024 Google LLC All rights reserved. +# +# Licensed 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. +from sqlalchemy import text, Table, Column, Integer, PrimaryKeyConstraint, String +from sqlalchemy.testing import eq_ +from sqlalchemy.testing.plugin.plugin_base import fixtures + + +class TestBasics(fixtures.TablesTest): + @classmethod + def define_tables(cls, metadata): + Table( + "numbers", + metadata, + Column("number", Integer), + Column("name", String(20)), + PrimaryKeyConstraint("number"), + ) + + def test_hello_world(self, connection): + greeting = connection.execute(text("select 'Hello World'")) + eq_("Hello World", greeting.fetchone()[0]) + + def test_insert_number(self, connection): + connection.execute( + text("insert or update into numbers(number, name) values (1, 'One')") + ) + name = connection.execute(text("select name from numbers where number=1")) + eq_("One", name.fetchone()[0]) From 09ad9c4b3b6f10da78e217f8246b048853483e48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 14:35:57 +0100 Subject: [PATCH 02/20] test: run system tests on prod --- .kokoro/presubmit/compliance.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.kokoro/presubmit/compliance.cfg b/.kokoro/presubmit/compliance.cfg index 1261d8a0..6e4f768f 100644 --- a/.kokoro/presubmit/compliance.cfg +++ b/.kokoro/presubmit/compliance.cfg @@ -3,5 +3,5 @@ # Only run this nox session. env_vars: { key: "NOX_SESSION" - value: "unit" + value: "system" } From bab0414848e329808346840b998b7983144b9a91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 14:48:15 +0100 Subject: [PATCH 03/20] build: allow any Python version for sys tests --- noxfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noxfile.py b/noxfile.py index 8370e702..9244b757 100644 --- a/noxfile.py +++ b/noxfile.py @@ -252,7 +252,7 @@ def compliance_test_20(session): ) -@nox.session(python=DEFAULT_PYTHON_VERSION_FOR_SQLALCHEMY_20) +@nox.session() def system(session): """Run SQLAlchemy dialect system test suite.""" From e517c87abfff34e7e0ad1159d7f17e1e6ed82abd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 15:55:57 +0100 Subject: [PATCH 04/20] build: keep instance and create new databases instead --- create_test_config.py | 19 ++++++----- create_test_database.py | 73 +++++++++++++++++++++++++---------------- noxfile.py | 3 +- 3 files changed, 57 insertions(+), 38 deletions(-) diff --git a/create_test_config.py b/create_test_config.py index 7ef6f255..388cba86 100644 --- a/create_test_config.py +++ b/create_test_config.py @@ -18,18 +18,18 @@ import sys -def set_test_config(project, instance, user=None, password=None, host=None, port=None): +def set_test_config(project, instance, database, user=None, password=None, host=None, port=None): config = configparser.ConfigParser() if user is not None and password is not None and host is not None and port is not None: url = ( f"spanner+spanner://{user}:{password}@{host}:{port}" f"/projects/{project}/instances/{instance}/" - "databases/compliance-test" + f"databases/{database}" ) else: url = ( f"spanner+spanner:///projects/{project}/instances/{instance}/" - "databases/compliance-test" + f"databases/{database}" ) config.add_section("db") config["db"]["default"] = url @@ -41,17 +41,18 @@ def set_test_config(project, instance, user=None, password=None, host=None, port def main(argv): project = argv[0] instance = argv[1] - if len(argv) == 6: - user = argv[2] - password = argv[3] - host = argv[4] - port = argv[5] + database = argv[2] + if len(argv) == 7: + user = argv[3] + password = argv[4] + host = argv[5] + port = argv[6] else: user = None password = None host = None port = None - set_test_config(project, instance, user, password, host, port) + set_test_config(project, instance, database, user, password, host, port) if __name__ == "__main__": diff --git a/create_test_database.py b/create_test_database.py index 1e29d44f..d8b82045 100644 --- a/create_test_database.py +++ b/create_test_database.py @@ -14,14 +14,15 @@ # See the License for the specific language governing permissions and # limitations under the License. -import configparser import os import time from create_test_config import set_test_config +from google.api_core import datetime_helpers from google.api_core.exceptions import AlreadyExists, ResourceExhausted from google.cloud.spanner_v1 import Client from google.cloud.spanner_v1.instance import Instance +from google.cloud.spanner_v1.database import Database USE_EMULATOR = os.getenv("SPANNER_EMULATOR_HOST") is not None @@ -66,43 +67,59 @@ def delete_stale_test_instances(): ) -def create_test_instance(): - configs = list(CLIENT.list_instance_configs()) - if not USE_EMULATOR: - # Filter out non "us" locations - configs = [config for config in configs if "asia-southeast1" in config.name] +def delete_stale_test_databases(): + """Delete test databases that are older than four hours.""" + cutoff = (int(time.time()) - 4 * 60 * 60) * 1000 + instance = CLIENT.instance("sqlalchemy-dialect-test") + if not instance.exists(): + return + database_pbs = instance.list_databases() + for database_pb in database_pbs: + database = Database.from_pb(database_pb, instance) + create_time = datetime_helpers.to_milliseconds(database_pb.create_time) + if create_time > cutoff: + continue + try: + database.drop() + except ResourceExhausted: + print( + "Unable to drop stale database '{}'. May need manual delete.".format( + database.database_id + ) + ) - instance_config = configs[0].name - create_time = str(int(time.time())) - unique_resource_id = "%s%d" % ("-", 1000 * time.time()) - instance_id = ( - "sqlalchemy-dialect-test" - if USE_EMULATOR - else "sqlalchemy-test" + unique_resource_id - ) - labels = {"python-spanner-sqlalchemy-systest": "true", "created": create_time} - instance = CLIENT.instance(instance_id, instance_config, labels=labels) +def create_test_instance(): + instance_id = "sqlalchemy-dialect-test" + instance = CLIENT.instance(instance_id) + if not instance.exists(): + instance_config = f"projects/{PROJECT}/instanceConfigs/regional-us-east1" + if USE_EMULATOR: + configs = list(CLIENT.list_instance_configs()) + instance_config = configs[0].name + create_time = str(int(time.time())) + labels = {"python-spanner-sqlalchemy-systest": "true", "created": create_time} + + instance = CLIENT.instance(instance_id, instance_config, labels=labels) - try: - created_op = instance.create() - created_op.result(1800) # block until completion - except AlreadyExists: - pass # instance was already created + try: + created_op = instance.create() + created_op.result(1800) # block until completion + except AlreadyExists: + pass # instance was already created - if USE_EMULATOR: - database = instance.database("compliance-test") - database.drop() + unique_resource_id = "%s%d" % ("-", 1000 * time.time()) + database_id = "sqlalchemy-test" + unique_resource_id try: - database = instance.database("compliance-test") + database = instance.database(database_id) created_op = database.create() created_op.result(1800) except AlreadyExists: - pass # instance was already created + pass # database was already created - set_test_config(PROJECT, instance_id) + set_test_config(PROJECT, instance_id, database_id) -delete_stale_test_instances() +delete_stale_test_databases() create_test_instance() diff --git a/noxfile.py b/noxfile.py index 9244b757..0795fd1d 100644 --- a/noxfile.py +++ b/noxfile.py @@ -293,7 +293,7 @@ def unit(session): session.install("opentelemetry-api==1.27.0") session.install("opentelemetry-sdk==1.27.0") session.install("opentelemetry-instrumentation==0.48b0") - session.run("python", "create_test_config.py", "my-project", "my-instance") + session.run("python", "create_test_config.py", "my-project", "my-instance", "my-database") session.run("py.test", "--quiet", os.path.join("test/unit"), *session.posargs) @@ -311,6 +311,7 @@ def mockserver(session): "create_test_config.py", "my-project", "my-instance", + "my-database", "none", "AnonymousCredentials", "localhost", From dcb4e2be3e52e1710a6993bce4f5c77258869328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 15:57:09 +0100 Subject: [PATCH 05/20] chore: format code --- noxfile.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/noxfile.py b/noxfile.py index 0795fd1d..0e701e3b 100644 --- a/noxfile.py +++ b/noxfile.py @@ -293,7 +293,9 @@ def unit(session): session.install("opentelemetry-api==1.27.0") session.install("opentelemetry-sdk==1.27.0") session.install("opentelemetry-instrumentation==0.48b0") - session.run("python", "create_test_config.py", "my-project", "my-instance", "my-database") + session.run( + "python", "create_test_config.py", "my-project", "my-instance", "my-database" + ) session.run("py.test", "--quiet", os.path.join("test/unit"), *session.posargs) From ac2e34ba8357650cd310260cc3ef5dda381c932d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 16:01:59 +0100 Subject: [PATCH 06/20] fix: do not use static fallback config --- noxfile.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/noxfile.py b/noxfile.py index 0e701e3b..30b3a7c1 100644 --- a/noxfile.py +++ b/noxfile.py @@ -356,21 +356,12 @@ def _migration_test(session): session.run("python", "create_test_database.py") - project = os.getenv( - "GOOGLE_CLOUD_PROJECT", - os.getenv("PROJECT_ID", "emulator-test-project"), - ) - db_url = ( - f"spanner+spanner:///projects/{project}/instances/" - "sqlalchemy-dialect-test/databases/compliance-test" - ) - config = configparser.ConfigParser() if os.path.exists("test.cfg"): config.read("test.cfg") else: config.read("setup.cfg") - db_url = config.get("db", "default", fallback=db_url) + db_url = config.get("db", "default") session.run("alembic", "init", "test_migration") From bcc889396d691eb50f93aa1e48def1dd07d4c594 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 16:07:38 +0100 Subject: [PATCH 07/20] fix: cleanup job --- migration_test_cleanup.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/migration_test_cleanup.py b/migration_test_cleanup.py index 62266359..dafde585 100644 --- a/migration_test_cleanup.py +++ b/migration_test_cleanup.py @@ -25,10 +25,11 @@ def main(argv): project = re.findall(r"projects(.*?)instances", db_url) instance_id = re.findall(r"instances(.*?)databases", db_url) + database_id = re.findall(r"databases(.*?)", db_url) client = spanner.Client(project="".join(project).replace("/", "")) instance = client.instance(instance_id="".join(instance_id).replace("/", "")) - database = instance.database("compliance-test") + database = instance.database("".join(database_id).replace("/", "")) database.update_ddl(["DROP TABLE account", "DROP TABLE alembic_version"]).result(120) From 3fe29531d62c4a13820ee83b80217712a6fb514e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 31 Oct 2024 16:16:10 +0100 Subject: [PATCH 08/20] fix: search until end of string --- migration_test_cleanup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration_test_cleanup.py b/migration_test_cleanup.py index dafde585..c56b10d0 100644 --- a/migration_test_cleanup.py +++ b/migration_test_cleanup.py @@ -25,7 +25,7 @@ def main(argv): project = re.findall(r"projects(.*?)instances", db_url) instance_id = re.findall(r"instances(.*?)databases", db_url) - database_id = re.findall(r"databases(.*?)", db_url) + database_id = re.findall(r"databases(.*?)$", db_url) client = spanner.Client(project="".join(project).replace("/", "")) instance = client.instance(instance_id="".join(instance_id).replace("/", "")) From 6f6a2641f7f74250b5a710871f698e7e7cc25648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 1 Nov 2024 15:17:44 +0100 Subject: [PATCH 09/20] build: only run system tests on the emulator for presubmits --- .kokoro/presubmit/compliance.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.kokoro/presubmit/compliance.cfg b/.kokoro/presubmit/compliance.cfg index 6e4f768f..1261d8a0 100644 --- a/.kokoro/presubmit/compliance.cfg +++ b/.kokoro/presubmit/compliance.cfg @@ -3,5 +3,5 @@ # Only run this nox session. env_vars: { key: "NOX_SESSION" - value: "system" + value: "unit" } From a182f86f912694b9ce75822ba4c802e896a12ba8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 1 Nov 2024 16:23:24 +0100 Subject: [PATCH 10/20] build: skip system tests when skipping conformance tests --- noxfile.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/noxfile.py b/noxfile.py index 30b3a7c1..569822f0 100644 --- a/noxfile.py +++ b/noxfile.py @@ -264,6 +264,11 @@ def system(session): "Credentials or emulator host must be set via environment variable" ) + if os.environ.get("RUN_COMPLIANCE_TESTS", "true") == "false" and not os.environ.get( + "SPANNER_EMULATOR_HOST", "" + ): + session.skip("RUN_COMPLIANCE_TESTS is set to false, skipping") + session.install( "pytest", "pytest-cov", From aa6ce6e5c5889bb2a455835a09ed5bbd0fbc84b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 1 Nov 2024 19:00:00 +0100 Subject: [PATCH 11/20] test: run tests with Python 3.8 --- .github/workflows/test_suite.yml | 2 +- noxfile.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test_suite.yml b/.github/workflows/test_suite.yml index 0f219787..40238b93 100644 --- a/.github/workflows/test_suite.yml +++ b/.github/workflows/test_suite.yml @@ -141,7 +141,7 @@ jobs: - name: Setup Python uses: actions/setup-python@v4 with: - python-version: 3.12 + python-version: 3.8 - name: Install nox run: python -m pip install nox - name: Run System Tests diff --git a/noxfile.py b/noxfile.py index 569822f0..98e0ada3 100644 --- a/noxfile.py +++ b/noxfile.py @@ -252,7 +252,7 @@ def compliance_test_20(session): ) -@nox.session() +@nox.session(python=DEFAULT_PYTHON_VERSION) def system(session): """Run SQLAlchemy dialect system test suite.""" From daddd1dbfae071d6363fe82078c8faffd123e1a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 1 Nov 2024 19:19:45 +0100 Subject: [PATCH 12/20] test: try this --- .github/workflows/test_suite.yml | 2 +- .kokoro/build.sh | 2 +- noxfile.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test_suite.yml b/.github/workflows/test_suite.yml index 40238b93..0f219787 100644 --- a/.github/workflows/test_suite.yml +++ b/.github/workflows/test_suite.yml @@ -141,7 +141,7 @@ jobs: - name: Setup Python uses: actions/setup-python@v4 with: - python-version: 3.8 + python-version: 3.12 - name: Install nox run: python -m pip install nox - name: Run System Tests diff --git a/.kokoro/build.sh b/.kokoro/build.sh index 2c87f749..c69a085c 100755 --- a/.kokoro/build.sh +++ b/.kokoro/build.sh @@ -44,7 +44,7 @@ fi if [[ -n "${NOX_SESSION:-}" ]]; then python3 -m nox -s ${NOX_SESSION:-} elif [[ "${RUN_COMPLIANCE_TESTS}" -eq "false" ]]; then - python3 -m nox -s unit + python3 -m nox -s system else python3 -m nox fi diff --git a/noxfile.py b/noxfile.py index 98e0ada3..569822f0 100644 --- a/noxfile.py +++ b/noxfile.py @@ -252,7 +252,7 @@ def compliance_test_20(session): ) -@nox.session(python=DEFAULT_PYTHON_VERSION) +@nox.session() def system(session): """Run SQLAlchemy dialect system test suite.""" From 383f1a08908d2077546fd872bf90775378c92105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 1 Nov 2024 19:24:11 +0100 Subject: [PATCH 13/20] test: no tests --- .kokoro/build.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.kokoro/build.sh b/.kokoro/build.sh index c69a085c..a4ae61ef 100755 --- a/.kokoro/build.sh +++ b/.kokoro/build.sh @@ -44,7 +44,8 @@ fi if [[ -n "${NOX_SESSION:-}" ]]; then python3 -m nox -s ${NOX_SESSION:-} elif [[ "${RUN_COMPLIANCE_TESTS}" -eq "false" ]]; then - python3 -m nox -s system + echo "not running any tests" + # python3 -m nox -s system else python3 -m nox fi From 88e0b3c87e8a7edf5e5347c8baf33c4bda7d014c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 1 Nov 2024 19:26:55 +0100 Subject: [PATCH 14/20] fix: support storing columns for indices --- create_test_database.py | 2 ++ .../sqlalchemy_spanner/sqlalchemy_spanner.py | 16 ++++++++++++++ test/mockserver_tests/test_basics.py | 8 +++++++ test/system/test_basics.py | 22 +++++++++++++++++-- 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/create_test_database.py b/create_test_database.py index d8b82045..3fffb92d 100644 --- a/create_test_database.py +++ b/create_test_database.py @@ -76,6 +76,8 @@ def delete_stale_test_databases(): database_pbs = instance.list_databases() for database_pb in database_pbs: database = Database.from_pb(database_pb, instance) + if database.create_time is None: + continue create_time = datetime_helpers.to_milliseconds(database_pb.create_time) if create_time > cutoff: continue diff --git a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py index 6754931a..1608d192 100644 --- a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py +++ b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py @@ -492,6 +492,22 @@ def post_create_table(self, table): return post_cmds + def visit_create_index( + self, create, include_schema=False, include_table_schema=True, **kw + ): + text = super().visit_create_index(create, include_schema, include_table_schema, **kw) + index = create.element + storing = index.dialect_options["spanner"]["storing"] + if storing: + storing_columns = [ + index.table.c[col] if isinstance(col, str) else col + for col in storing + ] + text += " STORING (%s)" % ", ".join( + [self.preparer.quote(c.name) for c in storing_columns] + ) + return text + def get_identity_options(self, identity_options): text = ["sequence_kind = 'bit_reversed_positive'"] if identity_options.start is not None: diff --git a/test/mockserver_tests/test_basics.py b/test/mockserver_tests/test_basics.py index b6c916c4..e1a86165 100644 --- a/test/mockserver_tests/test_basics.py +++ b/test/mockserver_tests/test_basics.py @@ -127,3 +127,11 @@ def test_create_multiple_tables(self): "\n) PRIMARY KEY (id)", requests[0].statements[i], ) + + def test_reflect(self): + engine = create_engine( + "spanner:///projects/p/instances/i/databases/d", + connect_args={"client": self.client, "pool": FixedSizePool(size=10)}, + ) + metadata = MetaData() + metadata.reflect(engine, views=True) diff --git a/test/system/test_basics.py b/test/system/test_basics.py index 6a53b234..79dc517a 100644 --- a/test/system/test_basics.py +++ b/test/system/test_basics.py @@ -11,21 +11,29 @@ # 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. -from sqlalchemy import text, Table, Column, Integer, PrimaryKeyConstraint, String +from sqlalchemy import text, Table, Column, Integer, PrimaryKeyConstraint, \ + String, Index, inspect, MetaData from sqlalchemy.testing import eq_ from sqlalchemy.testing.plugin.plugin_base import fixtures +from sqlalchemy.testing.suite.test_reflection import metadata class TestBasics(fixtures.TablesTest): @classmethod def define_tables(cls, metadata): - Table( + numbers = Table( "numbers", metadata, Column("number", Integer), Column("name", String(20)), + Column("alternative_name", String(20)), PrimaryKeyConstraint("number"), ) + Index( + "idx_numbers_name", + numbers.c.name, + spanner_storing=[numbers.c.alternative_name], + ) def test_hello_world(self, connection): greeting = connection.execute(text("select 'Hello World'")) @@ -37,3 +45,13 @@ def test_insert_number(self, connection): ) name = connection.execute(text("select name from numbers where number=1")) eq_("One", name.fetchone()[0]) + + def test_reflect(self, connection): + engine = connection.engine + meta: MetaData = MetaData() + meta.reflect(bind=engine) + eq_(1, len(meta.tables)) + table = meta.tables["numbers"] + eq_(1, len(table.indexes)) + index = meta.tables["numbers"].index("idx_numbers_name") + From fe899d35c6398b8d91bb8d72755900894dfa0840 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 4 Nov 2024 13:43:29 +0100 Subject: [PATCH 15/20] fix: split key and non-key columns --- .../sqlalchemy_spanner/sqlalchemy_spanner.py | 45 ++++++++++++++----- test/system/test_basics.py | 29 +++++++++--- 2 files changed, 58 insertions(+), 16 deletions(-) diff --git a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py index 1608d192..7bc97b6e 100644 --- a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py +++ b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py @@ -495,13 +495,14 @@ def post_create_table(self, table): def visit_create_index( self, create, include_schema=False, include_table_schema=True, **kw ): - text = super().visit_create_index(create, include_schema, include_table_schema, **kw) + text = super().visit_create_index( + create, include_schema, include_table_schema, **kw + ) index = create.element storing = index.dialect_options["spanner"]["storing"] if storing: storing_columns = [ - index.table.c[col] if isinstance(col, str) else col - for col in storing + index.table.c[col] if isinstance(col, str) else col for col in storing ] text += " STORING (%s)" % ", ".join( [self.preparer.quote(c.name) for c in storing_columns] @@ -1013,15 +1014,35 @@ def get_multi_indexes( i.table_schema, i.table_name, i.index_name, - ARRAY_AGG(ic.column_name), + ( + SELECT ARRAY_AGG(ic.column_name) + FROM information_schema.index_columns ic + WHERE ic.index_name = i.index_name + AND ic.table_catalog = i.table_catalog + AND ic.table_schema = i.table_schema + AND ic.table_name = i.table_name + AND ic.column_ordering is not null + ) as columns, i.is_unique, - ARRAY_AGG(ic.column_ordering) + ( + SELECT ARRAY_AGG(ic.column_ordering) + FROM information_schema.index_columns ic + WHERE ic.index_name = i.index_name + AND ic.table_catalog = i.table_catalog + AND ic.table_schema = i.table_schema + AND ic.table_name = i.table_name + AND ic.column_ordering is not null + ) as column_orderings, + ( + SELECT ARRAY_AGG(storing.column_name) + FROM information_schema.index_columns storing + WHERE storing.index_name = i.index_name + AND storing.table_catalog = i.table_catalog + AND storing.table_schema = i.table_schema + AND storing.table_name = i.table_name + AND storing.column_ordering is null + ) as storing_columns, FROM information_schema.indexes as i - JOIN information_schema.index_columns AS ic - ON ic.index_name = i.index_name - AND ic.table_catalog = i.table_catalog - AND ic.table_schema = i.table_schema - AND ic.table_name = i.table_name JOIN information_schema.tables AS t ON i.table_catalog = t.table_catalog AND i.table_schema = t.table_schema @@ -1032,7 +1053,7 @@ def get_multi_indexes( {schema_filter_query} i.index_type != 'PRIMARY_KEY' AND i.spanner_is_managed = FALSE - GROUP BY i.table_schema, i.table_name, i.index_name, i.is_unique + GROUP BY i.table_catalog, i.table_schema, i.table_name, i.index_name, i.is_unique ORDER BY i.index_name """.format( table_filter_query=table_filter_query, @@ -1052,6 +1073,8 @@ def get_multi_indexes( "column_sorting": { col: order for col, order in zip(row[3], row[5]) }, + "include_columns": row[6], + "dialect_options": {"spanner_storing": row[6]}, } row[0] = row[0] or None table_info = result_dict.get((row[0], row[1]), []) diff --git a/test/system/test_basics.py b/test/system/test_basics.py index 79dc517a..5d397cce 100644 --- a/test/system/test_basics.py +++ b/test/system/test_basics.py @@ -11,8 +11,18 @@ # 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. -from sqlalchemy import text, Table, Column, Integer, PrimaryKeyConstraint, \ - String, Index, inspect, MetaData +from sqlalchemy import ( + text, + Table, + Column, + Integer, + PrimaryKeyConstraint, + String, + Index, + inspect, + MetaData, + Boolean, +) from sqlalchemy.testing import eq_ from sqlalchemy.testing.plugin.plugin_base import fixtures from sqlalchemy.testing.suite.test_reflection import metadata @@ -27,11 +37,13 @@ def define_tables(cls, metadata): Column("number", Integer), Column("name", String(20)), Column("alternative_name", String(20)), + Column("prime", Boolean), PrimaryKeyConstraint("number"), ) Index( "idx_numbers_name", numbers.c.name, + numbers.c.prime, spanner_storing=[numbers.c.alternative_name], ) @@ -41,7 +53,9 @@ def test_hello_world(self, connection): def test_insert_number(self, connection): connection.execute( - text("insert or update into numbers(number, name) values (1, 'One')") + text( + "insert or update into numbers(number, name, prime) values (1, 'One', false)" + ) ) name = connection.execute(text("select name from numbers where number=1")) eq_("One", name.fetchone()[0]) @@ -53,5 +67,10 @@ def test_reflect(self, connection): eq_(1, len(meta.tables)) table = meta.tables["numbers"] eq_(1, len(table.indexes)) - index = meta.tables["numbers"].index("idx_numbers_name") - + index = next(iter(table.indexes)) + eq_(2, len(index.columns)) + eq_("name", index.columns[0].name) + eq_("prime", index.columns[1].name) + dialect_options = index.dialect_options["spanner"] + eq_(1, len(dialect_options["storing"])) + eq_("alternative_name", dialect_options["storing"][0]) From c6c4a48572b8c179a9273ab709f330f566572b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 4 Nov 2024 13:46:03 +0100 Subject: [PATCH 16/20] build: run system tests on real Spanner --- .kokoro/build.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/.kokoro/build.sh b/.kokoro/build.sh index 54514682..0985afd0 100755 --- a/.kokoro/build.sh +++ b/.kokoro/build.sh @@ -45,4 +45,5 @@ if [[ -n "${NOX_SESSION:-}" ]]; then python3 -m nox -s ${NOX_SESSION:-} else python3 -m nox -s unit + python3 -m nox -s system fi From 64a9f097525945a2bfbd1f425868f4668eee7012 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 4 Nov 2024 14:09:08 +0100 Subject: [PATCH 17/20] fix: add include_columns as none --- .../sqlalchemy_spanner/sqlalchemy_spanner.py | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py index 7bc97b6e..20fac18e 100644 --- a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py +++ b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py @@ -499,14 +499,17 @@ def visit_create_index( create, include_schema, include_table_schema, **kw ) index = create.element - storing = index.dialect_options["spanner"]["storing"] - if storing: - storing_columns = [ - index.table.c[col] if isinstance(col, str) else col for col in storing - ] - text += " STORING (%s)" % ", ".join( - [self.preparer.quote(c.name) for c in storing_columns] - ) + if "spanner" in index.dialect_options: + options = index.dialect_options["spanner"] + if "storing" in options: + storing = options["storing"] + storing_columns = [ + index.table.c[col] if isinstance(col, str) else col + for col in storing + ] + text += " STORING (%s)" % ", ".join( + [self.preparer.quote(c.name) for c in storing_columns] + ) return text def get_identity_options(self, identity_options): @@ -1066,6 +1069,10 @@ def get_multi_indexes( result_dict = {} for row in rows: + dialect_options = {} + include_columns = row[6] + if include_columns: + dialect_options["spanner_storing"] = include_columns index_info = { "name": row[2], "column_names": row[3], @@ -1073,8 +1080,8 @@ def get_multi_indexes( "column_sorting": { col: order for col, order in zip(row[3], row[5]) }, - "include_columns": row[6], - "dialect_options": {"spanner_storing": row[6]}, + "include_columns": include_columns if include_columns else [], + "dialect_options": dialect_options, } row[0] = row[0] or None table_info = result_dict.get((row[0], row[1]), []) From d0bf690efb6e2934c0dd49bf580b41c086a54b12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 4 Nov 2024 14:15:35 +0100 Subject: [PATCH 18/20] fix: remove unused test --- google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py | 3 ++- test/mockserver_tests/test_basics.py | 8 -------- test/system/test_basics.py | 5 ++--- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py index 20fac18e..9ba0f922 100644 --- a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py +++ b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py @@ -1056,7 +1056,8 @@ def get_multi_indexes( {schema_filter_query} i.index_type != 'PRIMARY_KEY' AND i.spanner_is_managed = FALSE - GROUP BY i.table_catalog, i.table_schema, i.table_name, i.index_name, i.is_unique + GROUP BY i.table_catalog, i.table_schema, i.table_name, + i.index_name, i.is_unique ORDER BY i.index_name """.format( table_filter_query=table_filter_query, diff --git a/test/mockserver_tests/test_basics.py b/test/mockserver_tests/test_basics.py index e1a86165..b6c916c4 100644 --- a/test/mockserver_tests/test_basics.py +++ b/test/mockserver_tests/test_basics.py @@ -127,11 +127,3 @@ def test_create_multiple_tables(self): "\n) PRIMARY KEY (id)", requests[0].statements[i], ) - - def test_reflect(self): - engine = create_engine( - "spanner:///projects/p/instances/i/databases/d", - connect_args={"client": self.client, "pool": FixedSizePool(size=10)}, - ) - metadata = MetaData() - metadata.reflect(engine, views=True) diff --git a/test/system/test_basics.py b/test/system/test_basics.py index 5d397cce..6900e1e9 100644 --- a/test/system/test_basics.py +++ b/test/system/test_basics.py @@ -19,13 +19,11 @@ PrimaryKeyConstraint, String, Index, - inspect, MetaData, Boolean, ) from sqlalchemy.testing import eq_ from sqlalchemy.testing.plugin.plugin_base import fixtures -from sqlalchemy.testing.suite.test_reflection import metadata class TestBasics(fixtures.TablesTest): @@ -54,7 +52,8 @@ def test_hello_world(self, connection): def test_insert_number(self, connection): connection.execute( text( - "insert or update into numbers(number, name, prime) values (1, 'One', false)" + """insert or update into numbers (number, name, prime) + values (1, 'One', false)""" ) ) name = connection.execute(text("select name from numbers where number=1")) From 2d8f04cf2cc40c4218bc0dccb0f8cf86df05215b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 4 Nov 2024 17:56:31 +0100 Subject: [PATCH 19/20] fix: use lower case for asc/desc --- google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py | 2 +- test/system/test_basics.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py index 9ba0f922..dcd1153d 100644 --- a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py +++ b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py @@ -1079,7 +1079,7 @@ def get_multi_indexes( "column_names": row[3], "unique": row[4], "column_sorting": { - col: order for col, order in zip(row[3], row[5]) + col: order.lower() for col, order in zip(row[3], row[5]) }, "include_columns": include_columns if include_columns else [], "dialect_options": dialect_options, diff --git a/test/system/test_basics.py b/test/system/test_basics.py index 6900e1e9..4245881b 100644 --- a/test/system/test_basics.py +++ b/test/system/test_basics.py @@ -41,7 +41,7 @@ def define_tables(cls, metadata): Index( "idx_numbers_name", numbers.c.name, - numbers.c.prime, + numbers.c.prime.desc(), spanner_storing=[numbers.c.alternative_name], ) From 0301abe7b17d4b4febcb6c55e6d5cf367952af06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 4 Nov 2024 18:03:46 +0100 Subject: [PATCH 20/20] test: expect lower case desc --- test/test_suite_20.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_suite_20.py b/test/test_suite_20.py index 4902b3ab..22b23e0a 100644 --- a/test/test_suite_20.py +++ b/test/test_suite_20.py @@ -1414,7 +1414,7 @@ def idx( "include_columns": [], } if column_sorting: - res["column_sorting"] = {"q": "DESC"} + res["column_sorting"] = {"q": "desc"} if duplicates: res["duplicates_constraint"] = name return [res] @@ -1458,11 +1458,11 @@ def idx( *idx( "q", name="noncol_idx_nopk", - column_sorting={"q": "DESC"}, + column_sorting={"q": "desc"}, ) ], (schema, "noncol_idx_test_pk"): [ - *idx("q", name="noncol_idx_pk", column_sorting={"q": "DESC"}) + *idx("q", name="noncol_idx_pk", column_sorting={"q": "desc"}) ], (schema, self.temp_table_name()): [ *idx("foo", name="user_tmp_ix"),