mengw15 commented on code in PR #7789:
URL: https://github.com/apache/texera/pull/7789#discussion_r3826126441
##########
sql/updates/36.sql:
##########
@@ -80,22 +98,32 @@ BEGIN
op,
'{operatorProperties,fileName}',
CASE
- WHEN f.fn IS NOT NULL AND left(f.fn, 10) <>
'/datasets/'
+ WHEN f.fn ~ '^/datasets/'
+ AND EXISTS (SELECT 1 FROM dataset d JOIN "user" u
ON d.owner_uid = u.uid
+ WHERE u.email =
split_part(ltrim(f.fn, '/'), '/', 2)
+ AND d.name =
split_part(ltrim(f.fn, '/'), '/', 3))
+ THEN to_jsonb(regexp_replace(f.fn, '^/datasets/',
'/dataset/'))
Review Comment:
Agreed on the release part — I checked too, `release/v1.2` has no
`sql/updates/3*.sql`, so no release has a checksum to invalidate.
But the manual `psql` run isn't really a delivery mechanism for the group
it's meant to cover. Nothing prompts for it: the skip is silent, so a developer
who pulls and runs `local-dev up` sees a healthy startup and finds out only
when someone opens one of those workflows. Deployments tracking `main` migrate
automatically and have no hand-run step at all.
On "two migrations describing one path format is worse to carry forward" —
that's the normal cost once the first one has shipped anywhere, and append-only
is what makes the history trustworthy. 36 → 37 → 38 reads fine in order, and
it's cheap next to data that goes quietly stale.
So yes, I'd add the migration. Worth carrying the same `(user.email,
dataset.name)` EXISTS guard into it — after 36 runs, `/datasets/...` covers
both the paths it just prefixed and any local mounts that already looked like
that, so the guard is what keeps the second group untouched. It's the logic
you've already written here, just moved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
