-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add TiDB into the engine tests #3308
Conversation
@@ -215,7 +215,7 @@ async fn introspect_postgres_prisma2(api: &TestApi) -> TestResult { | |||
|
|||
//Mysql | |||
|
|||
#[test_connector(tags(Mysql))] | |||
#[test_connector(tags(Mysql), exclude(TiDB))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the problem with this test? Is there an issue on TiDB side tracking this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because TiDB currently does not support the point type, related issue: pingcap/tidb#6347
For this reason, I added an additional test case for TiDB:
prisma-engines/introspection-engine/introspection-engine-tests/tests/identify_version/mod.rs
Line 352 in 8177d21
t.inject_custom("location json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janpio when doing the prisma documentation probably this can be tagged as an exception ? Any other suggestions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not know yet. Right now a database either supports our feature set, or it does not. We did not have the case where it does, but not a specific type. Depending on the overall story and situation this might be be handled with a comment in the docs and may be a database specific error message - but if there are more things that is probably not enough and we would need to handle this on a higher level (new connector, which explodes our complexity).
introspection-engine/introspection-engine-tests/tests/rpc_calls/get_metadata_command_tests.rs
Outdated
Show resolved
Hide resolved
@@ -4,7 +4,7 @@ use introspection_engine_tests::test_api::*; | |||
use sql_introspection_connector::SqlIntrospectionConnector; | |||
use url::Url; | |||
|
|||
#[test_connector(tags(Mysql))] | |||
#[test_connector(tags(Mysql), exclude(TiDB))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not supported in TiDB? Does an issue exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there is a compatibility problem here, the name of the generated unique key in TiDB is not consistent with MySQL.
Expect:
----
model Test {
id Int @id
authorId Int @unique(map: "authorId") @default(autoincrement())
}
----
Actual:
----
model Test {
id Int @id
authorId Int @unique(map: "authorid") @default(autoincrement())
}
----
I have created an issue pingcap/tidb#39080, and push the enginers to take a look on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, probably related to TiDB bein case-insensitive for column names by default. Interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing. Do you have an issue or PR link? Just to connect the dots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢 We may miss this release cycle, this fix PR may have to wait until TiDB 6.5. Version 6.4 has been frozen earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shame. When will that be?
Is there a preview release or something? Happy to test with prerelease.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will publish the Nightly version in the Docker image.
The Nightly version can pass the test smoothly. But I'm not sure whether you can accept the Nightly version in the prisma test.
According to the previous release frequency, TiDB will release a mirror version every one to two months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can totally put the nightly image in the test here, as long as it is stable enough for us to learn what we want to learn - where Prisma and TiDB do not work together well right now.
Before merging this, we of course would love to switch to a different image then. (If we switch to nightly, I would open another PR that forks of the branch with the nightly to replace it with the current stable/minor so we can also monitor what current users of TiDB would experience)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please open another PR into this branch that replaces the docker container with the nightly version and removes this test file modification @Mini256?
introspection-engine/introspection-engine-tests/tests/tables/mysql.rs
Outdated
Show resolved
Hide resolved
@@ -779,7 +779,7 @@ fn set_default_current_timestamp_on_existing_column_works(api: TestApi) { | |||
} | |||
|
|||
// exclude: there is a cockroach-specific test. It's unexecutable there. | |||
#[test_connector(preview_features("referentialIntegrity"), exclude(CockroachDb))] | |||
#[test_connector(preview_features("referentialIntegrity"), exclude(CockroachDb, TiDB))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not work on TiDB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test result:
called `Result::unwrap()` on an `Err` value: ConnectorErrorImpl { user_facing_error: None, message:
Some("Unsupported modify column: this column has primary key flag"), source: None, context: SpanTrace [] }
Unsupported modify column: this column has primary key flag
Reason:
Does not support modifying the Reorg-Data types on the primary key columns but supports modifying the Meta-Only types.
Doc: https://docs.pingcap.com/tidb/dev/sql-statement-modify-column#mysql-compatibility
migration-engine/migration-engine-tests/tests/migrations/advisory_locking.rs
Outdated
Show resolved
Hide resolved
// Ignoring TiDB, unsupported drop primary key when the table's pkIsHandle is true. | ||
// TODO: ignore because not possible on cockroachdb. We would need a multi-step process there. | ||
#[test_connector(exclude(Vitess), exclude(CockroachDb))] | ||
#[test_connector(exclude(Vitess), exclude(CockroachDb, TiDB))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that mean exactly?
Is there an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some investigation and I learned that this problem is also caused by TIDB does not support deleting the primary key on the cluster index table.
However, for some historical reasons, if the primary key type is integer (INT
or BIGINT
) and the table is cluster index table. The pkIsHandle
flag of the table will be set to true
, which means that the table will use the primary key as the row handle.
If we try to drop a primary key of integer type on a clustered table, it gives us the following error:
(8200, "Unsupported drop primary key when the table's pkIsHandle is true")
If we drop a primary key that is not of type integer, it will throw this error:
(8200, 'Unsupported drop primary key when the table is using clustered index')
Our R&D engineers are considering optimizing this error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue for this work? Might be good to link there so we can notice when this is fixed one day.
// Ignoring TiDB, we can't remove auto_increment without @@tidb_allow_remove_auto_inc enabled. | ||
// We test this separately on cockroachdb. | ||
#[test_connector(exclude(Sqlite, CockroachDb))] | ||
#[test_connector(exclude(Sqlite, CockroachDb, TiDB))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a link for this config option?
Are there alternative strategies to achieve the same outcome?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link of this option: https://docs.pingcap.com/tidb/dev/system-variables#tidb_allow_remove_auto_inc-new-in-v2118-and-v304
🤔 Is there any way to enable this system variable before starting testing, so we can add an additional test case for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for other databases this can be configured either via environment variables (in our docker-compose), or we need to create a customized Docker image that executes some SQL on startup: https://github.com/prisma/engine-images/blob/main/cockroach/22.1.0/Dockerfile#L2 + https://github.com/prisma/engine-images/blob/main/cockroach/22.1.0/prisma_init.sql But that is suuuuper unflexible and error prone - so better to avoid.
I don't think we have a global hook that can be configured via the test suite unfortunately.
migration-engine/migration-engine-tests/tests/migrations/mysql.rs
Outdated
Show resolved
Hide resolved
@@ -58,7 +58,7 @@ fn reset_then_apply_with_migrations_directory_works(api: TestApi) { | |||
.assert_has_table("_prisma_migrations"); | |||
} | |||
|
|||
#[test_connector] | |||
#[test_connector(exclude(TiDB))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not work for TiDB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mini256 another one can you please respond ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to solve this problem, but I found that when this testcase executes to the line api.diagnose_migration_history(&dir).send_sync();
, in the absence of _prisma_migrations
tables in both TiDB and MySQL MySQL will not throw an error, while TiDB will cause the test to terminate due to the following error thrown.
called `Result::unwrap()` on an `Err` value: ConnectorErrorImpl { user_facing_error: None, message: Some("Schema change caused error: [schema:1146]Table 'reset_then_diagnostics_with_migrations_directory_works._prisma_migrations' doesn't exist"), source: None, context: SpanTrace [] }
Schema change caused error: [schema:1146]Table 'reset_then_diagnostics_with_migrations_directory_works._prisma_migrations' doesn't exist
@janpio, Do you have any idea about this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No details, but seems TiDB behaves a bit different which leads to an error where MySQL is fine. We'll need to figure out later if this is a problem on Prisma or TiDB side.
migration-engine/migration-engine-tests/tests/native_types/mysql.rs
Outdated
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-engine-tests/tests/new/interactive_tx.rs
Outdated
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-engine-tests/tests/new/interactive_tx.rs
Outdated
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-engine-tests/tests/new/occ.rs
Outdated
Show resolved
Hide resolved
...engine/connector-test-kit-rs/query-engine-tests/tests/new/ref_actions/on_delete/no_action.rs
Outdated
Show resolved
Hide resolved
...gine/connector-test-kit-rs/query-engine-tests/tests/new/ref_actions/on_delete/set_default.rs
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_10935.rs
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_7434.rs
Outdated
Show resolved
Hide resolved
@@ -42,7 +42,7 @@ mod aggr_group_by_having { | |||
_sum { int } | |||
} | |||
}"#, | |||
MongoDb(_) => vec![ | |||
MongoDb(_) | TiDB => vec![ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also related to the default order issue: pingcap/tidb#37285
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the linked issue was resolved, but unclear if the underlying problem is now gone as well.
…xclude making_an_existing_id_field_autoincrement_works test
(Nice work you are doing here right now @Mini256!) |
@janpio Thank you for your attention to the pull request. We hope to continue promoting this PR to merge, and after this PR is merged, our team will also continue to optimize the user experience of developers using Prisma with TiDB. In the next stage, we will promote our TiDB Serverless product so that developers can use HTAP capabilities to build their applications with just MySQL skills (Just like how we used TiDB to build OSSInsight). The code commit history may look a bit lengthy, so let me briefly summarize the recent changes I have made:
|
@janpio Updated the Docker image of TiDB to version 7.1.0. (Ready to merged) |
Thank you for submitting this PR, @Mini256! We mentioned in our documentation that TiDB has a community-maintained driver adapter. This driver adapter already allows users to effortlessly query TiDB databases. Thus, we currently do not plan to merge this PR and prisma/prisma#14631. |
// Exclude TiDB, TiDB does not guarantee the order of results returned when the `ORDER BY` | ||
// clause is not specified | ||
#[connector_test(exclude(TiDB))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue for this?
part of #14639