[SPARK-57760][SS] Make small optimizations to StatefulProcessorApiClient#56786
[SPARK-57760][SS] Make small optimizations to StatefulProcessorApiClient#56786funrollloops wants to merge 3 commits into
Conversation
HyukjinKwon
left a comment
There was a problem hiding this comment.
2 blocking, 0 non-blocking, 1 nit.
The closure-hoisting optimization is good; the serializer swap and the fast-path subclass leak read as unintended behavior changes for a "small optimizations" PR.
Correctness (2)
- stateful_processor_api_client.py:110:
PickleSerializer()drops cloudpickle —CPickleSerializerdefaults toCloudPickleSerializer— see inline - stateful_processor_api_client.py:45: scalar fast-path returns
np.float64/pd.Timestampunconverted (they subclassfloat/datetime) — see inline
Nits: 1 minor item (see inline comments).
Verification
Confirmed empirically: issubclass(np.float64, float) and issubclass(pandas.Timestamp, datetime) are both True, while np.int64/np.bool_ are not subclasses of int/bool. So the new fast-path leaks exactly np.float64 and pd.Timestamp (returned as-is, skipping .tolist()/.to_pydatetime()), while np.int64/np.bool_ still correctly reach the np.generic branch.
PR description suggestions
- Document: why the serializer is changed from
CPickleSerializertoPickleSerializer(this is a capability change, not just an optimization) — and whether dropping cloudpickle is intended. - Add: a real JIRA id (currently
SPARK-?????) and a short note on what is being optimized and how it was measured.
Benchmarked a tuple vs a frozenset, and the tuple came out noticeably faster.
|
Hi @HyukjinKwon, I'm waiting on my Jira account to be created. I'll create a Jira ticket for this PR once I am able. Attached is the script I used to benchmark the _serialize_to_bytes function. |
|
Thanks! Merging to master/4.x. |
### What changes were proposed in this pull request? Make two small optimizations to StatefulProcessorApiClient: 1. Call PickleSerializer() instead of using the default CPickleSerializer (which is CloudPickleSerializer). We don't need the latter since this path does not deal with code objects. 2. Micro-optimize state value normalization: add a fast-path for primitives, prefer `map` to generator comprehensions, and move the numpy import and function definition to the top level so it is done once #### Benchmarks This is a [microbenchmark for `_serialize_to_bytes`](https://gist.github.com/funrollloops/62bf5fae1654910b63a4539e1181db91): ##### Before ``` --- single-field tuples --- python int → LongType p50= 4.11µs p95= 4.21µs p99= 6.49µs python float → DoubleType p50= 4.10µs p95= 4.20µs p99= 4.96µs np.float64 → DoubleType p50= 4.42µs p95= 4.57µs p99= 6.46µs np.datetime64 → Timestamp p50= 7.32µs p95= 7.67µs p99= 11.60µs pd.Timestamp → Timestamp p50= 7.53µs p95= 7.76µs p99= 12.34µs --- wider tuples --- 10× python float → 10× DoubleType p50= 7.80µs p95= 7.94µs p99= 12.37µs 10× np.float64 → 10× DoubleType p50= 9.01µs p95= 9.23µs p99= 14.88µs mixed (np.f64, np.i64, str, pd.Ts) p50= 9.93µs p95= 10.26µs p99= 17.66µs ``` ##### After ``` --- single-field tuples --- python int → LongType p50= 1.17µs p95= 1.19µs p99= 1.22µs python float → DoubleType p50= 1.18µs p95= 1.19µs p99= 1.22µs np.float64 → DoubleType p50= 1.92µs p95= 1.98µs p99= 2.01µs np.datetime64 → Timestamp p50= 4.59µs p95= 4.71µs p99= 4.78µs pd.Timestamp → Timestamp p50= 5.07µs p95= 5.17µs p99= 5.24µs --- wider tuples --- 10× python float → 10× DoubleType p50= 2.19µs p95= 2.23µs p99= 2.26µs 10× np.float64 → 10× DoubleType p50= 7.72µs p95= 7.82µs p99= 7.89µs mixed (np.f64, np.i64, str, pd.Ts) p50= 7.34µs p95= 7.46µs p99= 7.58µs ``` ### Why are the changes needed? Together these changes improve transform with state on a simple rolling-window style benchmark by ~10%. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing unit tests. ### Was this patch authored or co-authored using generative AI tooling? No, but Claude was consulted in the process of producing this PR. Closes #56786 from funrollloops/tws-opt-1. Lead-authored-by: Sagar Mittal <75583+funrollloops@users.noreply.github.com> Co-authored-by: Sagar Mittal <sagar.mittal@databricks.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com> (cherry picked from commit 0af85ee) Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>
…evel in stateful_processor_api_client ### What changes were proposed in this pull request? This PR makes `numpy` a lazy import in `stateful_processor_api_client.py` again. The fix introduces a dedicated `_load_numpy()` function that resolves numpy availability on first use (called from `_serialize_to_bytes`), and caches the result in module-level `has_numpy` / `np` variables. The `_normalize_state_value` function itself remains free of any import or initialization logic, keeping the hot path lean. ### Why are the changes needed? SPARK-57760 (#56786) moved `import numpy` to the top level of `stateful_processor_api_client.py` for micro-optimization. However, this module is loaded transitively by `import pyspark` via: import pyspark -> pyspark.sql -> pyspark.sql.streaming -> pyspark.sql.streaming.stateful_processor -> pyspark.sql.streaming.stateful_processor_api_client -> import numpy This causes `test_import_spark_libraries` to fail because the test asserts that `import pyspark` does not pull in any third-party packages other than py4j. https://github.com/apache/spark/actions/runs/29027743228/job/86156618984 ``` pyspark.tests.test_import_spark with python3.12 failed: Running tests... ---------------------------------------------------------------------- test_import_spark_libraries (pyspark.tests.test_import_spark.ImportSparkTest.test_import_spark_libraries) We want to ensure "import pyspark" is fast. It matters when we spawn ... FAIL (0.252s) ====================================================================== FAIL [0.252s]: test_import_spark_libraries (pyspark.tests.test_import_spark.ImportSparkTest.test_import_spark_libraries) We want to ensure "import pyspark" is fast. It matters when we spawn ---------------------------------------------------------------------- Traceback (most recent call last): File "/__w/spark/spark/python/pyspark/tests/test_import_spark.py", line 61, in test_import_spark_libraries self.fail( AssertionError: Unexpected 3rd party package 'numpy' imported during 'import pyspark' ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - `pyspark.tests.test_import_spark.ImportSparkTest.test_import_spark_libraries` now passes. - Verified that `_normalize_state_value` correctly normalizes numpy scalars and nested containers (list/tuple/dict/namedtuple/Row) after `_load_numpy()` is triggered. - Confirmed `import pyspark` no longer imports numpy via `-X importtime`. ### Was this patch authored or co-authored using generative AI tooling? Kiro CLI / Claude Closes #57163 from sarutak/fix-numpy-import-time. Authored-by: Kousuke Saruta <sarutak@apache.org> Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com>
…evel in stateful_processor_api_client ### What changes were proposed in this pull request? This PR makes `numpy` a lazy import in `stateful_processor_api_client.py` again. The fix introduces a dedicated `_load_numpy()` function that resolves numpy availability on first use (called from `_serialize_to_bytes`), and caches the result in module-level `has_numpy` / `np` variables. The `_normalize_state_value` function itself remains free of any import or initialization logic, keeping the hot path lean. ### Why are the changes needed? SPARK-57760 (#56786) moved `import numpy` to the top level of `stateful_processor_api_client.py` for micro-optimization. However, this module is loaded transitively by `import pyspark` via: import pyspark -> pyspark.sql -> pyspark.sql.streaming -> pyspark.sql.streaming.stateful_processor -> pyspark.sql.streaming.stateful_processor_api_client -> import numpy This causes `test_import_spark_libraries` to fail because the test asserts that `import pyspark` does not pull in any third-party packages other than py4j. https://github.com/apache/spark/actions/runs/29027743228/job/86156618984 ``` pyspark.tests.test_import_spark with python3.12 failed: Running tests... ---------------------------------------------------------------------- test_import_spark_libraries (pyspark.tests.test_import_spark.ImportSparkTest.test_import_spark_libraries) We want to ensure "import pyspark" is fast. It matters when we spawn ... FAIL (0.252s) ====================================================================== FAIL [0.252s]: test_import_spark_libraries (pyspark.tests.test_import_spark.ImportSparkTest.test_import_spark_libraries) We want to ensure "import pyspark" is fast. It matters when we spawn ---------------------------------------------------------------------- Traceback (most recent call last): File "/__w/spark/spark/python/pyspark/tests/test_import_spark.py", line 61, in test_import_spark_libraries self.fail( AssertionError: Unexpected 3rd party package 'numpy' imported during 'import pyspark' ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - `pyspark.tests.test_import_spark.ImportSparkTest.test_import_spark_libraries` now passes. - Verified that `_normalize_state_value` correctly normalizes numpy scalars and nested containers (list/tuple/dict/namedtuple/Row) after `_load_numpy()` is triggered. - Confirmed `import pyspark` no longer imports numpy via `-X importtime`. ### Was this patch authored or co-authored using generative AI tooling? Kiro CLI / Claude Closes #57163 from sarutak/fix-numpy-import-time. Authored-by: Kousuke Saruta <sarutak@apache.org> Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com> (cherry picked from commit e4c894a) Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com>
…all` module ### What changes were proposed in this pull request? Move `test_import_spark` to `pyspark_install` module from `pyspark_core`. ### Why are the changes needed? 1. `test_import_spark` should be triggered for any changes in pyspark, including streaming/ml etc. `test_install` currently does that. 2. `test_install` is cheap because it is not executed in post-merge. It would be executed in scheduled CI only. We failed to catch the import issue from #56786 because the test did not run. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #57164 from gaogaotiantian/move-import-check. Authored-by: Tian Gao <gaogaotiantian@hotmail.com> Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com>
…all` module ### What changes were proposed in this pull request? Move `test_import_spark` to `pyspark_install` module from `pyspark_core`. ### Why are the changes needed? 1. `test_import_spark` should be triggered for any changes in pyspark, including streaming/ml etc. `test_install` currently does that. 2. `test_install` is cheap because it is not executed in post-merge. It would be executed in scheduled CI only. We failed to catch the import issue from #56786 because the test did not run. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #57164 from gaogaotiantian/move-import-check. Authored-by: Tian Gao <gaogaotiantian@hotmail.com> Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com> (cherry picked from commit 1935ee0) Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com>
What changes were proposed in this pull request?
Make two small optimizations to StatefulProcessorApiClient:
mapto generator comprehensions, and move the numpy import and function definition to the top level so it is done onceBenchmarks
This is a microbenchmark for
_serialize_to_bytes:Before
After
Why are the changes needed?
Together these changes improve transform with state on a simple rolling-window style benchmark by ~10%.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing unit tests.
Was this patch authored or co-authored using generative AI tooling?
No, but Claude was consulted in the process of producing this PR.