dongjoon-hyun commented on PR #57834:
URL: https://github.com/apache/spark/pull/57834#issuecomment-5242190451
Two comments on the Python client change in
`python/pyspark/sql/connect/client/core.py`:
**1. `self._builder.metadata()` is iterated twice in `__init__` — a one-shot
iterable silently drops all connection metadata**
```python
metadata = self._builder.metadata()
if any(key.lower() == _OPERATION_ID_METADATA_KEY for key, _ in metadata):
...
self._metadata = [
(key, value) for key, value in metadata if key.lower() !=
_OPERATION_ID_METADATA_KEY
]
```
`ChannelBuilder.metadata()` is typed as `Iterable[Tuple[str, str]]`, and
`ChannelBuilder` is a public extension point (SPARK-43351). If a custom
subclass returns a generator, the `any(...)` check consumes it and the
subsequent list comprehension sees an empty iterable, so all connection
metadata (including auth headers) is silently dropped. Materializing it first
would make this safe:
```python
metadata = list(self._builder.metadata())
```
**2. Snapshotting metadata at client construction is a behavior change for
custom `ChannelBuilder`s**
Previously every RPC called `self._builder.metadata()` at call time, so a
custom `ChannelBuilder` that overrides `metadata()` to return fresh values
(e.g. a refreshed auth token) worked. With this PR the metadata is read once in
`__init__` and cached in `self._metadata`, so such implementations would now
send stale values for the lifetime of the client.
The Scala client has always snapshotted metadata at channel creation, so
this does bring the two clients into parity — but on the Python side it is a
silent regression for dynamic-metadata builders. Was this intentional? If not,
an alternative is to keep reading `self._builder.metadata()` per call and apply
the operation-id filter in a small helper (the filtering cost is negligible).
If it is intentional, it would be good to call the behavior change out in the
PR description.
--
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]
