mengw15 commented on code in PR #8238:
URL: https://github.com/apache/texera/pull/8238#discussion_r3969989775
##########
amber/src/main/python/core/storage/vfs_uri_factory.py:
##########
@@ -88,7 +88,7 @@ def extract_value(key: str) -> str:
else None
)
- resource_type_str = segments[-1].lower()
+ resource_type_str = segments[-1]
Review Comment:
`document_factory` dispatches only `RESULT` (:54) and `STATE` (:56), and
Python builds only those two URIs (:165, :170), so nothing in Python sends,
receives or handles `runtimeStatistics` / `consoleMessages`. This decodes a
shape no producer emits.
The mismatch runs the other way. `VFSURIFactory.scala` appends the resource
segment in exactly two places — `:176` for result/state and `:268`, which is
where `createRuntimeStatisticsURI` and `createConsoleMessagesURI` end up — and
both do `resourceType.toString.toLowerCase`. What travels on the wire is
`runtimestatistics` and `consolemessages`. Scala only round-trips it because
its own decode lowercases both sides (`:142-144`, `.find(_.toString.toLowerCase
== resourceTypeStr)`).
Against the enum verbatim:
```
segment before (.lower()) after (verbatim)
result / state OK OK
runtimestatistics ValueError ValueError <- what Scala sends
runtimeStatistics ValueError OK <- what the new
test sends
```
So the behaviour is unchanged for every URI that exists today. The new test
builds its URI from `resource_type.value`, i.e. it asserts the Python enum
against itself, which is why it cannot see the disagreement.
There is still something real underneath, just smaller and pointed the other
way: the enum claims to mirror the shared protocol and two of its four values
do not match what that protocol encodes. Nothing reads them today, but Scala
already writes those URIs and pyamber already reads VFS URIs, so the first
Python consumer to reach for `VFSResourceType.RUNTIME_STATISTICS` would
silently never match. Aligning the two values with what is encoded — and
keeping the `.lower()` — closes that in two lines, and a test decoding a
literal `.../runtimestatistics` would pin it.
Worth updating #8237 alongside: its premise ("Python rejects Scala-produced
... URIs" because they are camel case) and its repro both describe the inverse
of what happens.
--
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]
