-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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]: Check datatype of results before converting to DataFrame #4108
Conversation
ca0f310
to
2d5e427
Compare
Could you please clean the branch from other people commits? |
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.
Better to add a test so we don't regress.
superset/sql_lab.py
Outdated
@@ -231,11 +231,12 @@ def handle_error(msg): | |||
|
|||
# check whether the result set is comprised of lists or dict | |||
if data and len(data) > 0: |
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.
if data:
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.
Yep you're right the len check is redundant
superset/sql_lab.py
Outdated
df_data = np.array(data) | ||
else: | ||
first_row = data[0] | ||
first_row_types = set([type(c) for c in first_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.
Is this more readable?
any([isinstance(c, dict) for c in first_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.
Yeah I just took a stab at refactoring for clarity
6959b35
to
ea14efe
Compare
@xrmx how would you recommend testing this? AFAIK the only reason for using The existing Any suggestions how we might simulate an embedded dict column from the test suite? |
The easiest way to test that code would be to move it to an helper and unit test it. |
fix type checking fix conditional checks remove trailing whitespace and fix df_data fallback def actually remove trailing whitespace generalized type check to check all columns for dict refactor dict col check
5db8d3e
to
29be9c2
Compare
Ok moved the df conversion logic to helper and wrote two simple unit tests. Not sure the tests actually prevent regression because I'm mocking the query results... |
add missing newlines another missing newline fix quotes more quote fixes
8e44e76
to
be2a850
Compare
@xrmx do you think this is ready to merge? |
…pache#4108) * conditional check on datatype of results before converting to df fix type checking fix conditional checks remove trailing whitespace and fix df_data fallback def actually remove trailing whitespace generalized type check to check all columns for dict refactor dict col check * move df conversion to helper and add unit test add missing newlines another missing newline fix quotes more quote fixes
…pache#4108) * conditional check on datatype of results before converting to df fix type checking fix conditional checks remove trailing whitespace and fix df_data fallback def actually remove trailing whitespace generalized type check to check all columns for dict refactor dict col check * move df conversion to helper and add unit test add missing newlines another missing newline fix quotes more quote fixes
The merged hotfix #2412 resolved an issue with Presto/Hive columns #2398 which are lists of dictionaries.
However, this introduced issue #3934 with traditional RDB datasources like MS SQL Server, which only return lists of lists. Wrapping such results in an outer list causes pandas to interpret the list as a single column.
This PR attempts to resolve that issue by introspecting the datatypes of the columns in the first row of the returned
data
. The type check may not be checking for exactly the right thing, but we likely need some sort of conditional to avoid wrapping a list of lists in an outer list