Vamsi-klu commented on PR #63575: URL: https://github.com/apache/airflow/pull/63575#issuecomment-4061376197
Hey @justinpakzad, the implementation looks clean — the conditional inclusion with `if timeout is not None` is the right pattern so Snowflake falls back to the session-level `STATEMENT_TIMEOUT_IN_SECONDS` when unset. A few things I wanted to flag: **Missing docstring entry on the operator.** The `SnowflakeSqlApiOperator` class docstring documents every `__init__` parameter except the new `timeout`. Adding a `:param timeout:` entry would keep it consistent and discoverable. **No negative test for timeout=None.** The new test covers the positive case (timeout=120 in payload), but there's no explicit test verifying that when `timeout` is `None`, the key is *excluded* from the JSON body. The existing parametrized test implicitly covers this, but an explicit assertion like `assert "timeout" not in data` would make the intent clear — especially since sending `timeout: null` to Snowflake could behave differently than omitting the field entirely. **No operator-level test.** The test only validates the hook. Adding a test that verifies `SnowflakeSqlApiOperator(timeout=120)` correctly passes the value through to `execute_query` would close the gap between the two layers. **Worth considering: `template_fields`.** If users might want to parameterize the timeout at the DAG level (e.g., different timeouts for dev vs prod), adding `"timeout"` to the operator's `template_fields` tuple would make that possible. Not a blocker, just a thought. The core implementation is solid — minimal diff, correct placement in the API payload, and the `timeout=0` semantics (max timeout) are properly documented in the docstring. Nice work. -- 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]
