GenieFramework / SearchLight.jl

ORM layer for Genie.jl, the highly productive Julia web framework
https://genieframework.com
MIT License
139 stars 16 forks source link

Update to SQLite and MySQL v1 #27

Closed jeremiedb closed 4 years ago

jeremiedb commented 4 years ago

As these DB backends have move to v1, integrating with those updates would be welcome.

Breaking changes have been mentioned here: https://discourse.julialang.org/t/ann-dbinterface-jl-mysql-jl-1-0-and-sqlite-jl-1-0/34701

Most notably, the execute that replaces the Query. I've started looking at a quick PR for SearchLightSQLite here: https://github.com/jeremiedb/SearchLightSQLite.jl

But a first issue that arose while following the tutorial was in upped_migrations() after calling SearchLight.Migrations.status()

Whether using SQLite.execute of DBInterface.execute results in different outcome, not sure which would be the appropriate one for SearchLite:

julia> SQLite.execute(conn, "SELECT * FROM $(SearchLight.config.db_migrations_table_name)")
101

julia> SQLite.execute(conn, "SELECT * FROM $(SearchLight.config.db_migrations_table_name)") |> DataFrames.DataFrame
ERROR: ArgumentError: 'Int32' iterates 'Int32' values, which doesn't satisfy the Tables.jl `AbstractRow` interface
Stacktrace:
 [1] invalidtable(::Int32, ::Int32) at /home/jeremie/.julia/packages/Tables/okt7x/src/tofromdatavalues.jl:42
 [2] iterate at /home/jeremie/.julia/packages/Tables/okt7x/src/tofromdatavalues.jl:48 [inlined]
 [3] buildcolumns at /home/jeremie/.julia/packages/Tables/okt7x/src/fallbacks.jl:185 [inlined]
 [4] columns at /home/jeremie/.julia/packages/Tables/okt7x/src/fallbacks.jl:237 [inlined]
 [5] #DataFrame#453(::Bool, ::Type{DataFrame}, ::Int32) at /home/jeremie/.julia/packages/DataFrames/S3ZFo/src/other/tables.jl:40
 [6] DataFrame(::Int32) at /home/jeremie/.julia/packages/DataFrames/S3ZFo/src/other/tables.jl:31
 [7] |>(::Int32, ::Type) at ./operators.jl:854
 [8] top-level scope at none:0

julia> DBInterface.execute(conn, "SELECT * FROM $(SearchLight.config.db_migrations_table_name)")
SQLite.Query(SQLite.Stmt(SQLite.DB("db/books.sqlite"), Ptr{Nothing} @0x00007fb90dbfbcd8, Dict{Int64,Any}(), 101), Base.RefValue{Int32}(101), Symbol[:version], Type[Any], Dict(:version => 1))

julia> DBInterface.execute(conn, "SELECT * FROM $(SearchLight.config.db_migrations_table_name)") |> DataFrames.DataFrame
0×0 DataFrame

At first sight, I though SQLite.execute was the proper approach, though it return an Int32 which cannot be passed as a DataFrame. Maybe is it just a special case where the query return a single number? I was stucked at this point!

essenciary commented 4 years ago

@jeremiedb Thanks! Yes, that is the next point on the agenda. I just registered SearchLight 0.19 with deps updates and lots of API updates to make the models more Julian, improve performance, etc. Now I'm registering the 3 adapters at v0.2 to work on SearchLight 0.19. And finally, bring SQLite and MySQL at latest versions (LibPQ already is).

essenciary commented 4 years ago

