dbtsai commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4858919668
## Review feedback
Two issues in `PathAwareChannelBuilder._extract_attributes`
(`python/pyspark/sql/connect/client/core.py`).
### 1. IPv6 endpoints are rejected (`core.py:667`)
`netloc = self.url.netloc.split(":")` with `len(netloc) in (1, 2)` breaks on
IPv6 addresses:
- `sc://[::1]:15002/path1` -> `netloc='[::1]:15002'` -> `split(':')` yields
`['[', '', '1]', '15002']` (4 parts) -> falls into the `else` branch and raises
`INVALID_CONNECT_URL`.
- `sc://[::1]/path1` -> `['[', '', '1]']` (3 parts) -> same failure.
`DefaultChannelBuilder` (`core.py:486-495`) handles this correctly by using
`self.url.hostname` / `self.url.port` and bracket-wrapping IPv6 hosts, so this
new builder is a functional regression for IPv6 users. Two related issues on
the same path:
- `sc://host:abc/p` -> `int(netloc[1])` raises a raw `ValueError` instead of
the wrapped `PySparkValueError`.
- `sc://:15002/p` -> `['', '15002']` (2 parts) -> `host=''` is silently
accepted.
Suggested fix: parse host/port via `self.url.hostname` / `self.url.port` as
`DefaultChannelBuilder` does (correct IPv6 bracket-wrapping, hostname
validation, consistent error path), and derive `netloc_has_port` from
`self.url.port is not None`.
### 2. Path port is lost when combined with the standard `/;params` form
(`core.py:688-700`)
The trailing-path-port syntax loses the intended port when users combine it
with Spark's existing `/;param=value` form. For example
`sc://gateway/app/driver:443/;token=abc` parses (via `urlparse`) to
`path='/app/driver:443/'`, `params='token=abc'`. Then:
- `last_segment = prefix.rsplit("/", 1)[-1]` on `/app/driver:443/` -> `''`
(the trailing slash leaves an empty last segment).
- `":" in last_segment` is therefore `False`, so the port-extraction block
(lines 693-700) is skipped.
- Result: `self._port` stays at the fallback `15002`, and `_path_prefix`
becomes `/app/driver:443` with `:443` left literally in the prefix.
So the intended port 443 is silently dropped. This is the documented
standard param form (the class docstring advertises
`sc://host[:port][/;params]`), so combining it with path routing is a natural
usage. Without the trailing slash (`sc://gateway/app/driver:443;token=abc`) it
works correctly (`last_segment='driver:443'`, port 443 extracted), which
confirms the trailing `/` before `;` is the trigger.
Suggested fix: parse the last non-empty path segment (strip a trailing `/`
before `rsplit`) so the port is recognized. Given the docstring already
promises the `/;params` form, fixing the parser seems preferable to documenting
the limitation.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
