Re: [PR] Add support for non-deferred pagination mode in GenericTransfer [airflow]

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

Copilot commented on code in PR #64321:
URL: https://github.com/apache/airflow/pull/64321#discussion_r3025332234


##########
providers/common/sql/src/airflow/providers/common/sql/operators/generic_transfer.pyi:
##########
@@ -55,6 +55,7 @@ class GenericTransfer(BaseOperator):
     preoperator: Incomplete
     insert_args: Incomplete
     page_size: Incomplete
+    deferrable: bool
     def __init__(
         self,

Review Comment:
   The public `.pyi` stub for `GenericTransfer` still does not include `paginated_sql_statement_clause` as an attribute/constructor parameter, even though the implementation and tests support it. This can break MyPy for provider code that uses custom pagination clauses. Please add `paginated_sql_statement_clause` to the stub (attribute and `__init__` signature) to match `generic_transfer.py`.



##########
providers/common/sql/src/airflow/providers/common/sql/operators/generic_transfer.pyi:
##########
@@ -68,6 +69,7 @@ class GenericTransfer(BaseOperator):
         preoperator: str | list[str] | None = None,
         insert_args: dict | None = None,
         page_size: int | None = None,
+        deferrable: bool = False,

Review Comment:
   In the `.pyi` stub, `deferrable` is declared with a default of `False`, but the runtime default is configuration-driven (`default_deferrable`). To avoid misleading API consumers, consider using an unspecified default in the stub (e.g., `deferrable: bool = ...`) or aligning the stub default with the implementation’s effective behavior.
   ```suggestion
           deferrable: bool = ...,
   ```



##########
providers/common/sql/src/airflow/providers/common/sql/operators/generic_transfer.py:
##########
@@ -90,6 +91,7 @@ def __init__(
         insert_args: dict | None = None,
         page_size: int | None = None,
         paginated_sql_statement_clause: str | None = None,
+        deferrable: bool = conf.getboolean("operators", "default_deferrable", fallback=False),
         **kwargs,
     ) -> None:

Review Comment:
   `deferrable` now defaults to `conf.getboolean("operators", "default_deferrable", fallback=False)`. Previously, pagination (`page_size` + single SQL string) always deferred; with the current default (commonly `False`), existing DAGs using `page_size` will switch to non-deferrable execution and occupy a worker slot. If the intention is to *add* a non-deferrable option without changing defaults, consider keeping the previous behavior (e.g., default `deferrable=True` for paginated mode, or accept `deferrable: bool | None = None` and treat `None` as “preserve legacy behavior”). Also add/adjust a unit test to lock in the intended default.



-- 
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: commits-unsubscribe@airflow.apache.org

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