-
Notifications
You must be signed in to change notification settings - Fork 415
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
Update sql strings with sea_query sql statements #3959
Conversation
Thanks for working on this. Starting small with an example was the way to go. Good call! It looks good, but I wonder if it's more readable at the end of the day. Can you try to use |
Yup that sounds good to me. I will start working on updating them. |
@guilload I have completed about 95% of the updates for the |
Looking very good so far. You can keep going. Thank you! |
@guilload I have completed the migration for the two functions. Please do check the unit tests because I had to modify the strings due to some quirks in seaquery. |
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
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.
Looking good. The 'tag' = ANY(tags)
might be a deal breaker :/ Let's see what they say.
For testing:
make docker-compose-up DOCKER_SERVICES='postgres'
QW_TEST_DATABASE_URL=postgres://quickwit-dev:quickwit-dev@localhost:5432/quickwit-metastore-dev cargo test --manifest-path quickwit/Cargo.toml -p quickwit-metastore --lib --all-features -- --nocapture postgres
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
We can merge this once the conflict is solved. |
Description
This PR is a draft version to kickstart #3758. I used
sea_query
to update one sql query and I want to get the initial comments from @guilload on whether this is fine. If thesea_query
implementation seems good, then I will continue with converting the remaining sql strings into the sea query orm syntax. Otherwise, I will try to reach other options.How was this PR tested?
This PR was tested using
cargo test --features=postgres
.