Skip to content

test(workflow-core): add unit test coverage for DatasetFileDocument URI parsing#6473

Open
eugenegujing wants to merge 1 commit into
apache:mainfrom
eugenegujing:test/dataset-file-document-spec
Open

test(workflow-core): add unit test coverage for DatasetFileDocument URI parsing#6473
eugenegujing wants to merge 1 commit into
apache:mainfrom
eugenegujing:test/dataset-file-document-spec

Conversation

@eugenegujing

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

This PR adds unit test coverage for the URI parsing in DatasetFileDocument (common/workflow-core). Every dataset file read in the engine goes through this parsing — the constructor splits a dataset:///{repositoryName}/{versionHash}/{fileRelativePath} URI into its three components — yet the class previously had zero references in the test tree.

The new DatasetFileDocumentSpec (10 tests, constructor + getters only, no I/O) covers:

  • Parsing a valid 3-segment dataset URI into repository name, version hash, and relative file path, plus joining of multi-segment relative paths (expected paths built with Paths.get so assertions are platform-separator independent).
  • URL decoding that mirrors how FileResolver builds these URIs in production (URLEncoder-encoded segments assembled with the multi-argument URI constructor): percent-escapes and bare + in the version hash and in each relative-path segment decode exactly once, including a non-ASCII UTF-8 file-name round trip.
  • The four getters (getRepositoryName, getVersionHash, getFileRelativePath, getURI), asserting getURI returns the original URI unchanged.
  • Current edge-case behavior, pinned deliberately: the repository name is not URL-decoded (matching FileResolver, which encodes only the relative-path segments), redundant and trailing slashes are collapsed by Paths.get before segment counting, and dot segments (. / ..) are preserved without normalization — marked "(current behavior)" in the test rather than endorsed.
  • The error path: URIs with fewer than three path segments (0, 1, 2, and a trailing-slash form that looks like 3 but yields 2) throw IllegalArgumentException with the message "URI format is incorrect".

The spec lives in the org.apache.texera.amber.core.storage.model package so it can construct the private[storage] class directly. No test touches asInputStream, asFile, or clear, so the suite runs without network, LakeFS, or environment-variable dependencies. Test-only change — no production code modified, no UI change.

Any related issues, documentation, discussions?

Closes #6441

How was this PR tested?

  • sbt "WorkflowCore/testOnly org.apache.texera.amber.core.storage.model.DatasetFileDocumentSpec" — 10/10 pass (suite runs in ~170 ms).
  • Mutation-checked during development: deliberately breaking individual assertions made exactly the corresponding tests fail, then restored to green, confirming each assertion constrains the implementation.

Was this PR authored or co-authored using generative AI tooling?

Co-authored using Claude Code(Fable 5)

…RI parsing

Add DatasetFileDocumentSpec for the constructor URI parsing that every dataset file read goes through, previously untested. The 10 tests cover parsing a valid dataset URI into repository name, version hash, and relative file path, multi-segment path joining, URL decoding that mirrors how FileResolver encodes these URIs in production (including non-ASCII file names), the four getters, and the error path where URIs with fewer than three path segments throw IllegalArgumentException("URI format is incorrect").

They also pin current edge-case behavior: the repository name is not URL-decoded (matching FileResolver, which encodes only the relative-path segments), redundant and trailing slashes are collapsed, and dot segments are preserved without normalization. Only the constructor and getters are exercised, so the suite needs no network, LakeFS, or environment variables.

Closes apache#6441
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 7 better · 🔴 3 worse · ⚪ 5 noise (<±5%) · 0 without baseline

Compared against main c7473a1 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 548 0.334 17,324/25,014/25,014 us 🔴 +8.0% / 🔴 +78.2%
🟢 bs=100 sw=10 sl=64 1,223 0.746 81,113/103,901/103,901 us 🟢 -16.2% / 🟢 -11.3%
bs=1000 sw=10 sl=64 1,434 0.875 710,283/735,185/735,185 us ⚪ within ±5% / 🟢 +27.2%
Baseline details