@jeremiedb So now we have SearchLight at v0.19 with the latest dependencies (including YAML 0.4 so it's no longer incompatible with Genie). And we have the SQLite, MySQL and Postgres adapters at v0.2. All these bring a simplified API and better performance.

I'll try by the end of the week to see if I can upgrade SQLite and MySQL to v1 of the Julia libraries. Which will only leave writing some docs - so maybe next week if I manage to find some time. This would get us on track to v1 for SearchLight.

jeremiedb commented 4 years ago

Awesome, thanks for the great work!

essenciary commented 4 years ago

@jeremiedb All right, that wasn't as hard as I was afraid it would be! I have updated both packages and registered v0.3 for both.

Seem to be working great and the performance of the DB libraries looks massively improved!

jeremiedb commented 4 years ago

@essenciary Following the update to v0.3.0, the issue stemming from SearchLight.Migrations.status() is however still present.

Following steps described in https://genieframework.github.io/Genie.jl/guides/Working_With_Genie_Apps.html, the call results in the same type of error:

julia> SearchLight.Migrations.status()
ERROR: ArgumentError: column name :version not found in the data frame
Stacktrace:
 [1] getindex(::DataFrames.DataFrame, ::typeof(!), ::Symbol) at /home/jeremie/.julia/packages/DataFrames/S3ZFo/src/other/index.jl:250
 [2] upped_migrations() at /home/jeremie/.julia/packages/SearchLight/8LItr/src/Migration.jl:298
 [3] status() at /home/jeremie/.julia/packages/SearchLight/8LItr/src/Migration.jl:320
 [4] top-level scope at none:0

Same version not found in data frame error is raised with last_up() call as well.

Issue seems to come from when upped_migrations() is called, the following query doesn't returns a dataframe as expected:

result = SearchLight.query("SELECT version FROM $(SearchLight.config.db_migrations_table_name) ORDER BY version DESC", internal = true)

julia> SearchLight.query("SELECT version FROM $(SearchLight.config.db_migrations_table_name) ORDER BY version DESC", internal = true)
0×0 DataFrames.DataFrame

It seems like the SQLite.Query type returns by DBinterface.execute isn't appropriate for sinking into a DataFrame. Accessing the lookup object return by the query seems to work:

julia> res = DBInterface.execute(SearchLight.connection(), "SELECT * FROM $(SearchLight.config.db_migrations_table_name)").lookup |> DataFrame
1×1 DataFrame
│ Row │ version │
│     │ Int64   │
├─────┼─────────┤
│ 1   │ 1       │

The above doesn't seem in lined with the DBInterface documentation mentionning that an iterator should be returned. I'm really not clear on whether this might be a bug from DBInterface that improperly handle a special case, or the initial DB that may have been improperly initialized, or maybe a limitation in the conversion to DataFrame.

essenciary commented 4 years ago

@jeremiedb I tested the update with the 3 adapters and didn't have any issue with the migrations or execution of any queries. Can you please zip and attach the whole app so I can run it?

jeremiedb commented 4 years ago

@essenciary Sure, here is a minimal example. I started a new one from scratch following https://genieframework.github.io/Genie.jl/guides/Working_With_Genie_Apps.html

First place where I got a different behavior was at: SearchLight.Generator.newmodel("Book") which only generated app/resources/books/Books.jl bit not the migration, validation or test files.

So I used SearchLight.Generator.newmigration() to generate a migration file. Then I hit the wall with SearchLight.Migrations.status() resulting in the mentioned error.

AppTest.zip

essenciary commented 4 years ago

Ah, sorry about that - that's actually an error in the documentation. The right code should be SearchLight.Generator.newresource("Book"). The newmodel method only creates the model - it's the newresource which creates all the files. I pushed an update to the docs.

Please try it and let me know how it works.

jeremiedb commented 4 years ago

I'm afraid I created a distraction regarding the newmodel vs newresource. The issue is encountered regardless if newmigration or newresource was used and appears more tied to SearchLight.Migrations.status() which calls upped_migrations(). Attached is the App where newresource was used.

Once in app, the sequence I use is:

(v1.3) pkg> activate .
julia> using Genie
julia> using SearchLight
julia> using SearchLightSQLite
julia> Genie.loadapp()
julia> SearchLight.Configuration.load() |> SearchLight.connect
julia> SearchLight.Migrations.status()
ERROR: ArgumentError: column name :version not found in the data frame

AppTest.zip

essenciary commented 4 years ago

@jeremiedb You don't need to load all the dependencies by hand and I think that confuses Genie. Best way to load the app is running the correct repl script within the app's bin/ folder.

Otherwise, just do:

pkg> activate .
julia> using Genie
julia> Genie.loadapp()
julia> using SearchLight
julia> SearchLight.Migration.status()

When you call loadapp and you have config/initializers/searchlight.jl set up, that sets up SearchLight.

jeremiedb commented 4 years ago

Either with initialization with bin/repl or with the manual loadapp(), I still get the same error with SearchLight.Migration.status(): ERROR: ArgumentError: column name :version not found in the data frame. You didn't run through this issue with the attached AppTest? Do you have the same if running the following:

julia> SearchLight.query("SELECT version FROM $(SearchLight.config.db_migrations_table_name) ORDER BY version DESC", internal = true)
0×0 DataFrames.DataFrame
essenciary commented 4 years ago

@jeremiedb I did when running the things in the sequence you provided. But not when running them as per may comment above.

Are you in Genie's Gitter? Wanna have a quick chat to look at the code and compare outcome?

jeremiedb commented 4 years ago

Solved with https://github.com/GenieFramework/SearchLight.jl/releases/tag/v0.19.1