rusackas opened a new pull request, #43101: URL: https://github.com/apache/superset/pull/43101
### SUMMARY `DuckDBEngineSpec.fetch_data` (added in #34831) overrode the base implementation to capture `cursor.description` before `fetchall()` and restore it afterward, working around a reported duckdb-engine bug ([Mause/duckdb_engine#1322](https://github.com/Mause/duckdb_engine/issues/1322)) where `cursor.description` allegedly became `None` after fetching. I couldn't reproduce this at the layer the override actually touches (a raw DBAPI cursor obtained via `conn.cursor()`), across every combination I could test: - duckdb-engine 0.12.1 and 0.17.0 - SQLAlchemy 1.4.54 and 2.0.51 (both `master`'s pre- and post-#42803 pins) - The real `DuckDBEngineSpec.execute`/`fetch_data` methods against current pinned versions (`duckdb==1.5.5`, `duckdb-engine==0.17.0`, `sqlalchemy==2.0.51`) The original issue's own repro checks `result.cursor.description` via `Connection.execute()`, and does show `None` — but the same thing happens with plain sqlite3 through SQLAlchemy, so it looks like generic SQLAlchemy `CursorResult` behavior (it releases its cached cursor reference after fully buffering rows) rather than a duckdb-engine defect. I posted these findings on the upstream issue: https://github.com/Mause/duckdb_engine/issues/1322#issuecomment-5276379065 Since the workaround appears to be defending against something that doesn't reproduce, this removes it and adds a regression test confirming the inherited base `fetch_data` correctly preserves `cursor.description` against a real DuckDB cursor. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A - internal engine spec change, no UI impact. ### TESTING INSTRUCTIONS - `pytest tests/unit_tests/db_engine_specs/test_duckdb.py -v` - New test `test_fetch_data_preserves_cursor_description` exercises a real in-memory DuckDB cursor through `DuckDBEngineSpec.execute`/`fetch_data` (no override) and asserts `cursor.description` is still populated after `fetchall()`. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
