MBS-14208: ignore ended area relationships#3686
MBS-14208: ignore ended area relationships#3686owlpharoah wants to merge 1 commit intometabrainz:masterfrom
Conversation
|
This is a good start, thanks. But this kind of thing (changing SQL functions) is a schema change - because of how MusicBrainz works and our agreements with data users, we only make these once every six months at most and they need some extra changes. See a similar change I made for our last schema change: #3417 You don't need to work on all those extra bits though, if you prefer we can do that ourselves :) But just so it's clear that this won't be merged / released until probably May 2026, whether you make the extra schema-change related changes or we do. |
|
sounds good! ill make the required changes. just a small doubt; should the changes be in another file 31.all.sql or in the same one as #3417 |
|
You should create a file in |
added test cases Ran Schema Script
owlpharoah
force-pushed
the
mbs-14208
branch
from
74aa748 to
9786292
Compare
|
yup made the required changes and ran the script. do let me know if anything has to be changed :) |
|
This wasn't announced in https://blog.metabrainz.org/2026/03/11/schema-change-release-may-11-2026/ so will unfortunately have to be delayed until the following schema change release. |
|
Argh. It was in the list and marked as ready, did we really forget to post it 🫠 It doesn't actually break the schema though, does it? It just changes a function. So it doesn't seem particularly problematic to still release it, but I guess it's minor enough so it can also wait if needed. |
|
I'd prefer we avoided adding in unannounced changes at the last minute. I do agree it seems minor, but sometimes issues come up that we don't expect. This PR is also incomplete and requires further changes. |
mwiencek
left a comment
mwiencek
left a comment
There was a problem hiding this comment.
I was going to suggest removing the ended rows from the area_containment table in the upgrade script, but I think it's fine to just tell people to rebuild the area_containment table using ./admin/BuildMaterializedTables --database=MAINTENANCE --force area_containment in the upgrade notes. Can you mention that in the documentation section of the PR description so we don't forget? :)
| FROM l_area_area laa | ||
| JOIN link ON laa.link = link.id | ||
| WHERE link.link_type = $1 | ||
| AND link.ended = FALSE |
There was a problem hiding this comment.
You're missing a space here. :) The functions must be exactly identical to the ones in the upgrade script (we have tests that verify this).
| AND link.ended = FALSE | |
| AND link.ended = FALSE |
| FROM l_area_area laa | ||
| JOIN link ON laa.link = link.id | ||
| WHERE link.link_type = $1 | ||
| AND link.ended = FALSE |
There was a problem hiding this comment.
| AND link.ended = FALSE | |
| AND link.ended = FALSE |
| "20150429-tag-upvote.sql", | ||
| "20150403-drop-empty-functions.sql" | ||
| ], | ||
| "mirror_only": [ |
There was a problem hiding this comment.
Please also revert all non-relevant formatting changes from this file. What tool made those changes? If it was an AI tool, disclose AI usage in the PR description.
3 participants
Problem
Right now, even if an area relationship is ended, it still shows up. Ideally this ended relationship should be ignored
Solution MBS-14208
The solution is to only include area relationships where
link.ended=False. earlier it was done regardless of the ended status.Testing
Wrote automated tests