Re: [PR] [SPARK-60175][PYTHON] Improve type annotations and kwargs handling [spark]

["HyukjinKwon (via GitHub)" <gi...@apache.org>]

HyukjinKwon commented on code in PR #59380:
URL: https://github.com/apache/spark/pull/59380#discussion_r4257588740


##########
python/pyspark/pandas/spark/accessors.py:
##########
@@ -815,8 +817,8 @@ def to_spark_io(
 
         >>> df.spark.to_spark_io(path='%s/to_spark_io/foo.json' % path, format='json')
         """
-        if "options" in options and isinstance(options.get("options"), dict) and len(options) == 1:
-            options = options.get("options")  # type: ignore[assignment]
+        if options is None:

Review Comment:
   **Non-blocking (P2):** Same issue here, and it is also reachable through `DataFrame.to_delta` / `to_orc`. They only unwrap `options=` when it is the sole keyword, so `psdf.to_delta(path, options={'mergeSchema': 'true'}, overwriteSchema='true')` now silently loses `overwriteSchema`, which was applied before this PR. Likewise `psdf.spark.to_spark_io(path, format='json', options={}, compression='gzip')` writes uncompressed output. Merging the two forms (`{**(options or {}), **kwargs}`) fixes both paths.
   
   See **Shared repair plan 1** in the review body.



##########
python/pyspark/pandas/spark/accessors.py:
##########
@@ -744,8 +745,8 @@ def to_table(
 
         >>> df.to_table('%s.my_table' % db, partition_cols='date')
         """
-        if "options" in options and isinstance(options.get("options"), dict) and len(options) == 1:
-            options = options.get("options")  # type: ignore[assignment]
+        if options is None:

Review Comment:
   **Non-blocking (P2):** When a caller passes both an `options` dict and keyword options, the keywords are now silently dropped. `psdf.spark.to_table('t', options={'a': 'b'}, compression='snappy')` only sends `a` to `saveAsTable`, and `options={}` plus keywords sends nothing. Before this change those keywords did reach the writer. `DataFrame.to_table` forwards `**options` unchanged, so it hits this too. Merging both forms keeps the typing fix and keeps every option, with keywords winning on a conflict:
   
   ```python
   options = {**(options or {}), **kwargs}
   ```
   
   See **Shared repair plan 1** in the review body.



##########
python/pyspark/pandas/spark/accessors.py:
##########
@@ -684,7 +684,8 @@ def to_table(
         mode: str = "overwrite",
         partition_cols: Optional[Union[str, List[str]]] = None,
         index_col: Optional[Union[str, List[str]]] = None,
-        **options: "OptionalPrimitiveType",
+        options: Optional[Dict[str, "OptionalPrimitiveType"]] = None,
+        **kwargs: "OptionalPrimitiveType",

Review Comment:
   **Nit (P3):** The Parameters sections of `to_table` and `to_spark_io` still describe only `options` ("Additional options passed directly to Spark." / `options : dict`). They have no entry for the new `**kwargs` and don't say how the two forms combine. `DataFrame.to_table` also reuses this docstring with its own `**options` signature. Could you document `options` as an optional dict and `**kwargs` as extra options merged into it?
   
   See **Shared repair plan 1** in the review body.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org