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

Truncate data that is expanded #7777

Merged
merged 3 commits into from Jul 1, 2019
Merged

Truncate data that is expanded #7777

merged 3 commits into from Jul 1, 2019

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Jun 25, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

When we expand complex columns in SQL Lab, the original data is still presented. This is redundant, and for most rows the values will be partially truncated. Users have complained that this wastes valuable space when previewing wide tables, while at the same bringing very little information.

This PR modifies the results table so that complex columns are presented with [...] or {...} as the results, instead of the full values (which can be seen in the columns to its left). See the screenshots below:

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen Shot 2019-06-25 at 11 27 08 AM

After:

Screen Shot 2019-06-25 at 11 27 33 AM

Note that only expanded columns are truncated. The "json" column is not truncated, in this example.

TEST PLAN

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

REVIEWERS

@khtruong @etr2460

if (content.substring(0, 1) === '[') {
type = '[…]';
} else if (content.substring(0, 1) === '[') {
type = '{…}';
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we hit this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it was supposed to be content.substring(0, 1) === '{'. Will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, the content doesn't distinguish between rows and arrays. A row content will also be "[ "a", "b", ... ]".

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 understand, but I think it's fine to show the rows as [...], since we represent the data as Javascript arrays.

Copy link
Contributor

@khtruong khtruong Jun 26, 2019

Choose a reason for hiding this comment

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

Sounds good. We can shorten this logic then.

const type = content.substring(0, 1) === '[' ? '[...]' : ''

@codecov-io
Copy link

codecov-io commented Jun 25, 2019

Codecov Report

Merging #7777 into master will increase coverage by 0.17%.
The diff coverage is 82.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7777      +/-   ##
==========================================
+ Coverage   65.71%   65.88%   +0.17%     
==========================================
  Files         459      459              
  Lines       21981    22001      +20     
  Branches     2415     2418       +3     
==========================================
+ Hits        14445    14496      +51     
+ Misses       7415     7384      -31     
  Partials      121      121
Impacted Files Coverage Δ
superset/db_engine_specs/presto.py 78.28% <ø> (+8.57%) ⬆️
...src/components/FilterableTable/FilterableTable.jsx 83.22% <82.6%> (-0.34%) ⬇️
superset/db_engine_specs/base.py 87.98% <0%> (-0.33%) ⬇️
superset/data/bart_lines.py 33.33% <0%> (ø) ⬆️
superset/connectors/druid/models.py 82.38% <0%> (ø) ⬆️
superset/tasks/schedules.py 80.1% <0%> (ø) ⬆️
superset/stats_logger.py 74.41% <0%> (ø) ⬆️
superset/sql_lab.py 76.88% <0%> (ø) ⬆️
superset/db_engine_specs/clickhouse.py 53.84% <0%> (ø) ⬆️
superset/sql_parse.py 99.2% <0%> (ø) ⬆️
... and 77 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 0c9e6d0...8e91920. Read the comment docs.

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a couple performance comments, I think it's actually worth considering them since this component can contain a ton of cells

);
const content = String(cellData);
let type;
if (content.substring(0, 1) === '[') {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is over optimization at this point, but since this function could be called tens of thousands of times, it might be better to set content.substring(0,1) to a const instead of calling it twice.

// fit table width if content doesn't fill the width of the container
this.totalTableWidth = containerWidth;
getCellContent({ cellData, columnKey }) {
const complexColumn = this.props.expandedColumns.some(
Copy link
Member

Choose a reason for hiding this comment

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

we should do this outside of getCellContent and pass it in since you'd be doing this loop through all the columns for each row in the table

@betodealmeida betodealmeida merged commit 34ca2ae into apache:master Jul 1, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 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 size/M 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants