-
Notifications
You must be signed in to change notification settings - Fork 111
include synonyms in odbcListObjects()
output
#773
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
Draft
simonpcouch
wants to merge
13
commits into
main
Choose a base branch
from
fix-221
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
356c188
include synonyms in `odbcListObjects()` output
simonpcouch 98d9df5
note change in NEWS
simonpcouch 30f592b
support synonyms in `dbListTables()`
simonpcouch 124c7b9
transition query to handle NULLs
simonpcouch e268bf8
support synonyms in `dbExistsTable()`
simonpcouch f12dd24
test support for `dbListTables()` and `dbExistsTable()`
simonpcouch 80a7b43
update verbiage in NEWS
simonpcouch 1a8170e
address review comments
simonpcouch da86cc4
restore table filter edge case
simonpcouch 503f45f
correct test for appropriate object type
simonpcouch 779f0b2
only return synonyms when listing tables
simonpcouch 1cbc62d
fix bug when a schema consists only of synonyms
simonpcouch 79b2c8d
simplify `synonyms_query()`
simonpcouch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 wonder if it's worth checking to see if this works for synonyms too? (without the
tempdb..
). If so, that would substantially simplify this function, and maybe make it simpler?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.
@simonpcouch Did you see this comment?
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! Just haven't made a moment to come back to this PR to address this comment and run some quick benchmarks to see how it affects performance. Will re-request review then, likely tomorrow!
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.
Oh sorry, I though you had re-requested a review, but I must've been misreading my notifications.
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.
Was almost able to make this work with:
The only issue is that
OBJECT_ID()
seems to only be able to work with fully qualified names. i.e.Created on 2024-04-10 with reprex v2.1.0
Specifically, this triggered this DBItest test.
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.
Hmmmm, is
dbListTables()
correct here? i.e.SELECT * FROM test2
won't work (IIUC) because it's not in the current schema. So should it actually be listed?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.
:thinkies: The DBI docs for the generic read:
so... I think it's correct that
dbListTables()
would listtest2
.Some statements:
dbListTables()
,dbExistsTable()
should returnTRUE
.SELECT * FROM foo
works thendbExistsTable(con, "foo")
should return TRUE?So
dbListTables()
anddbExistsTable()
should agree, anddbListTables()
seems to be doing the right thing. The connection toSELECT * FROM foo
doesn't seem to hold up, at least for SQL Server.FWIW, with this PR:
Created on 2024-04-10 with reprex v2.1.0
With CRAN, both
"test2"
calls areFALSE
.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'm pretty sure that the DBI docs are underspecified here — I think they really should read a "character vector that enumerates all tables and views in the database for the active schema/catalog.".
So maybe we're blowing out the scope of this PR, but I think we should narrow down this behaviour because I think that we should be returning
FALSE
for thetest
calls too.But might be easiest to discuss this live on a call with some examples?