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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
