FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix integer compliance tests by HemangChothani · Pull Request #20 · googleapis/python-spanner-sqlalchemy · GitHub

This repository was archived by the owner on May 14, 2026. It is now read-only.

test: fix integer compliance tests - #20

Merged
larkee merged 10 commits into
mainfrom
fix_integer_compliance_test
Mar 19, 2021
Merged

larkee merged 10 commits into
mainfrom
fix_integer_compliance_test

Conversation

Copy link
Copy Markdown
Contributor

No description provided.

HemangChothani requested a review from larkee March 8, 2021 11:30
google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 8, 2021
Comment thread test/test_suite.py
Comment thread test/test_suite.py Outdated
"""
SPANNER OVERRIDE:

Spanner is not able cleanup data and drop the table correctly,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do you know why Spanner is not cleaning up the data and dropping the table correctly? Could #28 potentially resolve this?

This override reason doesn't seem tied to a feature that Spanner doesn't support such as temporary tables or multiple rows in empty primary key tables so I think this may be a bug rather than something we should override.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Locally, I can get this test to pass without the override by changing the db_api implementation to execute DDL statements immediately instead of queuing them until a non-DDL statement is executed.

Please add a TODO to remove this override

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, because locally implemented both the PRs #28, googleapis/python-spanner#277, but still failing for me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The required change is shown below:

--- a/google/cloud/spanner_dbapi/cursor.py
+++ b/google/cloud/spanner_dbapi/cursor.py
@@ -174,14 +174,9 @@ class Cursor(object):
         try:
             classification = parse_utils.classify_stmt(sql)
             if classification == parse_utils.STMT_DDL:
-                self.connection._ddl_statements.append(sql)
+                self.connection.database.update_ddl([sql]).result()
                 return
 
-            # For every other operation, we've got to ensure that
-            # any prior DDL statements were run.
-            # self._run_prior_DDL_statements()
-            self.connection.run_prior_DDL_statements()
-
             if not self.connection.autocommit:
                 if classification == parse_utils.STMT_UPDATING:
                     sql = parse_utils.ensure_where_clause(sql)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The docs here and some other places do seem to lean towards batching the DDL statements since they might be quite time consuming. That was probably the original idea behind this implementation.
Perhaps we can offer a solution where we would execute DDL statement in autocommit=True mode and keep the current implementation in autocommit=False mode.
I think it will be intuitive from the user's perspective.

--- a/google/cloud/spanner_dbapi/cursor.py
+++ b/google/cloud/spanner_dbapi/cursor.py
        try:
             classification = parse_utils.classify_stmt(sql)
             if classification == parse_utils.STMT_DDL:
                 self.connection._ddl_statements.append(sql)
+                if self.connection.autocommit:
+                    self.connection.run_prior_DDL_statements()
                 return
 
            # For every other operation, we've got to ensure that
            # any prior DDL statements were run.
            # self._run_prior_DDL_statements()
            self.connection.run_prior_DDL_statements()

            if not self.connection.autocommit:
                if classification == parse_utils.STMT_UPDATING:
                    sql = parse_utils.ensure_where_clause(sql)

@larkee WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@larkee Yes, I was talking about autocommit=Flase. I think need to remove TODO note then.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Correct. Please remove it and update the override explanation to mention that DDL statements aren't executed until a non-DDL statement is executed 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@larkee Still this doesn't work for me with changes recommended by alex, am i missing something here?

def _literal_round_trip(self, type_, input_, output, filter_=None):
    t = Table("int_t", self.metadata, Column("x", type_))
        t.create()

        with db.connect() as conn:
            for value in input_:
                ins = (
                    t.insert()
                    .values(x=literal(value))
                    .compile(
                        dialect=db.dialect, compile_kwargs=dict(literal_binds=True),
                    )
                )
                conn.execute(ins) --> thorws an error
                conn.execute("SELECT 1")  # didn't execute
            if self.supports_whereclause:
                stmt = t.select().where(t.c.x == literal(value))
            else:
                stmt = t.select()

            stmt = stmt.compile(
                dialect=db.dialect, compile_kwargs=dict(literal_binds=True),
            )
            for row in conn.execute(stmt):
                value = row[0]
                if filter_ is not None:
                    value = filter_(value)
                assert value in output

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@larkee Test passes for the very first time when table doesn't exist and after that it failed every time with an error 400 Duplicate name in schema: int_t.
DROP TABLE is still not working properly after merging all the related branches (with current master branch of spanner and spaner-sqlalchemy). I think the culprit is autocommit=False because when set autocommit=True in spanner_dbapi everything works fine and table deleted successfully after every test.

Solution might be execute non-DDL statement after the DROP TABLE or commit manually.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Solution might be execute non-DDL statement after the DROP TABLE

This is what I would suggest but I'm not sure where DROP TABLE is called between tests. For now I think the current implementation is fine. We can revisit this another time 👍

Comment thread test/test_suite.py
Comment thread test/test_suite.py Outdated
"""
SPANNER OVERRIDE:

Spanner is not able cleanup data and drop the table correctly,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Solution might be execute non-DDL statement after the DROP TABLE

This is what I would suggest but I'm not sure where DROP TABLE is called between tests. For now I think the current implementation is fine. We can revisit this another time 👍

Comment thread test/test_suite.py
larkee changed the title fix: integer complaince tests test: fix integer compliance tests Mar 18, 2021
larkee merged commit 79695c2 into main Mar 19, 2021
IlyaFaer deleted the fix_integer_compliance_test branch November 17, 2021 10:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL