Custom fetch all handler for vertica to not miss errors #34041
Conversation
|
|
||
| def vertica_fetch_all_handler(cursor) -> list[tuple] | None: | ||
| """Replace the default DbApiHook fetch_all_handler .""" | ||
| to_return = fetch_all_handler(cursor) |
There was a problem hiding this comment.
Could we rename to_return? Does rows make sense as the name of this value? or result might be a bit better than to_return
There was a problem hiding this comment.
ok I'll rename it as result
| if cursor.description is not None: | ||
| while cursor.nextset(): | ||
| if cursor.description is not None: | ||
| row = cursor.fetchone() | ||
| while row: | ||
| row = cursor.fetchone() | ||
| return to_return |
There was a problem hiding this comment.
Will this value be changed during the execution of lines 31-36? It seems this variable is not touched after it is assigned. But I guess this is something related to cursor?
There was a problem hiding this comment.
Returned value will not change between line 31 to 37, all this code is here to make vertica client throws error.
If you run the following sql:
INSERT INTO MyTable (Key, Label) values (1, 'test 1');
INSERT INTO MyTable (Key, Label) values (1, 'test 2');
INSERT INTO MyTable (Key, Label) values (3, 'test 3');
each insert has its own result set and if you don't try to fetch data of thoses result sets you won't detect error on the second insert.
There was a problem hiding this comment.
Yes, fortunately that as beginning with airflow I tested that it worked as I thincked, but if I had the habit with other database before I may have ended with a really bad surprise.
|
Interesting ... LGTM. |
There was a problem hiding this comment.
One change request though: : could you please add some comments (docstring) in VerticaHook explaining why we have this and explaining why we have this complexity in the Vertica Hook. It should contain link to the issue that you have in the commit message as well as some explanation for those who would like to use their own handler for Vertica Hook.
I think without it it will be difficult for future maintainers to figure out why we are doing it and for the users to properly write their own handlers if they choose to do so.
|
Added docstring (hope this is understandable enough) |
|
Thanks! LGTM! |
Change the default fetch all handler for vertica provider in order to catch all errors in multi-statement query (issue 32993)
Closes: #32993