Skip to content
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

fix(presto): Handle ROW data stored as string #10456

Merged
merged 5 commits into from
Jul 28, 2020

Conversation

betodealmeida
Copy link
Member

SUMMARY

When serializing nested types we convert them to string, which breaks the expand_data in Presto.

I added a check in expand_data so that when the data associated with a nested column (types ARRAY and ROW) is a string, it gets deserialized back into the original data.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

A Presto query selecting an ARRAY and a ROW without the PRESTO_EXPAND_DATA feature flag:

Screen Shot 2020-07-28 at 12 41 30 PM

With the feature enabled, data should be displayed similar to how BigQuery does, with arrays expanded into multiple lines, and rows expanded into multiple columns. This is currently broken:

Screen Shot 2020-07-28 at 12 41 33 PM

This PR fixes the problem, and the data gets expanded correctly:

Screen Shot 2020-07-28 at 12 41 36 PM

TEST PLAN

Tested with a simple query (see above), and added a unit test covering the problem.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@betodealmeida betodealmeida added !deprecated-label:bug Deprecated label - Use #bug instead lyft Related to Lyft minor-review labels Jul 28, 2020
@@ -653,12 +656,15 @@ def expand_data( # pylint: disable=too-many-locals
# expand columns; we append them to the left so they are added
# immediately after the parent
expanded = get_children(column)
to_process.extendleft((column, level) for column in expanded)
to_process.extendleft((column, level) for column in expanded[::-1])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reversed expanded so that sub-columns are added in the right order, otherwise we get:

a ROW(b, c) => a, a.c, a.b

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #10456 into master will increase coverage by 1.44%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10456      +/-   ##
==========================================
+ Coverage   70.60%   72.04%   +1.44%     
==========================================
  Files         601      620      +19     
  Lines       32329    36465    +4136     
  Branches     3275     3695     +420     
==========================================
+ Hits        22826    26272    +3446     
- Misses       9397    10075     +678     
- Partials      106      118      +12     
Flag Coverage Δ
#cypress 55.69% <ø> (-0.02%) ⬇️
#javascript 59.88% <ø> (+0.26%) ⬆️
#python 72.31% <91.66%> (+2.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engine_specs/presto.py 70.42% <90.00%> (+0.17%) ⬆️
superset/result_set.py 98.34% <100.00%> (+0.02%) ⬆️
...onents/ErrorMessage/ErrorMessageWithStackTrace.tsx 18.75% <0.00%> (-7.57%) ⬇️
...rontend/src/SqlLab/components/QueryAutoRefresh.jsx 65.90% <0.00%> (-6.82%) ⬇️
superset/errors.py 95.65% <0.00%> (-4.35%) ⬇️
superset-frontend/src/chart/Chart.jsx 61.25% <0.00%> (-4.06%) ⬇️
...rontend/src/visualizations/FilterBox/FilterBox.jsx 62.34% <0.00%> (-3.29%) ⬇️
superset-frontend/src/components/DeleteModal.tsx 89.47% <0.00%> (-2.20%) ⬇️
superset-frontend/src/components/Modal.tsx 92.85% <0.00%> (-1.59%) ⬇️
...uperset-frontend/src/dashboard/util/propShapes.jsx 91.30% <0.00%> (-1.56%) ⬇️
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d10669...63bf737. Read the comment docs.

@dpgaspar dpgaspar changed the title Handle ROW data stored as string fix(presto): Handle ROW data stored as string Jul 28, 2020
@betodealmeida betodealmeida merged commit 4f67827 into master Jul 28, 2020
betodealmeida added a commit that referenced this pull request Jul 28, 2020
* Handle ROW data stored as string

* Use destringify

* Fix mypy

* Fix mypy with cast

* Bypass pylint
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jul 28, 2020
fix(presto): Handle ROW data stored as string (apache#10456)
jinhyukchang pushed a commit to lyft/incubator-superset that referenced this pull request Aug 28, 2020
* Handle ROW data stored as string

* Use destringify

* Fix mypy

* Fix mypy with cast

* Bypass pylint
jinhyukchang added a commit to lyft/incubator-superset that referenced this pull request Aug 28, 2020
[VIZ-1979] [Backporting] fix(presto): Handle ROW data stored as string (apache#10456)
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* Handle ROW data stored as string

* Use destringify

* Fix mypy

* Fix mypy with cast

* Bypass pylint
@amitmiran137 amitmiran137 deleted the handle_presto_row_data_str branch March 29, 2021 18:10
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* Handle ROW data stored as string

* Use destringify

* Fix mypy

* Fix mypy with cast

* Bypass pylint
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels !deprecated-label:bug Deprecated label - Use #bug instead lyft Related to Lyft size/M 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants