hiboyang commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4859971286
> ## 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.
Thanks for the comments! Will make these changes after I get initial PR
check passing.
--
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]
