You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "Ma77Ball (via GitHub)" <gi...@apache.org> on 2026/07/20 21:32:22 UTC

[PR] test(pyamber): add unit tests for vfs_uri_factory [texera]

Ma77Ball opened a new pull request, #6641:
URL: https://github.com/apache/texera/pull/6641

   ### What changes were proposed in this PR?
   - Add unit tests covering VFSURIFactory base/result/state URI construction, build to decode round-trips, the absent-globalportid None branch, and decode error paths (bad scheme, missing key, unknown resource type).
   ### Any related issues, documentation, discussions?
   Closes: #6640
   ### How was this PR tested?
   - Run: `cd amber && python -m pytest src/test/python/core/storage/test_vfs_uri_factory.py`, expect all 10 tests passing
   ### Was this PR authored or co-authored using generative AI tooling?
   Co-authored with Claude Opus 4.8 in compliance with ASF


-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(pyamber): add unit tests for vfs_uri_factory [texera]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6641:
URL: https://github.com/apache/texera/pull/6641#issuecomment-5027494894

   <!-- texera-reviewer-suggestion -->
   ### 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.
   


-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(pyamber): add unit tests for vfs_uri_factory [texera]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6641:
URL: https://github.com/apache/texera/pull/6641#discussion_r3617819682


##########
amber/src/test/python/core/storage/test_vfs_uri_factory.py:
##########
@@ -0,0 +1,140 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import pytest
+
+from core.storage.vfs_uri_factory import VFSResourceType, VFSURIFactory
+from proto.org.apache.texera.amber.core import (
+    ExecutionIdentity,
+    GlobalPortIdentity,
+    OperatorIdentity,
+    PhysicalOpIdentity,
+    PortIdentity,
+    WorkflowIdentity,
+)
+
+
+def _gpi(
+    op_id: str = "myOp",
+    layer: str = "main",
+    port: int = 0,
+    internal: bool = False,
+    is_input: bool = True,
+) -> GlobalPortIdentity:
+    return GlobalPortIdentity(
+        op_id=PhysicalOpIdentity(
+            logical_op_id=OperatorIdentity(id=op_id), layer_name=layer
+        ),
+        port_id=PortIdentity(id=port, internal=internal),
+        input=is_input,
+    )
+
+
+class TestCreatePortBaseUri:
+    def test_base_uri_encodes_scheme_ids_and_serialized_port(self):
+        uri = VFSURIFactory.create_port_base_uri(
+            WorkflowIdentity(id=7), ExecutionIdentity(id=3), _gpi()
+        )
+        # The base URI stitches together the vfs scheme, the wid/eid segments,
+        # and the serialized global port identity as a single trailing segment.
+        assert uri == (
+            "vfs:///wid/7/eid/3/globalportid/"
+            "(logicalOpId=myOp,layerName=main,portId=0,"
+            "isInternal=false,isInput=true)"
+        )
+
+    def test_result_uri_appends_result_segment(self):
+        base = VFSURIFactory.create_port_base_uri(
+            WorkflowIdentity(id=1), ExecutionIdentity(id=1), _gpi()
+        )
+        assert VFSURIFactory.result_uri(base) == f"{base}/result"
+
+    def test_state_uri_appends_state_segment(self):
+        base = VFSURIFactory.create_port_base_uri(
+            WorkflowIdentity(id=1), ExecutionIdentity(id=1), _gpi()
+        )
+        assert VFSURIFactory.state_uri(base) == f"{base}/state"
+
+
+class TestDecodeUriRoundTrip:
+    def test_result_uri_round_trips_through_decode(self):
+        wid, eid, gpi = WorkflowIdentity(id=42), ExecutionIdentity(id=9), _gpi(
+            op_id="opA", layer="main", port=2, internal=True, is_input=False
+        )
+        base = VFSURIFactory.create_port_base_uri(wid, eid, gpi)
+        uri = VFSURIFactory.result_uri(base)
+

Review Comment:
   `VFSURIFactory.decode_uri()` lowercases the final path segment before converting it to `VFSResourceType`, but `VFSResourceType` includes camelCase values (`runtimeStatistics`, `consoleMessages`). This means URIs ending with `/runtimestatistics` or `/consolemessages` (the format produced by the Scala VFSURIFactory) will currently raise `ValueError("Unknown resource type")` in Python. Consider adding unit coverage for these resource types and adjusting `decode_uri` to do a case-insensitive match against enum values (e.g., compare `t.value.lower()`), so the Python decoder stays compatible with existing URIs.



