fix: Address regression introduced in #21284 #21470

john-bodley · 2022-09-14T18:48:27Z

SUMMARY

Addresses regression introduced in #21284.

cc: @EugeneTorap

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

AFTER

TESTING INSTRUCTIONS

CI and verified the table selector worked in various surfaces.

ADDITIONAL INFORMATION

Has associated issue: Fixes always return with this error. ["There was an error loading the tables"] #21476
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2022-09-14T20:34:30Z

@eschutho, @lyndsiWilliams, and @pkdotson per #21075 it seems there's a new dataset selector which uses server side searching which was disabled in #21284. Is this actually a requirement for these new components or is it possible to remove said logic?

codecov · 2022-09-14T21:24:08Z

Codecov Report

Merging #21470 (33388ad) into master (59ca786) will increase coverage by 0.09%.
The diff coverage is 80.74%.

❗ Current head 33388ad differs from pull request most recent head a6209a4. Consider uploading reports for the commit a6209a4 to get more accurate results

@@            Coverage Diff             @@
##           master   #21470      +/-   ##
==========================================
+ Coverage   66.59%   66.69%   +0.09%     
==========================================
  Files        1791     1793       +2     
  Lines       68555    68490      -65     
  Branches     7319     7275      -44     
==========================================
+ Hits        45654    45676      +22     
+ Misses      21008    20946      -62     
+ Partials     1893     1868      -25