Latest main c7473a1 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 548 tuples/sec 579 tuples/sec 845.85 tuples/sec -5.4% -35.2%
bs=10 sw=10 sl=64 MB/s 0.334 MB/s 0.353 MB/s 0.516 MB/s -5.4% -35.3%
bs=10 sw=10 sl=64 p50 17,324 us 16,034 us 11,663 us +8.0% +48.5%
bs=10 sw=10 sl=64 p95 25,014 us 26,485 us 14,037 us -5.6% +78.2%
bs=10 sw=10 sl=64 p99 25,014 us 26,485 us 18,666 us -5.6% +34.0%
bs=100 sw=10 sl=64 throughput 1,223 tuples/sec 1,149 tuples/sec 1,102 tuples/sec +6.4% +11.0%
bs=100 sw=10 sl=64 MB/s 0.746 MB/s 0.701 MB/s 0.673 MB/s +6.4% +10.9%
bs=100 sw=10 sl=64 p50 81,113 us 85,459 us 91,441 us -5.1% -11.3%
bs=100 sw=10 sl=64 p95 103,901 us 123,978 us 97,817 us -16.2% +6.2%
bs=100 sw=10 sl=64 p99 103,901 us 123,978 us 108,384 us -16.2% -4.1%
bs=1000 sw=10 sl=64 throughput 1,434 tuples/sec 1,401 tuples/sec 1,128 tuples/sec +2.4% +27.2%
bs=1000 sw=10 sl=64 MB/s 0.875 MB/s 0.855 MB/s 0.688 MB/s +2.3% +27.1%
bs=1000 sw=10 sl=64 p50 710,283 us 711,522 us 901,939 us -0.2% -21.2%
bs=1000 sw=10 sl=64 p95 735,185 us 751,753 us 940,934 us -2.2% -21.9%
bs=1000 sw=10 sl=64 p99 735,185 us 751,753 us 977,688 us -2.2% -24.8%
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,365.25,200,128000,548,0.334,17324.46,25014.13,25014.13
1,100,10,64,20,1635.45,2000,1280000,1223,0.746,81112.58,103900.98,103900.98
2,1000,10,64,20,13947.79,20000,12800000,1434,0.875,710283.38,735185.05,735185.05

@codecov-commenter

codecov-commenter commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.20%. Comparing base (aa0bc44) to head (26213c6).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6473      +/-   ##
============================================
+ Coverage     70.17%   70.20%   +0.02%     
- Complexity     3388     3393       +5     
============================================
  Files          1142     1142              
  Lines         44844    44844              
  Branches       4949     4949              
============================================
+ Hits          31471    31482      +11     
+ Misses        11739    11723      -16     
- Partials       1634     1639       +5     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 76.76% <ø> (ø) Carriedforward from aa0bc44
amber 66.73% <ø> (+0.06%) ⬆️
computing-unit-managing-service 9.97% <ø> (ø)
config-service 52.30% <ø> (ø)
file-service 65.63% <ø> (ø)
frontend 68.91% <ø> (ø) Carriedforward from aa0bc44
notebook-migration-service 78.94% <ø> (ø)
pyamber 91.19% <ø> (ø) Carriedforward from aa0bc44
workflow-compiling-service 55.14% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Scala unit tests in common/workflow-core to cover DatasetFileDocument URI parsing and its getter methods, ensuring the dataset-file URI contract is exercised in isolation (no I/O).

Changes:

  • Introduces DatasetFileDocumentSpec with cases for valid dataset URIs, multi-segment relative paths, URL-decoding behavior, and error handling for invalid URIs.
  • Adds coverage for edge-case parsing behaviors currently exhibited by the implementation (e.g., redundant/trailing slashes, dot segments).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add unit test coverage for DatasetFileDocument URI parsing

3 participants