-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(pyamber): add unit tests for vfs_uri_factory [texera]

Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #6641:
URL: https://github.com/apache/texera/pull/6641


-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(pyamber): add unit tests for vfs_uri_factory [texera]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6641:
URL: https://github.com/apache/texera/pull/6641#issuecomment-5027534877

   <!-- texera-benchmarks-comment -->
   ## ⚠️ Benchmark changes need a look
   
   🟢 2 better · 🔴 4 worse · ⚪ 9 noise (<±5%) · 0 without baseline
   
   > Compared against main `f14cd39` 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](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/29780568537)
   
   | | config | throughput | MB/s | latency | max Δ latest / 7d |
   |---|---|---:|---:|---:|---:|
   | 🟢 | bs=10 sw=10 sl=64 | 446 | 0.272 | 21,986/31,361/31,361 us | 🟢 -9.6% / 🔴 +100.6% |
   | 🔴 | bs=100 sw=10 sl=64 | 922 | 0.563 | 106,988/149,606/149,606 us | 🔴 +25.9% / 🔴 +41.1% |
   | ⚪ | bs=1000 sw=10 sl=64 | 1,088 | 0.664 | 917,901/963,535/963,535 us | ⚪ within ±5% / 🟢 -8.1% |
   
   <details><summary>Baseline details</summary>
   
   Latest main `f14cd39` from same runner
   
   | config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
   |---|---|---:|---:|---:|---:|---:|
   | bs=10 sw=10 sl=64 | throughput | 446 tuples/sec | 440 tuples/sec | 767.23 tuples/sec | +1.4% | -41.9% |
   | bs=10 sw=10 sl=64 | MB/s | 0.272 MB/s | 0.268 MB/s | 0.468 MB/s | +1.5% | -41.9% |
   | bs=10 sw=10 sl=64 | p50 | 21,986 us | 21,317 us | 12,705 us | +3.1% | +73.1% |
   | bs=10 sw=10 sl=64 | p95 | 31,361 us | 34,704 us | 15,633 us | -9.6% | +100.6% |
   | bs=10 sw=10 sl=64 | p99 | 31,361 us | 34,704 us | 20,214 us | -9.6% | +55.1% |
   | bs=100 sw=10 sl=64 | throughput | 922 tuples/sec | 974 tuples/sec | 1,001 tuples/sec | -5.3% | -7.9% |
   | bs=100 sw=10 sl=64 | MB/s | 0.563 MB/s | 0.594 MB/s | 0.611 MB/s | -5.2% | -7.9% |
   | bs=100 sw=10 sl=64 | p50 | 106,988 us | 103,297 us | 99,949 us | +3.6% | +7.0% |
   | bs=100 sw=10 sl=64 | p95 | 149,606 us | 118,790 us | 105,996 us | +25.9% | +41.1% |
   | bs=100 sw=10 sl=64 | p99 | 149,606 us | 118,790 us | 117,999 us | +25.9% | +26.8% |
   | bs=1000 sw=10 sl=64 | throughput | 1,088 tuples/sec | 1,092 tuples/sec | 1,033 tuples/sec | -0.4% | +5.3% |
   | bs=1000 sw=10 sl=64 | MB/s | 0.664 MB/s | 0.667 MB/s | 0.631 MB/s | -0.4% | +5.3% |
   | bs=1000 sw=10 sl=64 | p50 | 917,901 us | 919,112 us | 974,571 us | -0.1% | -5.8% |
   | bs=1000 sw=10 sl=64 | p95 | 963,535 us | 968,369 us | 1,016,467 us | -0.5% | -5.2% |
   | bs=1000 sw=10 sl=64 | p99 | 963,535 us | 968,369 us | 1,048,798 us | -0.5% | -8.1% |
   
   </details>
   
   <details><summary>Raw CSV</summary>
   
   ```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,448.51,200,128000,446,0.272,21986.36,31360.70,31360.70
   1,100,10,64,20,2169.28,2000,1280000,922,0.563,106987.89,149606.46,149606.46
   2,1000,10,64,20,18381.66,20000,12800000,1088,0.664,917900.95,963534.78,963534.78
   ```
   
   </details>
   


-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(pyamber): add unit tests for vfs_uri_factory [texera]

Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on PR #6641:
URL: https://github.com/apache/texera/pull/6641#issuecomment-5027627556

   Please make the CI green.


-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(pyamber): add unit tests for vfs_uri_factory [texera]

Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on PR #6641:
URL: https://github.com/apache/texera/pull/6641#issuecomment-5027713575

   Please address the comment.


-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org