handle stable id for features data [MRXN23-588] [MRXN23-589] [MRXN23-590] #1654
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@aagm @KevSanchez I've asked you both for a review for additional cross-checks. I have included in this baseline PR both schema and TypeORM entity changes as well as a brief writeup of the plan for the documentation folder (for Alicia to cross-check, since we discussed this quite a while ago, and for Kevin as intro to the context of this epic). I have already prepared a PR that should handle the new columns/properties through the cloning flow (piece exporters and importers), but that's still in progress - I've adjusted obvious things in tests to match new expected data, but I'm waiting for tests to run fully and let me know if there's anything I've missed. |
| DROP TRIGGER tr_precompute_feature_property_list ON features_data; | ||
|
|
||
| CREATE TRIGGER tr_precompute_feature_property_list AFTER INSERT ON features_data | ||
| FOR EACH ROW EXECUTE | ||
| PROCEDURE precompute_feature_property_list(); |
There was a problem hiding this comment.
You' basically removing the AFTER UPDATE condition here, right? What's the reason for it? (mostly because I'm not 100% sure on what this trigger does, and I would like to know the difference)
There was a problem hiding this comment.
that's correct - switching from a setup where we trigger AFTER INSERT OR UPDATE to only triggering AFTER INSERT. In practice, there's no need to ever execute precompute_feature_property_list() more than once and ON INSERT, as spatial data in features_data can never actually change, by design (we don't have a way to update features_data rows).
On the other hand, keeping the AFTER UPDATE part of the trigger setup would pointlessly recalculate feature property lists when the migration would do the simple update to set the stable id for existing data - this would take forever even on a local development instance with not much spatial data in it, so I took the chance to simplify this a bit, which also makes this whole migration run snappily rather than taking ages and doing a lot of repeated work.
| ALTER TABLE features_data | ||
| ALTER COLUMN stable_id SET DEFAULT gen_random_uuid(); |
There was a problem hiding this comment.
If I undesrtood correctly, it doesn't really matter, but to keep it consistent with the initial migration, this could be set to be the feature_data id, right? Unless it's actually not possible to reference a generated primary key in a another column's DEFAULT clause
There was a problem hiding this comment.
yep, as far as I know and could gather from PG's docs (https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARMS-DEFAULT), I think that this is not possible unless using a trigger, which I think would be overkill for this - I thought about this too because at least from fully new features (that is, not copied from a source project through cloning) it seems neater, even though in practice it doesn't matter.
Using a generated always ... stored column allows to reference other columns, but then it would not be possible to set this column to the value we want. And using generated ... identity allows to update to user-supplied values, but this would apply to integer sequence columns only, so not good for our uuids. As far as I know, at least!
|
merging this as discussed today |
hotzevzl
deleted the
chore/api/MRXN23-588+MRXN23-589+MRXN23-590_handle-stable-id-for-features-data
branch
Foundation work for the Make Splits Great Again epic.
This PR adds table columns and includes TypeORM entities changes to accommodate the data needed to link features to subsets of
features_dataat the outset, rather than by running spatial queries when needed.https://vizzuality.atlassian.net/browse/MRXN23-588
https://vizzuality.atlassian.net/browse/MRXN23-589
https://vizzuality.atlassian.net/browse/MRXN23-590