-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[BUGFIX] Fix validation for QueryAsset
with create_temp_table: False
#8979
[BUGFIX] Fix validation for QueryAsset
with create_temp_table: False
#8979
Conversation
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
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.
Thanks for the contribution! Before we can consider merging it looks like there are a few CI failures to address.
table_selectable = batch_data.selectable | ||
schema_name = None |
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.
I'm wondering if the change in this else clause is causing the CI failures. In the case where batch_data.selectable isn't a sqlalchemy object we are using the source table and schema names.
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.
table_selectable = batch_data.selectable | |
schema_name = None | |
table_selectable = batch_data.source_table_name or batch_data.selectable.name | |
schema_name = batch_data.source_schema_name or batch_data.selectable.schema |
if sqlalchemy.TextClause and isinstance(selectable, sqlalchemy.TextClause): | ||
query: sqlalchemy.TextClause = selectable | ||
# if a custom query was passed | ||
elif sqlalchemy.Subquery and isinstance(selectable, sqlalchemy.Subquery): |
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.
Would you mind adding a test for this case to tests/expectations/test_util.py
?
@@ -93,21 +93,26 @@ def _spark( # noqa: PLR0913 | |||
def _get_sqlalchemy_column_metadata( | |||
execution_engine: SqlAlchemyExecutionEngine, batch_data: SqlAlchemyBatchData | |||
): | |||
table_selectable: str | sqlalchemy.TextClause |
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.
Would you mind adding tests for this case to tests/expectations/metrics/test_table_column_types.py
?
Hi @moatazelmohtaseb ! Thank you so much for this contribution. Just following up to see if you had any questions about the requested changes. |
Thank you so much for this contribution. For now we are closing this PR, but if you are interested in picking this work back up please reopen it! |
#8325
After investigating the error, it seems to be an issue with
table_column_types.py
's method_get_sqlalchemy_column_metadata
.Tested the change on a custom QueryAsset with
create_temp_table
being eitherTrue
orFalse
invoke lint
(usesblack
+ruff
)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!