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

Multi value fields #181

Closed
wants to merge 22 commits into from
Closed

Multi value fields #181

wants to merge 22 commits into from

Conversation

saecki
Copy link
Contributor

@saecki saecki commented Dec 17, 2022

This is a POC to store multiple values per field. Starting with artists and album artists. If this was to be merged, we should probably do the same for lyricists and composers.
It also paves the way for semantic browsing, as we now have a table of artists (and possibly composers and lyricists). And something similar can be done for albums.

One thing to be cautious about is that ids of songs, and artists will have to be stable, we could also make the join table work on paths and artists names. But to be honest I prefer using numeric ids.

Let me know what you think.

@saecki saecki marked this pull request as draft December 17, 2022 15:28
@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2022

Codecov Report

Merging #181 (6c2309d) into master (00b6444) will increase coverage by 0.12%.
The diff coverage is 84.95%.

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   72.17%   72.30%   +0.12%     
==========================================
  Files          31       31              
  Lines        1840     1946     +106     
==========================================
+ Hits         1328     1407      +79     
- Misses        512      539      +27     
Impacted Files Coverage Δ
src/app/lastfm.rs 8.57% <0.00%> (ø)
src/app/index/update/inserter.rs 74.41% <68.25%> (-14.78%) ⬇️
src/app/index/update/collector.rs 84.93% <68.75%> (-3.17%) ⬇️
src/app/index/types.rs 77.41% <76.92%> (-0.36%) ⬇️
src/app/index/update/cleaner.rs 88.23% <83.33%> (-1.06%) ⬇️
src/app/playlist.rs 91.17% <86.66%> (-2.67%) ⬇️
src/app/index/query.rs 88.97% <91.93%> (+1.22%) ⬆️
src/app/index/metadata.rs 88.69% <100.00%> (-0.46%) ⬇️
src/db/schema.rs 71.42% <100.00%> (+71.42%) ⬆️
src/service/actix/api.rs 76.87% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@agersant
Copy link
Owner

Hello! As always, thank you for the continued hard work 🙏

However, there are two important reasons which prevent me from accepting this PR:

  1. With how fundamental these changes are, I would want to implement them myself. Not because I don't trust the quality of the work you have been writing, but because I feel more comfortable owning code I write myself in terms of familiarity, edge cases, performance characteristics, etc.. And this is code I will be maintaining for a long time (at my own snail pace 🐌).
  2. I have been speculating on a different approach for this specific subject (too early to share). I am worried that the classic SQL multi-table + foreign keys approach would require too many requests and/or joins to implement the complex queries described in Add new API functions to support browsing the collection by genre/artist/album #92 efficiently and elegantly, and also have might have significant impact on indexing performance.

The search PR that has been open forever is in a similar state. I have different ideas on implementation details, and more importantly, the scope is large enough that I want to do the initial work on it. I hope this is not a deterrent for future smaller-scope contributions as I am very happy with everything you have been doing on Polaris so far.

@saecki
Copy link
Contributor Author

saecki commented Dec 17, 2022

Understandable, have a great day!

But seriously, I understand your concerns.
I just checked and the indexing times alone are about 100x slower after this patch, which is unacceptable. Using a transaction the performance impacts aren't too bad (about 45% slower on my machine).
Nonetheless I respect that you feel more comfortable having written fundamental parts of polaris yourself. I'm interested in seeing your approach once it's ready.

@agersant
Copy link
Owner

Some good news on this front! I have started working on a major update to Polaris which does include multi-value fields and much more.

If you're curious, you can track progress on this board or in the next branch.

@saecki saecki closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants