aicam commented on code in PR #7789:
URL: https://github.com/apache/texera/pull/7789#discussion_r3825036530
##########
sql/updates/36.sql:
##########
@@ -80,22 +92,26 @@ BEGIN
op,
'{operatorProperties,fileName}',
CASE
- WHEN f.fn IS NOT NULL AND left(f.fn, 10) <>
'/datasets/'
+ WHEN f.fn ~ '^/(dataset|model)s/'
+ THEN to_jsonb(regexp_replace(f.fn,
'^/(dataset|model)s/', '/\1/'))
Review Comment:
**Case 2 rewrites unconditionally, so it can break genuine local file
paths.**
The case-1 arm just below only rewrites a value when its first two segments
match an existing `(user.email, dataset.name)` pair. That guard is what makes
it safe to run over a column that also holds plain filesystem paths and URLs:
no match, no rewrite. Case 2 keys off the leading segment alone.
But `/datasets` is also an ordinary directory name, and
`FileResolver.resolve` tries `localResolveFunc` before
`versionedResourceResolveFunc`, so a scan-source `fileName` of
`/datasets/imdb/movies.csv` on a host with a `/datasets` data mount is a valid,
working configuration today. (It is only 3 segments, so `parsePrefixedPath`
rejects it outright -- it resolves purely as a local file.) This migration
rewrites it to `/dataset/imdb/movies.csv` and it stops resolving. The header
comment at L44 covers paths that *contain* `/datasets/` further along, not ones
rooted at it.
Same guard as case 1, with the offsets shifted by one because case-2 paths
carry a prefix -- after `ltrim` the email is part 2 and the name part 3:
```suggestion
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/'))
```
Falling through is safe: an unguarded value then hits case 1's `WHEN`, whose
own guard tests `datasets`/`imdb` as an email/name pair, fails, and lands in
`ELSE`, leaving the value untouched.
The same edit applies to the `datasetVersionPath` arm at L108-109 and to
both `workflow_version` arms at L180-181 / L193-194. The WHERE predicates at
L70/L72 and L155/L157 need it too, otherwise those rows are still selected and
merely no-op'd.
Dropping `model` from the regex is deliberate: `sql/updates/37.sql` is what
creates the `model` table and it runs after this changeset, so when 36 executes
no stored workflow can hold a `/models/` path yet -- and on a DB where 37 has
already run, 36 has run too and will not re-run.
--
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]
