sidshas03 commented on PR #55694:
URL: https://github.com/apache/airflow/pull/55694#issuecomment-3300721894
**Final update — all changes consolidated in `c8e7998`.**
### What this PR fixes
* Ensures **`AthenaSQLHook`** correctly handles `hook_params` with clear
precedence:
**hook\_params > connection extras** for `s3_staging_dir`, `work_group`,
`driver`, `aws_domain`.
### What I changed
1. **Parity in `get_conn()`**
* Explicitly set:
* `conn_kwargs["driver"] = self.driver` (when provided)
* `conn_kwargs["aws_domain"] = self.aws_domain` (when provided)
* Keeps `get_conn()` consistent with `_get_conn_params()` / `get_uri()`.
2. **Constructor refactor (explicit, IDE-friendly API)**
* Switched from the `kwargs.pop(...)` pattern to **explicit optional
`__init__` params**:
`s3_staging_dir`, `work_group`, `driver`, `aws_domain`,
`session_kwargs`, `config_kwargs`,
`role_arn`, `assume_role_method`, `assume_role_kwargs`,
`aws_session_token`, `endpoint_url`.
* **Backward compatibility kept:** `athena_conn_id` restored as
**positional-or-keyword** param; remaining fields are keyword-only. `**kwargs`
retained for compatibility.
3. **Tests**
* Added **`test_hook_params_override_extras()`** to assert the precedence
(hook\_params override extras).
* Test hygiene: removed issue references, fixed imports, and used a
dynamic BaseHook patch path.
4. **Formatting / static checks**
* Ran `prek --all-files` to resolve trailing whitespace, spacing, Ruff
formatting.
* No functional changes—just style fixes to satisfy static checks.
### Verification
* **Static checks:** pass locally (Breeze / `prek`).
* **Targeted unit tests (Amazon Non-DB):** pass locally in Breeze.
### Commit timeline (for reference)
* **198c5e1** – Parity in `get_conn()`, new precedence test, and test
cleanup.
* **dbe9916** – First pass switching to explicit `__init__`.
* **f006c57** – Restored BC with positional-or-keyword `athena_conn_id`.
* **c8e7998** – Amended `f006c57` with formatting fixes (no logic change) →
**final state**.
If you prefer a different parameter order/naming or want docstrings expanded
for the new params, I can follow up quickly.
Sorry for the mess, recently I have started this opensource and I am trying
to improve. Sorry for so much confusion.
Thank you so much @vincbeck @o-nikolas @jroachgolf84 - please kindly review
this and let me know your feedbacks or any changes required.
--
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]
