o-nikolas commented on PR #71619: URL: https://github.com/apache/airflow/pull/71619#issuecomment-5458109493
> > > Thanks so much for catching those handler and multi-statement issues, @o-nikolas! Really appreciate you (and your AI agent 😄) taking the time to dig into pyexasol's ExaStatement internals — that's a subtle one I definitely wouldn't have caught on my own. Just pushed a fix for everything: > > > ``` > > > * Switched to `exasol_fetch_all_handler` on all three SQLExecuteQueryOperator tasks > > > > > > * Split the CREATE TABLE + INSERT into separate tasks so we're not relying on multi-statement execution > > > > > > * Removed the now-stale skip entry in test_project_structure.py (since we finally have an example!) > > > > > > * Added the missing how-to-guide link in provider.yaml, and synced get_provider_info.py to match > > > This is my first Airflow contribution, so thanks for the patience walking through it! Whenever someone with write access gets a chance, the workflow run is waiting on approval to actually execute. No rush at all > > > ``` > > > > > > Thanks for the fixes! Have you run this against a live Exasol+AWS environment for testing? > > Honest answer: no, I haven't run this against a live Exasol instance. I don't have an Exasol account, and tried spinning up the official exasol/docker-db image locally tonight, but ran into some Docker/WSL2 setup issues and then network connectivity problems reaching Docker Hub from where I am. Wasn't able to get a full live run working. > > What I did verify through code review: > > * pyexasol's ExaStatement has no `description` attribute, confirming why the default handler would raise a RuntimeError > > * exasol_fetch_all_handler's implementation switches on `statement.result_type` instead, matching how example_exasol.py uses it > > * ExasolHook.run() defaults to split_statements=False > > * The existing test_exasol_to_s3.py unit tests still pass > > > So this is verified by code reading + existing test coverage, not an actual live run. Happy to try again if there's a better way to get test access, but wanted to give you an honest answer rather than pretend I'd tested it. I appreciate the honesty :slightly_smiling_face: @ferruzzi and @vincbeck what do we think, I'm inclined to not merge this one if it hasn't ultimately been tested. But I'm open to being convinced otherwise. -- 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]
