Re: [PR] Bump `clickhouse-connect>=1.3.0` [airflow]

["koletzilla (via GitHub)" <gi...@apache.org>]

koletzilla commented on code in PR #68400:
URL: https://github.com/apache/airflow/pull/68400#discussion_r3426875226


##########
providers/clickhousedb/tests/unit/clickhousedb/hooks/test_clickhouse.py:
##########
@@ -999,6 +999,53 @@ def test_insert_rows_executemany_executes_once(self):
         assert len(values) == 3
 
 
+# ---------------------------------------------------------------------------
+# Regression: executemany=True with tuple rows (clickhouse-connect >= 1.3.0)
+#
+# clickhouse_connect < 1.3.0 raised:
+#   AttributeError: 'tuple' object has no attribute 'keys'
+# because Cursor._try_bulk_insert called data[0].keys() without checking
+# whether the rows were mappings or sequences (PEP 249 allows both).
+# ---------------------------------------------------------------------------
+
+
+class TestInsertRowsExecutemanyRegression:

Review Comment:
   We added the tests in the upstream code (https://github.com/ClickHouse/clickhouse-connect/pull/795) but we wanted to also have some check here to make sure the fix was actually fixing our issue, but agree it feels a bit redundant. I have removed them.
   
   We talked about adding some integration tests in the future in this repo with a proper dockerized ClickHouse instance. Maybe we can add back some of these tests but instead of mocking, making the calls directly to this instance.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org