Flag	Coverage Δ
javascript	`52.87% <75.62%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...et-ui-chart-controls/src/shared-controls/index.tsx	`59.61% <0.00%> (+11.17%)`	⬆️
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...i-chart-controls/src/utils/expandControlConfig.tsx	`100.00% <ø> (ø)`
.../superset-ui-core/src/query/normalizeTimeColumn.ts	`100.00% <ø> (ø)`
...s/legacy-plugin-chart-heatmap/src/controlPanel.tsx	`57.14% <ø> (ø)`
...ns/legacy-plugin-chart-map-box/src/controlPanel.ts	`30.00% <ø> (ø)`
...t-chart-deckgl/src/utilities/sharedDndControls.jsx	`100.00% <ø> (ø)`
...s/legacy-preset-chart-nvd3/src/Bar/controlPanel.ts	`14.28% <ø> (ø)`
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx	`31.25% <ø> (ø)`
... and 46 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lyndsiWilliams · 2022-09-14T22:39:56Z

@eschutho, @lyndsiWilliams, and @pkdotson per #21075 it seems there's a new dataset selector which uses server side searching which was disabled in #21284. Is this actually a requirement for these new components or is it possible to remove said logic?

There are actually pre-existing instances of paginated searching elsewhere in SQL Lab. Bringing this search functionality to the front end could get really expensive, could we re-enable server side searching?

john-bodley · 2022-09-14T22:48:27Z

@lyndsiWilliams per you comment,

There are actually pre-existing instances of paginated searching elsewhere in SQL Lab. Bringing this search functionality to the front end could get really expensive, could we re-enable server side searching?

could you elaborate where this is used? Also when we (Airbnb) tried using the backend search functionality—augmenting our local environment—for fetching tables names we noticed the latency was unacceptable.

cc: @justinpark

lyndsiWilliams · 2022-09-14T23:42:46Z

@lyndsiWilliams per you comment,

There are actually pre-existing instances of paginated searching elsewhere in SQL Lab. Bringing this search functionality to the front end could get really expensive, could we re-enable server side searching?

could you elaborate where this is used? Also when we (Airbnb) tried using the backend search functionality—augmenting our local environment—for fetching tables names we noticed the latency was unacceptable.

cc: @justinpark

I looked into this further and discussed with Elizabeth, we actually don't use server side searching for this and it isn't used elsewhere in SQL Lab like I thought. The table data isn't stored in our database, we fetch it from their db and it's a "reflection" of their database, not actually scanning rows in a database.

It should be fine to remove the server side searching after all 😁

github-actions · 2022-09-14T23:49:10Z

@eschutho Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-09-14T23:49:10Z

@eschutho Ephemeral environment creation failed. Please check the Actions logs for details.

EugeneTorap · 2022-09-16T10:16:36Z

@john-bodley any news?
FYI we have /testenv up command to deploy test ENV and then you can ping @jinghua-qa and @andrey-zayats to test it to avoid the regression bugs.

EugeneTorap · 2022-09-16T15:46:45Z

@john-bodley Can you add in description Fix #21476 to auto close the issue after merging it?

Remove unused variable for CI to pass

lyndsiWilliams · 2022-09-16T18:37:15Z

/testenv up

github-actions · 2022-09-16T18:43:18Z

@lyndsiWilliams Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-09-16T18:43:20Z

@lyndsiWilliams Ephemeral environment creation failed. Please check the Actions logs for details.

lyndsiWilliams

LGTM! @jinghua-qa could you take a look at the ephemeral once it's up and make sure all tables are loading correctly now?

EugeneTorap · 2022-09-16T19:17:48Z

/testenv up

github-actions · 2022-09-16T19:18:03Z

@EugeneTorap Ephemeral environment creation is currently limited to committers.

lyndsiWilliams · 2022-09-16T19:21:12Z

/testenv up

github-actions · 2022-09-16T19:23:16Z

@lyndsiWilliams Ephemeral environment spinning up at http://35.90.233.189:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

eschutho · 2022-09-16T19:50:57Z

I tested SQL lab in the ephemeral and tables are loading now.

EugeneTorap · 2022-09-16T20:16:33Z

Can we merge it?

jinghua-qa · 2022-09-16T21:05:57Z

LGTM!

github-actions · 2022-09-16T21:18:31Z

Ephemeral environment shutdown and build artifacts deleted.

bumasl · 2022-09-17T17:11:06Z

/testenv up

Can someone explain to me how I can carry out the execution of this command?

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> (cherry picked from commit 8c16806)

)" This reverts commit 8c16806.

eschutho · 2022-09-28T16:32:55Z

@bumasl only Superset committers have permissions to create test environments. But you can request that someone start one for you if you'd like to test.

pull-request-size bot added the size/M label Sep 14, 2022

john-bodley requested a review from eschutho September 14, 2022 18:54

john-bodley marked this pull request as ready for review September 14, 2022 20:34

john-bodley force-pushed the john-bodley--fix-21284 branch from e942a2c to de5c8d9 Compare September 14, 2022 20:35

fix: Address regression introduced in apache#21284

32a4850

john-bodley force-pushed the john-bodley--fix-21284 branch from de5c8d9 to 32a4850 Compare September 14, 2022 20:37

eschutho approved these changes Sep 14, 2022

View reviewed changes

C-monC mentioned this pull request Sep 16, 2022

always return with this error. ["There was an error loading the tables"] #21476

Closed

Update index.tsx

a6209a4

Remove unused variable for CI to pass

lyndsiWilliams approved these changes Sep 16, 2022

View reviewed changes

lyndsiWilliams added the need:qa-review Requires QA review label Sep 16, 2022

eschutho merged commit 8c16806 into apache:master Sep 16, 2022

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Sep 19, 2022

fix: Address regression introduced in apache#21284 (apache#21470)

00b9e3d

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> (cherry picked from commit 8c16806)

mayurnewase added a commit to mayurnewase/incubator-superset that referenced this pull request Sep 22, 2022

Revert "fix: Address regression introduced in apache#21284 (apache#21470

219048f

)" This reverts commit 8c16806.

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Address regression introduced in #21284 #21470

fix: Address regression introduced in #21284 #21470

john-bodley commented Sep 14, 2022 •

edited by eschutho

Loading

john-bodley commented Sep 14, 2022

codecov bot commented Sep 14, 2022 •

edited

Loading

lyndsiWilliams commented Sep 14, 2022

john-bodley commented Sep 14, 2022 •

edited

Loading

lyndsiWilliams commented Sep 14, 2022 •

edited

Loading

github-actions bot commented Sep 14, 2022

github-actions bot commented Sep 14, 2022

EugeneTorap commented Sep 16, 2022 •

edited

Loading

EugeneTorap commented Sep 16, 2022 •

edited

Loading

lyndsiWilliams commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

lyndsiWilliams left a comment

EugeneTorap commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

lyndsiWilliams commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

eschutho commented Sep 16, 2022

EugeneTorap commented Sep 16, 2022

jinghua-qa commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

bumasl commented Sep 17, 2022 •

edited

Loading

eschutho commented Sep 28, 2022

fix: Address regression introduced in #21284 #21470

fix: Address regression introduced in #21284 #21470

Conversation

john-bodley commented Sep 14, 2022 • edited by eschutho Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

john-bodley commented Sep 14, 2022

codecov bot commented Sep 14, 2022 • edited Loading

Codecov Report

lyndsiWilliams commented Sep 14, 2022

john-bodley commented Sep 14, 2022 • edited Loading

lyndsiWilliams commented Sep 14, 2022 • edited Loading

github-actions bot commented Sep 14, 2022

github-actions bot commented Sep 14, 2022

EugeneTorap commented Sep 16, 2022 • edited Loading

EugeneTorap commented Sep 16, 2022 • edited Loading

lyndsiWilliams commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

lyndsiWilliams left a comment

Choose a reason for hiding this comment

EugeneTorap commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

lyndsiWilliams commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

eschutho commented Sep 16, 2022

EugeneTorap commented Sep 16, 2022

jinghua-qa commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

bumasl commented Sep 17, 2022 • edited Loading

eschutho commented Sep 28, 2022

john-bodley commented Sep 14, 2022 •

edited by eschutho

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

john-bodley commented Sep 14, 2022 •

edited

Loading

lyndsiWilliams commented Sep 14, 2022 •

edited

Loading

EugeneTorap commented Sep 16, 2022 •

edited

Loading

EugeneTorap commented Sep 16, 2022 •

edited

Loading

bumasl commented Sep 17, 2022 •

edited

Loading