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: Revert "fix: Apply normalization to all dttm columns (#25147)" #25801

Merged
merged 1 commit into from
Oct 31, 2023
Merged

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

merged 1 commit into from
Oct 31, 2023

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Oct 30, 2023 •
edited
Loading

SUMMARY

This PR reverts #25147 as we detected an issue after deploying 3.0.1 where the native UNIX time wasn't correctly converted to adhere to the Python date time format of the underlying column.

For example the ds column is a VARCHAR encoded via the %Y-%m-%d format yet was being treated as a UNIX timestamp.

Screenshot 2023-10-27 at 2 34 35 PM

I was able to isolate this specific change as the problematic PR by deploying an augmented version of 3.0.1 with said commit reverted. Note the reason for reverting as opposed to fixing forward is there seems to be a number of open comments with regards to the change and thus it seems more prudent to roll back and reimplement the logic. Additionally it is overly clear—per the PR description—what underlying issue the normalization/denormalization logic was trying to resolve.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

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

@john-bodley john-bodley changed the title revert: "fix: Apply normalization to all dttm columns (#25147)" fix(revert): "fix: Apply normalization to all dttm columns (#25147)" Oct 30, 2023
@john-bodley john-bodley changed the title fix(revert): "fix: Apply normalization to all dttm columns (#25147)" fix: Revert "fix: Apply normalization to all dttm columns (#25147)" Oct 30, 2023
@kgabryje
Copy link
Member

Thank you for finding the source of the problem @john-bodley! Let's merge the revert and I'll follow up with your comments next week

@michael-s-molina
Copy link
Member

Talked to @kgabryje and we agreed to revert this now to give us time to address the bug and comments in the original PR.

@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 Oct 31, 2023
@michael-s-molina michael-s-molina merged commit ae703b4 into apache:master Oct 31, 2023
35 of 38 checks passed
michael-s-molina pushed a commit that referenced this pull request Oct 31, 2023
@john-bodley @michael-s-molina
fix: Revert "fix: Apply normalization to all dttm columns (#25147)" (#…
c216b3e 
…25801)
@john-bodley john-bodley deleted the john-bodley--revert-25618 branch October 31, 2023 15:32
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>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@john-bodley
fix: Revert "fix: Apply normalization to all dttm columns (apache#25147…
1251704 
…)" (apache#25801)
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@john-bodley @sfirke
fix: Revert "fix: Apply normalization to all dttm columns (apache#25147…
573e8ba 
…)" (apache#25801)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@kgabryje kgabryje Awaiting requested review from kgabryje

@betodealmeida betodealmeida Awaiting requested review from betodealmeida

Assignees
No one assigned
Labels
size/L v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@john-bodley @kgabryje @michael-s-molina