feat(storage): add datasets resource-type prefix to logical paths - #6502
feat(storage): add datasets resource-type prefix to logical paths#6502tanishqgandhi1908 wants to merge 15 commits into
Conversation
… tests Review fixes on apache#5911: use dataset owner email in retrieveLatestDatasetVersion, replace brittle Option.get/.head with headOption, strip datasets prefix on selection-modal reopen, clarify FileResolver docs; add FileResolver/DatasetFileNode and frontend path-helper tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
|
/request-review @aicam |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6502 +/- ##
============================================
- Coverage 76.28% 76.27% -0.01%
Complexity 3452 3452
============================================
Files 1161 1161
Lines 45916 45897 -19
Branches 5099 5097 -2
============================================
- Hits 35025 35010 -15
+ Misses 9327 9323 -4
Partials 1564 1564
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 381 | 0.233 | 25,064/34,690/34,690 us | 🟢 -11.7% / 🔴 +116.7% |
| 🔴 | bs=100 sw=10 sl=64 | 780 | 0.476 | 127,723/155,642/155,642 us | 🔴 +15.4% / 🔴 +46.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 907 | 0.554 | 1,105,666/1,143,530/1,143,530 us | ⚪ within ±5% / 🔴 +13.6% |
Baseline details
Latest main dda9e83 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 381 tuples/sec | 402 tuples/sec | 767.66 tuples/sec | -5.2% | -50.4% |
| bs=10 sw=10 sl=64 | MB/s | 0.233 MB/s | 0.245 MB/s | 0.469 MB/s | -4.9% | -50.3% |
| bs=10 sw=10 sl=64 | p50 | 25,064 us | 24,154 us | 12,623 us | +3.8% | +98.6% |
| bs=10 sw=10 sl=64 | p95 | 34,690 us | 39,307 us | 16,011 us | -11.7% | +116.7% |
| bs=10 sw=10 sl=64 | p99 | 34,690 us | 39,307 us | 18,856 us | -11.7% | +84.0% |
| bs=100 sw=10 sl=64 | throughput | 780 tuples/sec | 828 tuples/sec | 998.44 tuples/sec | -5.8% | -21.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.476 MB/s | 0.505 MB/s | 0.609 MB/s | -5.7% | -21.9% |
| bs=100 sw=10 sl=64 | p50 | 127,723 us | 120,403 us | 100,463 us | +6.1% | +27.1% |
| bs=100 sw=10 sl=64 | p95 | 155,642 us | 134,865 us | 106,400 us | +15.4% | +46.3% |
| bs=100 sw=10 sl=64 | p99 | 155,642 us | 134,865 us | 117,712 us | +15.4% | +32.2% |
| bs=1000 sw=10 sl=64 | throughput | 907 tuples/sec | 922 tuples/sec | 1,034 tuples/sec | -1.6% | -12.3% |
| bs=1000 sw=10 sl=64 | MB/s | 0.554 MB/s | 0.563 MB/s | 0.631 MB/s | -1.6% | -12.2% |
| bs=1000 sw=10 sl=64 | p50 | 1,105,666 us | 1,086,250 us | 973,294 us | +1.8% | +13.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,143,530 us | 1,121,650 us | 1,019,213 us | +2.0% | +12.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,143,530 us | 1,121,650 us | 1,049,896 us | +2.0% | +8.9% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,524.76,200,128000,381,0.233,25064.07,34689.64,34689.64
1,100,10,64,20,2564.32,2000,1280000,780,0.476,127723.35,155641.71,155641.71
2,1000,10,64,20,22042.32,20000,12800000,907,0.554,1105666.19,1143529.66,1143529.66The datasets logical-path prefix strips four leading segments (datasets/owner/dataset/version); update the cover-image test's input path to include the prefix so the extracted relative path is the file name, not an empty string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I see that it has no behavioral change now, but the intention is to later introduce other source types prefixes such as models right? when we do so, it will eventually have a collision on the path without the prefix. how are we going to solve that data migration issue later? A possible solution:
If you can have a better solution or data migration plan that would be better. |
|
Makes sense. Sure let me research a bit and get back |
|
Thanks again @Yicong-Huang. I think prefix-required + a one-time migration would be a better approach than keeping datasets unprefixed. Reason: with models getting their own table, the leading segment is what tells the resolver which table to look in, so an unprefixed path can't be routed unambiguously. Making the prefix required keeps a single, self-describing path shape. Migration (one-time via Liquibase):
More context and the design (with diagrams) are in this discussion — happy to go deeper there: #6616 |
|
The topic is about backward compatibility. We understand the tradeoffs. I prefer to require existing users to use the new prefix format so that the codebase is cleaner. |
|
The migration plan sounds good to me, can we make it programmatic? i.e., please include a SQL script in this PR. |
Now that models are coming as a separate resource/table, the leading resource-type segment is what selects the backing table, so an unprefixed path can no longer be routed unambiguously. Make the "datasets" prefix required instead of a tolerated fallback, and migrate existing data. - FileResolver: a dataset path must start with the "datasets" segment; unprefixed paths are no longer treated as dataset paths. - pytexera DatasetFileDocument: same rule, mirroring the backend. - FileListerSourceOpExec: parse the now-prefixed datasetVersionPath (skip the "datasets" segment); extracted into a testable helper. - Migration (sql/updates/29.sql): prepend "datasets/" to legacy paths stored in workflow.content and workflow_version.content, covering both the fileName (scan sources) and datasetVersionPath (file lister) operator properties. Only values whose first two segments match an existing (user.email, dataset.name) are rewritten, so local paths and URLs are left untouched; email format is irrelevant (owner may be a username without "@"). Uses create_missing=false and is idempotent. - Example workflows: use datasets-prefixed paths. Tests: FileResolverSpec and WorkflowExecutionsResourceSpec updated; test_dataset_file_document.py updated; new FileListerSourceOpExecSpec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…l-path-prefix # Conflicts: # frontend/src/app/common/type/datasetVersionFileTree.spec.ts
…format python test - dataset-selection-modal.component.spec.ts: expect the datasets-prefixed selectedPath in version (non-file) mode, matching the emitted path. - test_dataset_file_document.py: apply ruff format (wrap an over-length line) so `ruff format --check` passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The cover-image endpoints resolve a dataset path built as
{owner}/{name}/{coverImage}. With the prefix now required by FileResolver,
these must carry the datasets/ segment; add it in the set-cover,
get-cover redirect, and cover-url handlers so cover images resolve again.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done, The migration is included as a SQL script in this PR: |
aicam
left a comment
There was a problem hiding this comment.
1- It has been said prefix is required 100 times, please make sure comments are meaningful and helpful
2- I am still concerned about this design: - frontend is not following backend design and ignores prefix - prefix MUST be an enum to allow extendability
What changes were proposed in this PR?
Adds an explicit resource-type prefix to asset logical file paths, changing the format from
/<owner>/<name>/<version>/<file>to/datasets/<owner>/<name>/<version>/<file>.Makes the
datasetsresource-type prefix required on dataset logical file paths (/datasets/<owner>/<name>/<version>/<file>), so other resource types (e.g. models) can be told apart by the prefix and routed to their own table. Unlike the initial approach, an unprefixed path no longer resolves — the prefix is what selects the resource's table.FileResolver): a dataset path must start withdatasets; the previous "parse unprefixed as-is" fallback is removed.DatasetFileDocument): same rule, mirroring the backend.datasetVersionPath(the second dataset-path property, alongside scan sources'fileName).datasetsnode; the selection modal emits prefixed paths; relative-path extraction strips the 4-segment prefix; unused client-side parser removed.DatasetResource): cover-image handlers now build prefixed paths.sql/updates/29.sql): one-time, Liquibase-run rewrite that prependsdatasets/to legacy paths inworkflow.contentandworkflow_version.content(bothfileNameanddatasetVersionPath). Only values whose first two segments match an existing(user.email, dataset.name)are rewritten (local paths/URLs untouched; email format is irrelevant); idempotent.Known migration limits: a path whose dataset was renamed or deleted since the workflow was saved won't match
(email, name), so it stays unprefixed and will fail to resolve (it was already unusable). The migration assumescontentis valid JSON (an app invariant) and aborts on a malformed row rather than skipping.Any related issues, documentation, discussions?
Closes #6495.
How was this PR tested?
New and updated unit tests, all passing locally:
FileResolverSpec: an unprefixed path (and a non-datasetsprefix) no longer resolves; a prefixed path resolves; too-few-segments is rejected.FileListerSourceOpExecSpec(new): prefixeddatasetVersionPathparses; unprefixed/wrong-prefix/too-few rejected.datasetVersionFileTreeanddataset-selection-modalspecs updated for the prefix.test_dataset_file_document.py— prefix required, presign re-emits it.The migration (
29.sql) was verified manually against sample data: unprefixed→prefixed;already-prefixed left unchanged (idempotent); local paths/URLs untouched; dangling
(renamed/deleted) datasets untouched; operators without the property get no spurious key added;
both
fileNameanddatasetVersionPathcovered.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)