rust-db / refinery

Powerful SQL migration toolkit for Rust.
MIT License
1.35k stars 126 forks source link

Missing math functions for SQLite #333

Closed Pamplemousse closed 6 months ago

Pamplemousse commented 6 months ago

One of my migration file that uses the SQLite function FLOOR fails to be applied by refinery, with the following error:

$ refinery migrate -c ./refinery.toml -p ./data/migrations/
current version: 1
applying migration: V2__update_spendings
Error: `error applying migration V2__update_spendings`, `no such function: FLOOR in 

[...]
  CAST(FLOOR(price) as INT),
[...]

Caused by:
    0: no such function: FLOOR in 

[...]
         CAST(FLOOR(price) as INT),
[...]
        at offset 103
    1: Error code 1: SQL error or missing database

The function is listed here https://www.sqlite.org/lang_mathfunc.html, which specifies that the -SQLITE_ENABLE_MATH_FUNCTIONS compilation option needs to be applied when building the library.

I suspect that the issue probably comes from a dependency, particularly rusqlite, and could be fixed by adding

[env]
LIBSQLITE3_FLAGS = { value = "-DSQLITE_ENABLE_MATH_FUNCTIONS", force=true }

in a .cargo/config.toml as proposed in https://github.com/rusqlite/rusqlite/issues/1211#issuecomment-1656027096 .


$ refinery --version
refinery_cli 0.8.10
jxs commented 6 months ago

Hi, and thanks for the interest. Huh, tbf I'd rather https://github.com/rusqlite/rusqlite/pull/1437 was merged, but it seems it hasn't had development. Have you tried forking refinery with that environment variable in the .cargo/config.toml and it works? If so feel free to provide a PR i'll review it.

Pamplemousse commented 6 months ago

It's unclear if that PR is ever gonna be accepted as per the comments in the issue linked above.

I am afraid if we need to support all of them behind feature flags... [...] If I considered all the points, I deduce that you will document how users can build rusqlite / libsqilte3-sys with any specific SQLite3 options without having to introduce another feature flag to avoid their proliferation.

Have you tried forking refinery with that environment variable in the .cargo/config.toml and it works? If so feel free to provide a PR i'll review it.

I did, and it seemed to work. It was unclear to me though how this should work for folks not using features = ["rusqlite"]. Is it OK to have .cargo/config.toml setting this env variable as it shouldn't impact them? :thinking:

jxs commented 6 months ago

yeah, feel free to submit a PR, the env variable is namedspaced enough