fix(virtual dataset sync): Sync virtual dataset columns when changing the SQL query #30903

fisjac · 2024-11-12T16:56:57Z

SUMMARY

When changing the SQL query for virtual datasets, the user first needs to save the virtual dataset, then needs to reopen the "edit dataset" modal and manually sync the columns. This PR will introduce changes to automatically sync the columns upon saving a virtual dataset with a new SQL query.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

https://www.loom.com/share/b9a1121ec44b47438e2675334403ee79

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
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

villebro · 2024-11-12T17:03:43Z

This is a total pain, super happy to see this potentially get fixed soon! ❤️

superset-frontend/src/components/Datasource/DatasourceEditor.jsx

superset-frontend/src/components/Datasource/DatasourceModal.tsx

kgabryje · 2024-11-28T15:03:40Z

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

+  catalog?: string;
+  schema?: string;
+  table_name?: string;
+  database?: Record<string, unknown>;


Could we improve the typing here? We probably have a type for database defined somewhere

@fisjac +1 for this

kgabryje · 2024-11-28T15:04:46Z

superset-frontend/src/components/Datasource/DatasourceModal.tsx

+    extra: datasource.extra,
+    is_managed_externally: datasource.is_managed_externally,
+    external_url: datasource.external_url,
+    metrics: datasource?.metrics?.map((metric: Record<string, unknown>) => {


Suggested change

metrics: datasource?.metrics?.map((metric: Record<string, unknown>) => {

metrics: datasource?.metrics?.map((metric: Metric) => {

See below comment regarding is_certified

kgabryje · 2024-11-28T15:06:12Z

superset-frontend/src/components/Datasource/DatasourceModal.tsx

+      }
+      return metricBody;
+    }),
+    columns: datasource?.columns?.map((column: Record<string, unknown>) => ({


Suggested change

columns: datasource?.columns?.map((column: Record<string, unknown>) => ({

columns: datasource?.columns?.map((column: Column) => ({

(or ColumnMeta)

There are some issues with doing this due to a part of the code that checks for an is_certified property on somewhat ambiguous Metric Column and Data items. I agree that these types should be tightened up, but I don't have enough context to start tweaking these in this PR.

@fisjac should not be too hard to find out what the right type is? Maybe we need a partial type or to update the original type if a property appears to be missing. It defeats the purpose of using typescript if we opt for unknown types

kgabryje · 2024-11-28T15:07:00Z

Small typing suggestions, other than that the code looks great 👍

dkrat7 · 2024-12-06T10:10:43Z

Hi,
syncing columns without saving and reopening the editor is great improvement.

However, i have some concerns regarding autosyncing on save:

What will happen if I accidently make a change to a query which will result in zero rows returned - all columns including metadata will be permanently removed?

Sometimes I include Jinja code in a query with condition that dataset returns zero rows if some filter is not selected (for example) - I won't be able to save that query without losing columns?

fisjac · 2024-12-06T15:45:45Z

@dkrat7 to my knowledge, columns are not dropped based on whether row level data are available for that column, but based upon whether the column is selected as part of the query. In your case, the columns will be dropped until the query is edited to include the desired columns.

Point being, if your query would include (or exclude) a column, then the virtual dataset will be synced to reflect the state of that query upon save.

yousoph · 2024-12-06T17:35:14Z

/testenv up

betodealmeida · 2024-12-10T16:59:41Z

What will happen if I accidently make a change to a query which will result in zero rows returned - all columns including metadata will be permanently removed?

It should be fine, since the cursor should still return a description of the columns even if the number of rows is 0. For example, for Postgres:

import psycopg2

query = "SELECT * FROM your_table WHERE 1=0;"  # Query guaranteed to return zero rows

db_config = {
    "dbname": "DB",
    "user": "user",
    "password": "password",
    "host": "host",
    "port": 5432,
}

with psycopg2.connect(**db_config) as conn:
    with conn.cursor() as cursor:
        cursor.execute(query)

        print("Cursor description:")
        for col in cursor.description:
            print(
                f"Name: {col.name}, Type: {col.type_code}, Display Size: {col.display_size}"
            )

For a test table this prints:

Cursor description:
Name: show_id, Type: 25, Display Size: None
Name: type, Type: 25, Display Size: None
Name: title, Type: 25, Display Size: None
Name: director, Type: 25, Display Size: None
Name: country, Type: 25, Display Size: None
Name: date_added, Type: 25, Display Size: None
Name: release_year, Type: 23, Display Size: None
Name: rating, Type: 25, Display Size: None
Name: duration, Type: 25, Display Size: None
Name: listed_in, Type: 25, Display Size: None
Name: description, Type: 25, Display Size: None
Name: iso_date_added, Type: 25, Display Size: None
Name: iso_release_year, Type: 25, Display Size: None
Name: xata_version, Type: 23, Display Size: None
Name: xata_createdat, Type: 1184, Display Size: None
Name: xata_updatedat, Type: 1184, Display Size: None
Name: xata_id, Type: 25, Display Size: None

geido

Thanks @fisjac. I am requesting changes as I am not sure we should delete the E2E tests. Also, some types need more love. Product feedback might also help to discuss the toasts. I feel, in general, this needs a bit more refinement.

geido · 2024-12-11T12:09:23Z

superset-frontend/cypress-base/cypress/e2e/explore/control.test.ts

-// ***********************************************
-// Tests for setting controls in the UI
-// ***********************************************
-import { interceptChart } from 'cypress/utils';


Is it intentional that we are deleting these E2E tests?

geido · 2024-12-11T12:09:47Z

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

+  catalog?: string;
+  schema?: string;
+  table_name?: string;
+  database?: Record<string, unknown>;


@fisjac +1 for this

geido · 2024-12-11T12:12:36Z

superset-frontend/src/components/Datasource/DatasourceModal.tsx

+      }
+      return metricBody;
+    }),
+    columns: datasource?.columns?.map((column: Record<string, unknown>) => ({


@fisjac should not be too hard to find out what the right type is? Maybe we need a partial type or to update the original type if a property appears to be missing. It defeats the purpose of using typescript if we opt for unknown types

geido · 2024-12-11T12:15:11Z

superset-frontend/src/components/Datasource/utils.js

+      columnChanges.finalColumns.push(currentCol);
+    }
+  });
+  if (columnChanges.modified.length) {


I am wondering if this is an opportunity to improve the user experience here and just show a generic message. From your sample video it looks really bad and possibly useless for the user to see such big success toasts with a bunch of columns in them. cc @kasiazjc

agreed that showing each of the columns changed winds up being a bit of an overkill from a user standpoint. Should we just do away with the toasts altogether in favor of something like "columns updated"?

geido · 2024-12-11T12:15:38Z

superset-frontend/src/explore/components/controls/TextAreaControl.jsx

@@ -94,31 +100,44 @@ class TextAreaControl extends Component {
      if (this.props.readOnly) {
        style.backgroundColor = '#f2f2f2';
      }
+      const codeEditor = (
+        <div>


What is this div for?

When trying to wrap a tooltip around the TextAreaEditor without a div, the tooltip doesn't land in the right place. Placing a div enables the tooltip to behave as intended.

geido · 2024-12-11T12:15:55Z

superset-frontend/src/explore/components/controls/TextAreaControl.jsx

-          maxLines={inModal ? 1000 : this.props.maxLines}
-          editorProps={{ $blockScrolling: true }}
+    const textArea = (
+      <div>


Same for this div, what is this for?

same answer as above

pull-request-size bot added the size/XS label Nov 12, 2024

sadpandajoe added the review:draft label Nov 14, 2024

pull-request-size bot added size/L and removed size/XS labels Nov 21, 2024

fisjac marked this pull request as ready for review November 22, 2024 21:24

dosubot bot added the data:dataset Related to dataset configurations label Nov 22, 2024

fisjac force-pushed the virtual-dataset-sync branch from 84e8a90 to 2041141 Compare November 25, 2024 15:53

github-actions bot added the packages label Nov 25, 2024

pull-request-size bot added size/XL and removed size/L labels Nov 27, 2024

fisjac force-pushed the virtual-dataset-sync branch 2 times, most recently from fc3f83c to 3d82059 Compare November 27, 2024 06:04

geido reviewed Nov 27, 2024

View reviewed changes

superset-frontend/src/components/Datasource/DatasourceEditor.jsx Outdated Show resolved Hide resolved

superset-frontend/src/components/Datasource/DatasourceModal.tsx Show resolved Hide resolved

geido requested review from kgabryje and Antonio-RiveroMartnez November 27, 2024 15:50

fisjac requested review from jinghua-qa, eschutho, rusackas and betodealmeida as code owners November 27, 2024 17:23

kgabryje reviewed Nov 28, 2024

View reviewed changes

sadpandajoe removed the review:draft label Dec 5, 2024

adding logic to sync virtual data columns on save

47d5b3a

fisjac force-pushed the virtual-dataset-sync branch from b8228f8 to 47d5b3a Compare December 6, 2024 05:19

lint

2874a56

removing flaky cypress test

72ab91e

fisjac mentioned this pull request Dec 6, 2024

feat(TextAreaControl): Adding Tooltip capabality to the text editor field #30917

Closed

9 tasks

fixing typo

070af71

apache deleted a comment from github-actions bot Dec 10, 2024

geido requested changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(virtual dataset sync): Sync virtual dataset columns when changing the SQL query #30903

fix(virtual dataset sync): Sync virtual dataset columns when changing the SQL query #30903

fisjac commented Nov 12, 2024 •

edited

Loading

villebro commented Nov 12, 2024

kgabryje Nov 28, 2024

geido Dec 11, 2024

kgabryje Nov 28, 2024 •

edited

Loading

fisjac Dec 6, 2024

kgabryje Nov 28, 2024

fisjac Dec 6, 2024 •

edited

Loading

geido Dec 11, 2024

kgabryje commented Nov 28, 2024

dkrat7 commented Dec 6, 2024

fisjac commented Dec 6, 2024

yousoph commented Dec 6, 2024

betodealmeida commented Dec 10, 2024

geido left a comment

geido Dec 11, 2024

geido Dec 11, 2024

geido Dec 11, 2024

geido Dec 11, 2024

fisjac Dec 12, 2024

geido Dec 11, 2024

fisjac Dec 12, 2024

geido Dec 11, 2024

fisjac Dec 12, 2024

	metrics: datasource?.metrics?.map((metric: Record<string, unknown>) => {
	metrics: datasource?.metrics?.map((metric: Metric) => {

	columns: datasource?.columns?.map((column: Record<string, unknown>) => ({
	columns: datasource?.columns?.map((column: Column) => ({

fix(virtual dataset sync): Sync virtual dataset columns when changing the SQL query #30903

Are you sure you want to change the base?

fix(virtual dataset sync): Sync virtual dataset columns when changing the SQL query #30903

Conversation

fisjac commented Nov 12, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisjac Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Nov 28, 2024

dkrat7 commented Dec 6, 2024

fisjac commented Dec 6, 2024

yousoph commented Dec 6, 2024

betodealmeida commented Dec 10, 2024

geido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisjac commented Nov 12, 2024 •

edited

Loading

kgabryje Nov 28, 2024 •

edited

Loading

fisjac Dec 6, 2024 •

edited

Loading