-
Notifications
You must be signed in to change notification settings - Fork 82
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(csharp): add more tests for verifying GetObjects (Depths and Patterns) #1299
test(csharp): add more tests for verifying GetObjects (Depths and Patterns) #1299
Conversation
Added tests to verify GetObjects for different depths like: Catalogs DbSchemas Tables All Also added tests to verify GetObjects with catalog, schema, and table names passed as patterns
|
||
string databaseName = testConfiguration.Metadata.Catalog; | ||
string schemaName = testConfiguration.Metadata.Schema; | ||
string partialDatabaseName = $"{GetPartialNameForPatternMatch(databaseName)}%"; |
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 should test a variety of wildcards and also test if case-sensitivity (or insensitivity) is handled correctly.
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, check that we handle embedded special characters like single-quote safely.
…and check that database object starts with the pattern name
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.
Thanks for adding the additional tests around GetObjects. The refactoring of the DriverTests is a lot cleaner as well.
…ters are provided (#1285) ## Replaced some cursor calls with static calls and filtered early when possible If a share isn't associated with a database then SELECT DATABASE_NAME FROM INFORMATION_SCHEMA.DATABASES call will not list it and therefore it doesn't seem necessary to call SHOW SHARES LIKE '%database_name%' to get a list and check if a DB isn't created for it. Therefore, those checks were removed. ### Comparison of performance improvements: I have created another PR: #1299 with additional tests for `GetObjects`. | Test | Before | After | % Improvement | |-----|---------------------|------------------|-------------------| | CanGetObjectsAll | 17.6 s | 3 s | 82.5 | | CanGetObjectsCatalogs | 503 ms | 333 ms | 33.80 | | CanGetObjectsCatalogsWithPattern | 421 ms | 369 ms | 12.35 | | CanGetObjectsDbSchemas | 4.4 s | 694 ms | 84.36 | | CanGetObjectsDbSchemasWithPattern | 4 s | 807 ms | 79.825 | | CanGetObjectsTables | 18 s | 2.8 s | 84.44 | | CanGetObjectsTablesWithPattern | 17.6 s | 2.9 s | 83.52 |
@jduo or @ryan-syed -- anything else needed here before merging? |
I think it should be good. Unable to add tests for case-sensitivity in patterns for GetObjects, as the spec is not clear about it. Also, as mentioned, the driver implementation uses |
Created an issue for case sensitivity: #1314 |
Added tests to verify GetObjects for different depths like: Catalogs
DbSchemas
Tables
All
Also added tests to verify GetObjects with catalog, schema, and table names passed as patterns