-
Notifications
You must be signed in to change notification settings - Fork 81
Add DBTables type #269
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
Add DBTables type #269
Conversation
Codecov Report
@@ Coverage Diff @@
## master #269 +/- ##
==========================================
+ Coverage 85.87% 86.01% +0.13%
==========================================
Files 5 5
Lines 701 708 +7
==========================================
+ Hits 602 609 +7
Misses 99 99
Continue to review full report at Codecov.
|
Ok, this is a good start in the right direction, @jeremiahpslewis! Thanks for taking this on. Part of the awkwardness though in the original issue was that there wasn't a well-defined schema for the list of tables, which isn't quite solved here (as far as I can tell). I'm thinking maybe we try something along the lines of: struct DBTables
tbls::Vector{String}
end
Tables.isrowtable(::Type{DBTables}) = true
Tables.rows(x::DBTables) = x
Tables.schema(x::DBTables) = Tables.Schema((:name,), (String,))
function Base.iterate(x::DBTables, st=1)
st > length(x.tbls) && return nothing
return (name=x.tbls[st],), st + 1
end
function tables(db::DB, sink=nothing)
tbls = DBInterface.execute(db, "SELECT name FROM sqlite_master WHERE type='table';")
if sink !== nothing
# for backwards compat
return tbls |> sink
end
x = columntable(tbls)
return DBTables(x == NamedTuple() ? String[] : x.name)
end in this way, since we're defining our own |
Thanks for the feedback. I think this may be already covered in the PR, but potentially not very clearly. The PR introduces the DBTables type here: Line 32 in b74ad81
|
There are a number of instances where tables are referred to as strings. If we move to a 'table object' with an optional schema, the question arises whether to support dispatch on strings and table objects (I suppose yes, for backward compatibility) as well as on table objects? E.g. here: Line 600 in b74ad81
|
But I do think a key step which this PR takes is introducing a DBTable type which tracks both name and (optionally) schema. Kind of wonder whether this is a generic which is useful in other DBs as well |
Ok, I better understand the design decision of defining |
Co-authored-by: Jacob Quinn <[email protected]>
Co-authored-by: Jacob Quinn <[email protected]>
Co-authored-by: Jacob Quinn <[email protected]>
@quinnj I think I understand your concerns and am hopeful this is already covered by the proposed changes. I extended the tests so that the way in which this PR solves the original issue is clearer, for both the case when tables are present in the db as well as when there are no tables; let me know :) |
@quinnj Do you have an update on this PR? Would love to tie up any loose ends. :) |
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.
Sorry for the slow review here; been tied up in other things. Thanks @jeremiahpslewis!
What is currently the point of |
So you can control what kind of "table" you get the results back in. i.e. DataFrame, CSV.write, |
@quinnj - this was the case before this PR and 1.4.1 release. Now this function always returns |
@quinnj as discussed.
Resolves #209.
Note: moved few tests from typeof -> isa, let me know if I should nix it :)
TODO: need to add a test for db without any tables