-
Notifications
You must be signed in to change notification settings - Fork 204
Fix zombie queries on metadata #608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix import order and see CI failures |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
|
|
||
| from sqlalchemy import exc | ||
| from sqlalchemy import sql | ||
| from sqlalchemy import __version__ as SQLALCHEMY_VERSION | ||
| from sqlalchemy.engine import Engine | ||
| from sqlalchemy.engine.base import Connection | ||
| from sqlalchemy.engine.default import DefaultDialect | ||
|
|
@@ -221,9 +222,20 @@ def _get_partitions( | |
| SELECT * FROM {schema}."{table_name}$partitions" | ||
| """ | ||
| ).strip() | ||
| res = connection.execute(sql.text(query)) | ||
| partition_names = [desc[0] for desc in res.cursor.description] | ||
| data_types = [desc[1] for desc in res.cursor.description] | ||
|
|
||
| if SQLALCHEMY_VERSION >= '2.0.0': | ||
| # Lower versions of SQLAlchemy doesn't support execution of query as ctxt man | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then why not always do it like the manual way? the context manager is just syntax sugar AFAIK for the same sequence of operations.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also this path (> 2.0) isn't fetching data anyway so it'll show CANCELLED too. BTW I don't agree that showing those as CANCELLED is wrong. We are unnecessarily fetching results for cosmetic benefits. |
||
| with connection.execute(sql.text(query)) as conn: | ||
| partition_names = [desc[0] for desc in conn.cursor.description] | ||
| data_types = [desc[1] for desc in conn.cursor.description] | ||
| else: | ||
| conn = connection.execute(sql.text(query)) | ||
| partition_names = [desc[0] for desc in conn.cursor.description] | ||
| data_types = [desc[1] for desc in conn.cursor.description] | ||
| res = conn.fetchall() # Fetching data to consider query as FINISHED and not CANCELED | ||
| if not conn.closed: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. try ... finally? |
||
| conn.close() | ||
|
|
||
| # Compare the column names and types to the shape of an Iceberg $partitions table | ||
| if (partition_names == ['partition', 'record_count', 'file_count', 'total_size', 'data'] | ||
| and data_types[0].startswith('row(') | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we avoid mocked tests since the sequence of responses is now hand constructed and doesn't really test anything meaningful