ahilashsasidharan commented on PR #70831:
URL: https://github.com/apache/airflow/pull/70831#issuecomment-5474452130

   > Thanks for the clear write-up — the reasoning matches the criterion in 
#70503 exactly, and it follows the same restoration already merged for the 
Repos operators in #70551.
   > 
   > I ran a differential against a live SQL warehouse. Provider 7.18.1 already 
contains #70340, so the unpatched wheel reproduces current `main` and the 
patched one carries your two edits (Airflow 3.2.2, real `warehouse_id`, real 
statements executed):
   > case       current main    with this PR
   > `statement` + templated `statement_id` rendering to `None` 
(`render_template_as_native_obj=True`)  task **succeeds** — the contradictory 
pair is silently ignored and the statement runs   Dag import error
   > `statement="SELECT 1"`, `statement_id=""`  task **succeeds** against the 
warehouse         Dag import error
   > `statement=""`, `statement_id=""`  `execute()`: "One of either statement 
or statement_id must be provided."        `__init__`: "Cannot provide both 
statement and statement_id."
   > `statement` only / `statement_id` only     constructed     constructed
   > 
   > The first row is an argument for this change that the description does not 
make: on `main`, supplying both arguments produces no error at all once one of 
them renders to `None`, and the sensor quietly runs the other. Catching that at 
parse time is a real improvement rather than a relocation — worth saying in the 
description.
   > 
   > The second row is a user-visible behaviour change: `statement_id=""` 
alongside a `statement` works today (confirmed end to end) and becomes a Dag 
import error here. Your parametrization shows it is deliberate, so this is only 
about making it visible to whoever reads the changelog later.
   > 
   > Nits inline, none blocking.
   > 
   > Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting
   
   Thanks! I've updated the PR description to note both changes. Let me know if 
anything else is needed to make it visible in the changelog.


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

Reply via email to