Skip to content
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

Batch insert suggestions #6189

Merged
merged 11 commits into from
Apr 6, 2023
Merged

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Apr 3, 2023

Pull Request Description

close #6080

Changelog

  • add: implement SuggestionsRepo.insertAll as a batch SQL insert
  • update: search/getSuggestionsDatabase returns empty suggestions. Currently, the method is only used at startup and returns the empty response anyway because the libs are not loaded at that point.
  • update: serialize only global (defined in the module scope) suggestions during the distribution building. There's no sense in storing the local library suggestions.
  • update: sqlite dependency
  • remove: unused methods from SuggestionsRepo
  • remove: Arguments table

Important Notes

Speeds up libraries loading by ~1 second.

2023-04-03-173423_2086x324_scrot
2023-04-03-173514_2083x321_scrot

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 3, 2023
@4e6 4e6 self-assigned this Apr 3, 2023
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved from the libs side.

@@ -8,6 +8,7 @@ logging-service.logger {
akka.event = error
akka.io = error
akka.stream = error
slick = error
slick.jdbc.JdbcBackend.statement = error # log SQL queries on debug level
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that comment doesn't seem to reflect the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to enable the debug logs for slick.jdbc.JdbcBackend.statement by default because it produces a lot of output. I think in general, we're not interested in SQL queries unless we're debugging some specific issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense now. Just from the comment I assumed this should have been = debug.

@@ -371,145 +371,7 @@ class SuggestionsHandlerEventsTest extends BaseServerTest with FlakySpec {
{ "jsonrpc" : "2.0",
"id" : 3,
"result" : {
"entries" : [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSuggestionDatabase now results in entries : [] being returned.

* @param hasDefault does the argument have the default value
* @param defaultValue optional default value
*/
case class ArgumentRow(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably removes removes the argument table.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Apr 6, 2023
@mergify mergify bot merged commit df4491d into develop Apr 6, 2023
@mergify mergify bot deleted the wip/db/6080-batch-insert-suggestions branch April 6, 2023 07:47
@radeusgd radeusgd mentioned this pull request Apr 6, 2023
5 tasks
Procrat added a commit that referenced this pull request Apr 6, 2023
* develop:
  Project Sharing (#6077)
  Adjust `{Table|Column}.parse` to use `Value_Type` (#6213)
  Add cloud endpoints for frontend (#6002)
  Implement `Table.union` for Database backend (#6204)
  Batch insert suggestions (#6189)
  Formatter fix to not fail when encountering an invalid symlink. (#6172)
  Suspended atom fields are evaluated only once (#6151)
  Text.to_display_text is (shortened) identity (#6174)
  Engine benchmark visualization tool can compare two bench runs (#6198)
  Add PRIVATE so function hidden from Component Browser and other tidying... (#6207)
  Hotfix for #6203. (#6210)
mergify bot pushed a commit that referenced this pull request Apr 6, 2023
Closes #5254

In #6189 the SQLite version was bumped to a newer release which has builtin support for Full and Right joins, so no workaround is no longer needed.
@enso-bot enso-bot bot mentioned this pull request Apr 6, 2023
7 tasks
MichaelMauderer pushed a commit that referenced this pull request Apr 25, 2023
Closes #5254

In #6189 the SQLite version was bumped to a newer release which has builtin support for Full and Right joins, so no workaround is no longer needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch insert suggestions
4 participants