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.

Sign up for GitHub

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

Jump to bottom

fix: always denorm column value before querying values #25919

Merged
merged 8 commits into from
Nov 13, 2023
Merged

fix: always denorm column value before querying values #25919

merged 8 commits into from
Nov 13, 2023

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Nov 8, 2023

SUMMARY

currently with some databases/engines the /<datasource_type>/<int:datasource_id>/column/<column_name>/values/ endpoint values due to malformatted queries. One of the edge cases has to do with column names not being denormalized. To fix this we'll denormalize the column name before querying for values

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

betodealmeida
betodealmeida previously requested changes Nov 8, 2023
View reviewed changes
superset/datasource/api.py Outdated
Comment on lines 119 to 124
# always denormalize column name before querying for values
db_engine_spec = datasource.database.db_engine_spec
db_dialect = datasource.database.get_dialect()
denomalized_col_name = db_engine_spec.denormalize_name(
db_dialect, column_name
)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to move this logic to the datasource.values_for_column method, so that the API doesn't have any business logic, and so that calls to datasource.values_for_column from other places also work as expected.

@pull-request-size pull-request-size bot added size/M and removed size/XS labels Nov 8, 2023
@hughhhh hughhhh force-pushed the filter-value-denorm branch from 5a79ca2 to 29b45fb Compare November 9, 2023 00:50
hughhhh
hughhhh commented Nov 9, 2023
View reviewed changes
superset/models/helpers.py
tp = None # todo(hughhhh): add back self.get_template_processor()
# always denormalize column name before querying for values
db_dialect = self.database.get_dialect()
denomalized_col_name = self.database.db_engine_spec.denormalize_name(
Copy link
Member Author

Choose a reason for hiding this comment

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

denomarlized name will return the original value if it can't be adjusted by the engine:
preset-io/superset@60e1526/superset/db_engine_specs/base.py#L1950

@john-bodley john-bodley self-requested a review November 9, 2023 17:49
jfrag1
jfrag1 approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@jfrag1 jfrag1 left a comment

Choose a reason for hiding this comment

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

LGTM

superset/models/helpers.py
tp = None # todo(hughhhh): add back self.get_template_processor()
# always denormalize column name before querying for values
db_dialect = self.database.get_dialect()
denomalized_col_name = self.database.db_engine_spec.denormalize_name(
Copy link
Member

Choose a reason for hiding this comment

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

nit: denomalized -> denormalized

@hughhhh hughhhh merged commit 8d8e1bb into apache:master Nov 13, 2023
29 checks passed
@hughhhh hughhhh deleted the filter-value-denorm branch November 13, 2023 18:18
@hughhhh hughhhh mentioned this pull request Nov 13, 2023
Merged
9 tasks
jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Nov 13, 2023
@hughhhh @jinghua-qa
fix: always denorm column value before querying values (apache#25919)
4bb2925 
(cherry picked from commit 8d8e1bb)
@michael-s-molina 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 Nov 13, 2023
michael-s-molina pushed a commit that referenced this pull request Nov 16, 2023
@hughhhh @michael-s-molina
fix: always denorm column value before querying values (#25919)
e07eed1
saghatelian added a commit to 10webio/superset that referenced this pull request Nov 27, 2023
@saghatelian @giftig
@Khrol @Always-prog @dpgaspar @rtexelm @geido @betodealmeida @justinpark @eschutho @john-bodley @michael-s-molina @mapledan @SA-Ark @Antonio-RiveroMartnez @kgabryje @FGrobelny @ggbaro @villebro @hughhhh @josedev-union @yousoph @jfrag1
3.0 (#3)
8651b24 
* fix(sqllab): reinstate "Force trino client async execution" (apache#25680)

* fix: remove unnecessary redirect (apache#25679)

(cherry picked from commit da42bf2)

* fix(chore): dashboard requests to database equal the number of slices it has (apache#24709)

(cherry picked from commit 75a7431)

* fix: bump to FAB 4.3.9 remove CSP exception (apache#25712)

(cherry picked from commit 8fb0c8d)

* fix(horizontal filter label): show full tooltip with ellipsis (apache#25732)

(cherry picked from commit e4173d9)

* fix: Revert "fix(Charts): Set max row limit + removed the option to use an empty row limit value" (apache#25753)

(cherry picked from commit e2fe967)

* fix: dataset update uniqueness (apache#25756)

(cherry picked from commit c7f8d11)

* fix(sqllab): slow pop datasource query (apache#25741)

(cherry picked from commit 2a2bc82)

* fix: allow for backward compatible errors (apache#25640)

* fix: DB-specific quoting in Jinja macro (apache#25779)

(cherry picked from commit 5659c87)

* fix: Revert "fix: Apply normalization to all dttm columns (apache#25147)" (apache#25801)

* fix: Resolve issue apache#24195 (apache#25804)

(cherry picked from commit 8737a8a)

* fix(SQL field in edit dataset modal): display full sql query (apache#25768)

(cherry picked from commit 1eba712)

* fix(sqllab): infinite fetching status after results are landed (apache#25814)

(cherry picked from commit 3f28eeb)

* fix: Fires onChange when clearing all values of single select (apache#25853)

(cherry picked from commit 8061d5c)

* fix: the temporal x-axis results in a none time_range. (apache#25429)

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

* fix(table chart): Show Cell Bars correctly apache#25625 (apache#25707)

(cherry picked from commit 916f7bc)

* fix: remove `update_charts_owners` (apache#25843)

* fix(charts): Time grain is None when dataset uses Jinja (apache#25842)

(cherry picked from commit 7536dd1)

* fix: Saving Mixed Chart with dashboard filter applied breaks adhoc_filter_b (apache#25877)

(cherry picked from commit 268c1dc)

* fix: database version field (apache#25898)

(cherry picked from commit 06ffcd2)

* fix: trino cursor (apache#25897)

(cherry picked from commit cdb18e0)

* chore: Updates CHANGELOG.md for 3.0.2

* fix(trino): allow impersonate_user flag to be imported (apache#25872)

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
(cherry picked from commit 458be8c)

* fix(table): Double percenting ad-hoc percentage metrics (apache#25857)

(cherry picked from commit 784a478)

* fix(sqllab): invalid sanitization on comparison symbol (apache#25903)

(cherry picked from commit 581d3c7)

* fix: update flask-caching to avoid breaking redis cache, solves apache#25339 (apache#25947)

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* fix: always denorm column value before querying values (apache#25919)

* chore(colors): Updating Airbnb brand colors (apache#23619)

(cherry picked from commit 6d8424c)

* fix: naming denomalized to denormalized in helpers.py (apache#25973)

(cherry picked from commit 5def416)

* fix(helm): Restart all related deployments when bootstrap script changed (apache#25703)

* fix(rls): Update text from tables to datasets in RLS modal (apache#25997)

(cherry picked from commit 210f1f8)

* fix: Make Select component fire onChange listener when a selection is pasted in (apache#25993)

(cherry picked from commit 5fccf67)

* fix(explore): redandant force param (apache#25985)

(cherry picked from commit e7a1876)

* chore: Optimize fetching samples logic (apache#25995)

(cherry picked from commit 326ac4a)

* fix(native filters): rendering performance improvement by reduce overrendering (apache#25901)

(cherry picked from commit e1d73d5)

* fix: update FAB to 4.3.10, Azure user info fix (apache#26037)

(cherry picked from commit 628cd34)

* chore: Updates CHANGELOG.md for 3.0.2 (rc2)

---------

Co-authored-by: Rob Moore <giftig@users.noreply.github.com>
Co-authored-by: Igor Khrol <igor.khrol@automattic.com>
Co-authored-by: Stepan <66589759+Always-prog@users.noreply.github.com>
Co-authored-by: Daniel Vaz Gaspar <danielvazgaspar@gmail.com>
Co-authored-by: Ross Mabbett <92495987+rtexelm@users.noreply.github.com>
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
Co-authored-by: mapledan <mapledan829@gmail.com>
Co-authored-by: Arko <90512504+SA-Ark@users.noreply.github.com>
Co-authored-by: Antonio Rivero <38889534+Antonio-RiveroMartnez@users.noreply.github.com>
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>
Co-authored-by: FGrobelny <150029280+FGrobelny@users.noreply.github.com>
Co-authored-by: Giacomo Barone <46573388+ggbaro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com>
Co-authored-by: josedev-union <70741025+josedev-union@users.noreply.github.com>
Co-authored-by: yousoph <sophieyou12@gmail.com>
Co-authored-by: Jack Fragassi <jfragassi98@gmail.com>
josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024
@hughhhh @josedev-union
fix: always denorm column value before querying values (apache#25919)
d953d06
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@hughhhh
fix: always denorm column value before querying values (apache#25919)
c11ccf4
@mistercrunch mistercrunch added 🍒 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
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@hughhhh @sfirke
fix: always denorm column value before querying values (apache#25919)
48a6901
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@hughhhh
fix: always denorm column value before querying values (apache#25919)
cfb606e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfrag1 jfrag1 jfrag1 approved these changes

@eschutho eschutho eschutho approved these changes

@john-bodley john-bodley Awaiting requested review from john-bodley

@betodealmeida betodealmeida Awaiting requested review from betodealmeida

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset:2023.45 size/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hughhhh @betodealmeida @eschutho @jfrag1 @mistercrunch @michael-s-molina @jinghua-qa