-
Notifications
You must be signed in to change notification settings - Fork 97
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
fix(go/adbc/driver/snowflake): Made GetObjects case insensitive #1328
fix(go/adbc/driver/snowflake): Made GetObjects case insensitive #1328
Conversation
…and added Skip capability for ValueTests TestConfiguration is now a part of SnowflakeTestingUtils, and can be used across the tests (no redundant reading the configuration) ValueTests were marked as Theory, changed them to SkippableTheory to skip the tests, if a XUnit.SkipException is thrown
conditions = append(conditions, ` CATALOG_NAME LIKE '`+*catalog+`'`) | ||
conditions = append(conditions, ` CATALOG_NAME ILIKE '`+*catalog+`'`) | ||
} | ||
if dbSchema != nil && *dbSchema != "" { | ||
conditions = append(conditions, ` SCHEMA_NAME LIKE '`+*dbSchema+`'`) | ||
conditions = append(conditions, ` SCHEMA_NAME ILIKE '`+*dbSchema+`'`) |
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.
would it be worthwhile adding tests in the Go side 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 was looking at the existing tests in driver_test.go
and they seemed like integration tests similar to what we have in DriverTest.cs
. I am not sure if adding the same kind of test would add as much value. Is there a specific test that you had in mind, that I can draw inspiration from?
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.
At a minimum perhaps checking that the query is the right text. I don't think that there's a test in driver_test.go
which verifies the case sensitivity though, which would be good to have.
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.
The tests all pass and the changes look good other than the Skip portions
|
||
public static IEnumerable<object[]> CatalogNamePatternData() | ||
{ | ||
Skip.IfNot(Utils.CanExecuteTestConfig(SnowflakeTestingUtils.SNOWFLAKE_TEST_CONFIG_VARIABLE)); |
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 would we skip here instead of just skipping the test that calls 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.
same with the other areas, including the SnowflakeTestingUtils class
Refactoring includes being able to match the exact catalog, schema, and table names when wild cards are involved in the name indentifiers of the GetObjects patterns For example: A pattern name like FOO_BAR will match against FOO1BAR and FOOABAR. Similarly FOO%BAR will match with foohellobar, FOObyeBAR etc.. Therefore it is essential to filter out the returned matches with the expected name identifiers. Similar checks would have to be added by authors using the Driver when calling the GetObjects API.
Refactoring includes being able to match the exact catalog, schema, and table names when wild cards are involved in the name indentifiers of the For example: A pattern name like Note: Similar checks would have to be added by authors using the Driver when calling the GetObjects API. |
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.
overall looks good, but i can't vouch for the csharp side
Description:
GetObjects
API was inconsistent case sensitivity for patterns.getObjectsDbSchemas
driver implementation usedLIKE
whereasgetObjectsTables
usedILIKE
Solution:
Based on discussion here: #1314 changed
GetObjects
API to useILIKE
throughout instead ofLIKE
Testing:
Added tests for lowercase, uppercase and
_
wildcardFixes #1314.