-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Sync index info: sqlserver and sqlite #36665
Sync index info: sqlserver and sqlite #36665
Conversation
Looks like there's a few sqlite tests failing, I'm not sure why though because they seem unrelated. |
1 flaky test on run #188 ↗︎
Details:
e2e/test/scenarios/native/native_subquery.cy.spec.js • 1 flaky test • flaky
Review all test suite changes for PR #36665 ↗︎ |
;; In sqlite a PK will implicitly have a UNIQUE INDEX, but if the PK is integer the getIndexInfo method from | ||
;; jdbc doesn't return it as indexed. so we need to manually get mark the pk as indexed here | ||
(cond-> ((get-method driver/describe-table-indexes :sql-jdbc) driver database table) | ||
(string? pk) |
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 string?
? What other type could pk
be?
(string? pk) | |
(string? pk) |
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.
nil
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.
Then ideally you should use some?
instead for readability. When I read this, I was questioning why such a specific check was made, and wondering whether the pk
could be something other than a string, and then wondering why we would only add pk
s that were strings and not other types. It doesn't help that get-table-pks
's docstring doesn't mention the return type.
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.
Looks good
Milestone 3 of #36452
These 2 drivers are JDBC drivers so it already works, this PR is mostly about making the tests work properly
After this we should be done syncing index info for all the drivers that support it