feat: implement UNIQUE constraints creation mechanism #24
Conversation
google-cla
bot
added
the
cla: yes
| "after_create", | ||
| DDL("create temporary view user_tmp_v as " "select * from user_tmp"), | ||
| ) | ||
| event.listen(user_tmp, "before_drop", DDL("drop view user_tmp_v")) |
There was a problem hiding this comment.
The method creates test tables. On line 244 instead of UniqueConstraint() an Index() is created. Everything else is the same.
| event.listen(user_tmp, "before_drop", DDL("drop view user_tmp_v")) | ||
|
|
||
| @testing.provide_metadata | ||
| def _test_get_unique_constraints(self, schema=None): |
There was a problem hiding this comment.
This test is not passing yet because of another error: one of the test columns is called asc, which causes syntax error. Without that column the test seems to be working fine, as unique constraints are replaced with unique indexes on the line 299, no other changes done.
|
The GA is showing 15 new failures. Some of these are clearly related to the insert bug. However, others look legitimate. Could you show the tests results when run locally with the bug fixes? |
|
@larkee, yeah, it looks like UPDATE: |
| metadata, | ||
| Column("id", Integer, primary_key=True), | ||
| Column("data", String(50)), | ||
| Column("parent_id", ForeignKey("some_table.id", name="fk_some_table")), |
There was a problem hiding this comment.
Explicitly setting a foreign key name
|
@larkee, I propose to override the test method, which defines tables for CTE tests. Originally it creates a foreign key without a name. Spanner gives a generated name to the constraint, but it's not reflected in the test case (as it's really small and other tests of the class are skipped - it doesn't see a reason to sync once again). So, there is no way to drop the constraint before dropping the table, as we don't have its name. As it's only a single test cleanup problem (the test itself is passing), I assume we better override testing data to explicitly set the FK name, instead of adding calls to backend into The strange thing is that in workflow a lot of tests are failing, while locally I have only expected failures, caused by the insert bug, and the failures, which are not yet processed (all the reflection tests are now executed because of import, but not all of them are processed in fact). Should we move the test workflow to kokoro?
|
SGTM 👍
Based on your screenshot, I suspect you are running the tests against prod. The workflow is running against the emulator. Unlike prod, the emulator cannot infer the type for null values. This is what is causing the additional failures. There was a fix being worked on in the DB API driver but it's not finished yet.
I am very unfamiliar with setting up a Kokoro workflow correctly so I'm hesitant to move away from GitHub Actions but I'll talk to the team about it. |
larkee
left a comment
larkee
left a comment
There was a problem hiding this comment.
LGTM.
Only questions are:
Should we override the asc column name since it's a reserved keyword?
Are there unique index specific tests? I just want to check that overriding the unique constraint tests makes the most sense.
I was going to take a look at keywords escaping system (I saw something like this in DB API). We can override this particular test any time, it's not gonna be a problem, but I assume there are more similar cases in tests (at least I've caught my eyes on one earlier), so I'd like to deal with the trouble overall (if it can be done overall).
Yes
No, no, I don't see any index-type-specific tests. They are just about indexes overall, like |
|
Merging to unblock further work |
3 participants
No description provided.