szehon-ho commented on code in PR #57412:
URL: https://github.com/apache/spark/pull/57412#discussion_r3632737306


##########
python/pyspark/pipelines/api.py:
##########
@@ -652,16 +673,31 @@ def create_auto_cdc_flow(
             },
         )
 
-    if stored_as_scd_type is not None and str(stored_as_scd_type) != "1":
+    if stored_as_scd_type is not None and str(stored_as_scd_type) not in ("1", 
"2"):
         raise PySparkTypeError(
             errorClass="NOT_EXPECTED_TYPE",
             messageParameters={
                 "arg_name": "stored_as_scd_type",
-                "expected_type": "Literal[1, '1']",
+                "expected_type": "Literal[1, 2, '1', '2']",
                 "arg_type": type(stored_as_scd_type).__name__,
             },
         )
 
+    # Track-history columns describe how consecutive upserts are coalesced 
into history records,
+    # a notion that only exists under SCD2. Reject them for any other SCD 
type. The engine
+    # enforces this too, but failing here gives a clearer, client-side error.
+    is_scd2 = stored_as_scd_type is not None and str(stored_as_scd_type) == "2"
+    if not is_scd2 and (
+        track_history_column_list is not None or 
track_history_except_column_list is not None
+    ):
+        raise PySparkValueError(
+            errorClass="INVALID_MULTIPLE_ARGUMENT_CONDITIONS",
+            messageParameters={
+                "arg_names": "track_history_column_list, 
track_history_except_column_list",
+                "condition": "specified unless stored_as_scd_type is 2 (or 
'2')",

Review Comment:
   Minor wording nit: the rendered message always names both parameters. With 
the template `"[<arg_names>] cannot be <condition>."`, a caller who passed only 
`track_history_column_list` still gets `[track_history_column_list, 
track_history_except_column_list] cannot be specified unless stored_as_scd_type 
is 2 (or '2').` Accurate as a rule about the pair, but less precise than naming 
the argument actually supplied. Optional.



##########
python/pyspark/pipelines/api.py:
##########
@@ -652,16 +673,31 @@ def create_auto_cdc_flow(
             },
         )
 
-    if stored_as_scd_type is not None and str(stored_as_scd_type) != "1":
+    if stored_as_scd_type is not None and str(stored_as_scd_type) not in ("1", 
"2"):
         raise PySparkTypeError(
             errorClass="NOT_EXPECTED_TYPE",
             messageParameters={
                 "arg_name": "stored_as_scd_type",
-                "expected_type": "Literal[1, '1']",
+                "expected_type": "Literal[1, 2, '1', '2']",
                 "arg_type": type(stored_as_scd_type).__name__,
             },
         )
 
+    # Track-history columns describe how consecutive upserts are coalesced 
into history records,
+    # a notion that only exists under SCD2. Reject them for any other SCD 
type. The engine
+    # enforces this too, but failing here gives a clearer, client-side error.
+    is_scd2 = stored_as_scd_type is not None and str(stored_as_scd_type) == "2"
+    if not is_scd2 and (

Review Comment:
   The empty-list case is treated as "provided". This gate uses `is not None`, 
so `track_history_column_list=[]` (or `[]` for the except variant) raises 
`INVALID_MULTIPLE_ARGUMENT_CONDITIONS` when the flow is not SCD2 -- even though 
an empty list serializes identically to an omitted one (`to_plans([])` -> `[]` 
-> unset repeated field on the wire) and is thus a no-op. Consider gating on 
non-empty (e.g. truthiness of the lists) so an empty list is accepted/ignored 
consistently with how it is serialized.



##########
python/pyspark/pipelines/api.py:
##########
@@ -627,6 +641,13 @@ def create_auto_cdc_flow(
     except_column_list = _normalize_optional_column_list(
         arg_name="except_column_list", column_list=except_column_list
     )
+    track_history_column_list = _normalize_optional_column_list(

Review Comment:
   Asymmetry with the docstring's "Only one ... can be specified" promise: the 
client validates the SCD2 requirement, but specifying *both* 
`track_history_column_list` and `track_history_except_column_list` is only 
rejected server-side 
(`AUTOCDC_BOTH_TRACK_HISTORY_COLUMN_LIST_AND_EXCEPT_COLUMN_LIST` in 
`PipelinesHandler.scala`). This mirrors the existing 
`column_list`/`except_column_list` handling (also server-only), so it is a 
consistent choice -- flagging only because the "both" case costs a server 
round-trip. Fine to leave as-is for consistency.



-- 
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]

Reply via email to