fix(chart): Supporting custom SQL as temporal x-axis column with filter #25126

zephyring · 2023-08-30T14:17:43Z

SUMMARY

properly extracting x_axis from sqlExpression when using custom sql as column while applying granularity in query context.
without the extraction, `unhashable type: 'dict' error message will pop up when updating chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

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

… while applying granularity

villebro

Thanks for the fix - one comment about using a pre-existing util. Also, could we add a test to make sure this works as expected? There should be several old integration tests for the chart data endpoint that can be repurposed for this test. Let me know if you need help setting it up.

superset/common/query_context_factory.py

villebro · 2023-08-30T15:54:56Z

@zephyring also, let's add a proper title + description to explain the change here.

…zef/fix_x_axis_custom_sql

…om_sql

kgabryje · 2023-09-15T13:09:19Z

/testenv up

github-actions · 2023-09-15T13:11:28Z

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

villebro

LGTM - one last nit about using a type guard to detect adhoc columns, other than that LGTM

superset/common/query_context_factory.py

…mpty

villebro · 2023-09-15T17:31:05Z

superset/common/query_context_factory.py

@@ -128,6 +128,8 @@ def _apply_granularity(

        if granularity := query_object.granularity:
            filter_to_remove = None
+            if is_adhoc_column(x_axis):  # type: ignore


I'm curious why we need # type: ignore here?

the x_axis is from form_data and it's ANY

It may be a good idea to broaden the scope of the type guard to Any so it can handle arbitrary types.

I think the type guard is better to scope down rather than scope up right?

@zephyring @villebro I think this PR introduced another potential bug in Adhoc column as X-Axis, you should sent a probe query to ensure x-axis is a temporal column.

hi @zhaoyongjie can you share more on what potential bug this may've introduced?

zephyring · 2023-09-15T17:41:04Z

superset/connectors/sqla/models.py

@@ -1007,6 +1007,8 @@ def adhoc_column_to_sqla(  # pylint: disable=too-many-locals
                    qry = sa.select([sqla_column]).limit(1).select_from(tbl)
                    sql = self.database.compile_sqla_query(qry)
                    col_desc = get_columns_description(self.database, self.schema, sql)
+                    if not col_desc:


dealing with case where custom_sql can be empty string and col_desc is empty

I think we should handle this on frontend and not let users save a custom sql that's an empty string

@kgabryje I agree - however, as it will be lots of work to clean up old metadata, I think we can maybe have both?

I think we should handle this edge case in the backend anyways. I will be nice to prevent user from saving empty custom sql in the frontend and show some friendly message. But for now if the custom_sql is any arbitrary string the API responds with 500 with error message directly from DB engine, which is "fine" and readable for users but just this empty string it throws index out of bound error to user which is not helpful.

we can create a separate PR for UI messaging and prevention though

github-actions · 2023-09-18T18:31:27Z

Ephemeral environment shutdown and build artifacts deleted.

…er (apache#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>

…er (#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>

zhaoyongjie · 2023-09-25T15:35:30Z

tests/integration_tests/charts/data/api_tests.py

+                        "granularity": "ds",
+                        "filters": [
+                            {"col": "ds", "op": "TEMPORAL_RANGE", "val": "No filter"}
+                        ],
+                        "extras": {
+                            "having": "",
+                            "where": "",
+                        },
+                        "applied_time_extras": {},
+                        "columns": [
+                            {
+                                "columnType": "BASE_AXIS",
+                                "datasourceWarning": False,
+                                "expressionType": "SQL",
+                                "label": "My column",
+                                "sqlExpression": "ds",
+                                "timeGrain": "P1W",
+                            }
+                        ],


Basically, the "granularity" and "Axis-column" shouldn't be concurrent sent from frontend. the "granularity" is a legacy Druid "concept"(and this concept as be wrapped under the hood in "modern" Druid), so from my original design would like to totally remove "granularity" in Superset, the "granularity" means that a temporal column in columns(or dimensions).

the "time_range" and "filters" in query_object is same logic as before.

Basically, the "granularity" and "Axis-column" shouldn't be concurrent sent from frontend. the "granularity" is a legacy Druid "concept"(and this concept as be wrapped under the hood in "modern" Druid), so from my original design would like to totally remove "granularity" in Superset, the "granularity" means that a temporal column in columns(or dimensions).

I'm trying to fix a bug related to granularity. Basically, the temporal granularity column should be renamed as "__timestamp" like what the legacy time series line chart did before(alias after group by should not be the same with the raw column name in clickhouse) but current chart don't, so granularity is a little different from other temporal columns(at least for users). If the "granularity" should be removed, should this bug be fixed on the frontend?

it seems like the time column as x-axis has been completely unusable for a very long time. thanks to this PR made custom SQL work

* fix: is_select with UNION (apache#25290) (cherry picked from commit bb002d6) * fix: Add explicit ON DELETE CASCADE for dashboard_roles (apache#25320) (cherry picked from commit d54e827) * fix(chart): Supporting custom SQL as temporal x-axis column with filter (apache#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> * fix: Use RLS clause instead of ID for cache key (apache#25229) (cherry picked from commit fba66c6) * fix: Improve the reliability of alerts & reports (apache#25239) (cherry picked from commit f672d5d) * fix: DashboardRoles cascade operation (apache#25349) (cherry picked from commit a971a28) * fix: datetime with timezone excel export (apache#25318) Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> (cherry picked from commit 5ebcd2a) * fix: Workaround for Cypress ECONNRESET error (apache#25399) (cherry picked from commit d76ff39) * fix(sqllab): invalid persisted tab state (apache#25308) (apache#25398) * fix: Rename on_delete parameter to ondelete (apache#25424) (cherry picked from commit 893b45f) * fix: preventing save button from flickering in SQL Lab (apache#25106) (cherry picked from commit 296ff17) * fix: chart import (apache#25425) (cherry picked from commit a4d8f36) * fix: swagger UI CSP error (apache#25368) (cherry picked from commit 1716b9f) * fix: smarter date formatter (apache#25404) (cherry picked from commit f0080f9) * fix(sqllab): invalid start date (apache#25437) * fix(nativeFilters): Speed up native filters by removing unnecessary rerenders (apache#25282) Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> (cherry picked from commit a0eeb4d) * fix(SqlLab): make icon placement even (apache#25372) (cherry picked from commit 11b49a6) * fix: Duplicate items when pasting into Select (apache#25447) (cherry picked from commit 7cf96cd) * fix: update the SQLAlchemy model definition at json column for Log table (apache#25445) (cherry picked from commit e83a76a) * fix(helm chart): set chart appVersion to 3.0.0 (apache#25373) * fix(mysql): handle string typed decimal results (apache#24241) (cherry picked from commit 7eab59a) * fix: Styles not loading because of faulty CSP setting (apache#25468) (cherry picked from commit 0cebffd) * fix(sqllab): error with lazy_gettext for tab titles (apache#25469) (cherry picked from commit ddde178) * fix: Address Mypy issue which is causing CI to fail (apache#25494) (cherry picked from commit 36ed617) * chore: Adds 3.0.1 CHANGELOG * fix: Unable to sync columns when database or dataset name contains `+` (apache#25390) (cherry picked from commit dbe0838) * fix(sqllab): Broken query containing 'children' (apache#25490) (cherry picked from commit b92957e) * chore: Expand error detail on screencapture (apache#25519) (cherry picked from commit ba541e8) * fix: tags permissions error message (apache#25516) (cherry picked from commit 50b0816) * fix: Apply normalization to all dttm columns (apache#25147) (cherry picked from commit 58fcd29) * fix: REST API CSRF exempt list (apache#25590) (cherry picked from commit 549abb5) * fix(RLS): Fix Info Tooltip + Button Alignment on RLS Modal (apache#25400) (cherry picked from commit a6d0e6f) * fix: thubmnails loading - Talisman default config (apache#25486) (cherry picked from commit 52f631a) * fix(Presto): catch DatabaseError when testing Presto views (apache#25559) Co-authored-by: Rui Zhao <zhaorui@dropbox.com> (cherry picked from commit be3714e) * fix(Charts): Set max row limit + removed the option to use an empty row limit value (apache#25579) (cherry picked from commit f556ef5) * fix(window): unavailable localStorage and sessionStorage (apache#25599) * fix: finestTemporalGrainFormatter (apache#25618) (cherry picked from commit 62bffaf) * fix: revert fix(sqllab): Force trino client async execution (apache#24859) (apache#25541) (cherry picked from commit e56e0de) * chore: Updates 3.0.1 CHANGELOG * fix(sqllab): Mistitled for new tab after rename (apache#25523) (cherry picked from commit a520124) * fix(sqllab): template validation error within comments (apache#25626) (cherry picked from commit b370c66) * fix: avoid 500 errors with SQLLAB_BACKEND_PERSISTENCE (apache#25553) (cherry picked from commit 99f79f5) * fix(import): Make sure query context is overwritten for overwriting imports (apache#25493) (cherry picked from commit a0a0d80) * fix: permalink save/overwrites in explore (apache#25112) Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> (cherry picked from commit e58a3ab) * fix(header navlinks): link navlinks to path prefix (apache#25495) (cherry picked from commit 51c56dd) * fix: improve upload ZIP file validation (apache#25658) * fix: warning of nth-child (apache#23638) (cherry picked from commit 16cc089) * fix(dremio): Fixes issue with Dremio SQL generation for Charts with Series Limit (apache#25657) (cherry picked from commit be82657) --------- Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com> Co-authored-by: Zef Lin <zef@preset.io> Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> Co-authored-by: Jack Fragassi <jfragassi98@gmail.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> Co-authored-by: Jack <41238731+fisjac@users.noreply.github.com> Co-authored-by: Daniel Vaz Gaspar <danielvazgaspar@gmail.com> Co-authored-by: Stepan <66589759+Always-prog@users.noreply.github.com> Co-authored-by: Corbin Bullard <corbindbullard@gmail.com> Co-authored-by: Gyuil Han <cnabro91@gmail.com> Co-authored-by: Celalettin Calis <celalettin1286@gmail.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Co-authored-by: ʈᵃᵢ <tdupreetan@gmail.com> Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com> Co-authored-by: mapledan <mapledan829@gmail.com> Co-authored-by: Igor Khrol <khroliz@gmail.com> Co-authored-by: Rui Zhao <105950525+zhaorui2022@users.noreply.github.com> Co-authored-by: Fabien <18534166+frassinier@users.noreply.github.com> Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com> Co-authored-by: OskarNS <soerensen.oskar@gmail.com>

…er (apache#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>

fix: dealing with case where x_axis is dict when column is custom_sql…

5b62640

… while applying granularity

pull-request-size bot added the size/S label Aug 30, 2023

villebro reviewed Aug 30, 2023

View reviewed changes

superset/common/query_context_factory.py Outdated Show resolved Hide resolved

zephyring changed the title ~~fix: dealing with case where x_axis is dict when column is custom_sql…~~ fix: Using Custom SQL as X Column while switching dataset for a chart Aug 30, 2023

renaming vars

8bc5c72

zephyring changed the title ~~fix: Using Custom SQL as X Column while switching dataset for a chart~~ fix(Chart): Supporting custom SQL as x-axis when querying chart with granularity Sep 6, 2023

zephyring changed the title ~~fix(Chart): Supporting custom SQL as x-axis when querying chart with granularity~~ fix(chart): Supporting custom SQL as x-axis when querying chart with granularity Sep 6, 2023

lint

a191583

pull-request-size bot added size/XS and removed size/S labels Sep 6, 2023

added api_tests

cf8c0ba

pull-request-size bot added size/M and removed size/XS labels Sep 6, 2023

zephyring changed the title ~~fix(chart): Supporting custom SQL as x-axis when querying chart with granularity~~ fix(chart): Supporting custom SQL as temporal x-axis column with filter Sep 6, 2023

zephyring marked this pull request as ready for review September 6, 2023 23:19

kgabryje and others added 4 commits September 11, 2023 16:38

Update control only if control exists in current viz type

f65d707

skip test for now

08a50ce

Merge remote-tracking branch 'origin/zef/fix_x_axis_custom_sql' into …

a76ed47

…zef/fix_x_axis_custom_sql

Update tests

d16ffbb

pull-request-size bot added size/L and removed size/M labels Sep 14, 2023

zef added 3 commits September 14, 2023 09:11

fixing test

16f941d

Merge remote-tracking branch 'origin/zef/fix_x_axis_custom_sql' into …

91788c1

…zef/fix_x_axis_custom_sql

Merge remote-tracking branch 'origin/master' into zef/fix_x_axis_cust…

bb52a1a

…om_sql

villebro approved these changes Sep 15, 2023

View reviewed changes

superset/common/query_context_factory.py Outdated Show resolved Hide resolved

zef added 2 commits September 15, 2023 09:31

using is_adhoc_column to determine x_axis

5c79619

addressing comments and fixed a small issue where custom sql can be e…

64e01b6

…mpty

fixed tests

672eea4

villebro reviewed Sep 15, 2023

View reviewed changes

zephyring commented Sep 15, 2023

View reviewed changes

eschutho merged commit c8c9482 into apache:master Sep 18, 2023
29 checks passed

eschutho deleted the zef/fix_x_axis_custom_sql branch September 18, 2023 18:30

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Sep 18, 2023

eschutho pushed a commit to Superset-Community-Partners/superset that referenced this pull request Sep 21, 2023

fix(chart): Supporting custom SQL as temporal x-axis column with filt…

65e5ea1

…er (apache#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>

michael-s-molina pushed a commit that referenced this pull request Sep 25, 2023

fix(chart): Supporting custom SQL as temporal x-axis column with filt…

8ca49d4

…er (#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>

zhaoyongjie reviewed Sep 25, 2023

View reviewed changes

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

fix(chart): Supporting custom SQL as temporal x-axis column with filt…

ef4b6f0

…er (apache#25126) Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>

mistercrunch added 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(chart): Supporting custom SQL as temporal x-axis column with filter #25126

fix(chart): Supporting custom SQL as temporal x-axis column with filter #25126

zephyring commented Aug 30, 2023 •

edited

Loading

villebro left a comment

villebro commented Aug 30, 2023

kgabryje commented Sep 15, 2023

github-actions bot commented Sep 15, 2023

villebro left a comment

villebro Sep 15, 2023

zephyring Sep 15, 2023

villebro Sep 15, 2023

zephyring Sep 15, 2023

zhaoyongjie Sep 25, 2023

zephyring Sep 25, 2023

zephyring Sep 15, 2023

kgabryje Sep 15, 2023

villebro Sep 15, 2023

zephyring Sep 15, 2023 •

edited

Loading

zephyring Sep 15, 2023

github-actions bot commented Sep 18, 2023

zhaoyongjie Sep 25, 2023

zhaoyongjie Sep 25, 2023

zero-element Oct 8, 2023 •

edited

Loading

fix(chart): Supporting custom SQL as temporal x-axis column with filter #25126

fix(chart): Supporting custom SQL as temporal x-axis column with filter #25126

Conversation

zephyring commented Aug 30, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro left a comment

Choose a reason for hiding this comment

villebro commented Aug 30, 2023

kgabryje commented Sep 15, 2023

github-actions bot commented Sep 15, 2023

villebro 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

zephyring Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero-element Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

zephyring commented Aug 30, 2023 •

edited

Loading

zephyring Sep 15, 2023 •

edited

Loading

zero-element Oct 8, 2023 •

edited

Loading