-
Notifications
You must be signed in to change notification settings - Fork 40
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
Handled multiple database tables in dataframe operator. #325
Conversation
Codecov Report
@@ Coverage Diff @@
## main #325 +/- ##
==========================================
+ Coverage 93.08% 93.48% +0.40%
==========================================
Files 35 35
Lines 1532 1535 +3
Branches 268 271 +3
==========================================
+ Hits 1426 1435 +9
+ Misses 74 69 -5
+ Partials 32 31 -1
Continue to review full report at Codecov.
|
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.
@utkarsharma2 it seems there is something strange because there is a commit that is part of this PR that is different from the commit in the main related to the 0.8.1 release(a0d0ddf).
Also, I suggest the commit message have the word Fix
since it fixes a live bug
conn_id_set = { | ||
t.conn_id for t in self.parameters.values() if isinstance(t, Table) | ||
} | ||
if param_tables and len(conn_id_set) == 1: |
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.
@utkarsharma2 as we spoke, since this was a bug found in an Astro release, we need a regression test to make sure the same bug doesn't appear again - particularly given the refactoring we're doing.
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.
It should be possible to unit test _set_variables_from_first_table
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 have added a UnitTest case checking all possible conditions.
|
||
|
||
def test__set_variables_from_first_table_with_different_db_tables_in_parameters(): | ||
"""Test _set_variables_from_first_table() when the tables passed are with same tables in parameters""" |
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.
"with same tables" -> "have different databases"
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.
Looks great, @utkarsharma2! Thanks for solving this!
Closes: #323
Within method - _set_variables_from_first_table
we try to fill in all DB params from the first table passes as a param to the dataframe operator, there was a corner case not handled there.
Check to see if all tables belong to same conn_id. Otherwise, we this can go wrong for cases