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.

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

feat: Add quarter unit to datetrunc #17416

Merged

Conversation

john-bodley
Copy link
Member

SUMMARY

It's somewhat typical to report for the current quarter (QTD) and currently the only way to do this is to define a broad time range and then add a custom SQL filter based on the current timestamp.

This request has been mentioned in a couple of Stack Overflow questions:

MTD, YTD, etc. are supported via the sweet date parse logic—kudos to whoever added this functionality—using the datetrunc functionality, thus I thought I should extend this to also include the quarter unit.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen Shot 2021-11-11 at 8 19 15 PM

TESTING INSTRUCTIONS

Added unit test.

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

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

LGTM, just a comment on the week parsing.

Also... the code in the parser doesn't even look like Python. Crazy.

elif unit == "month":
dttm = dttm.replace(day=1, hour=0, minute=0, second=0, microsecond=0)
elif unit == "week":
dttm = dttm - relativedelta(days=dttm.weekday())
dttm = -relativedelta(days=dttm.weekday())
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? Or did you mean ddtm -= relativedelta(...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops.

@codecov
Copy link

codecov bot commented Nov 12, 2021

Codecov Report

Merging #17416 (c5f3dab) into master (ffa55f7) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17416      +/-   ##
==========================================
- Coverage   77.01%   76.80%   -0.22%     
==========================================
  Files        1040     1040              
  Lines       56077    56080       +3     
  Branches     7738     7738              
==========================================
- Hits        43190    43072     -118     
- Misses      12629    12750     +121     
  Partials      258      258              
Flag Coverage Δ
hive ?
javascript 71.22% <ø> (ø)
mysql 81.89% <100.00%> (+<0.01%) ⬆️
postgres 81.90% <100.00%> (+<0.01%) ⬆️
presto ?
python 81.98% <100.00%> (-0.42%) ⬇️
sqlite 81.58% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...teFilterControl/components/DateFunctionTooltip.tsx 100.00% <ø> (ø)
superset/utils/date_parser.py 96.63% <100.00%> (+0.04%) ⬆️
superset/db_engines/hive.py 0.00% <0.00%> (-85.19%) ⬇️
superset/db_engine_specs/hive.py 69.49% <0.00%> (-16.99%) ⬇️
superset/db_engine_specs/presto.py 83.47% <0.00%> (-6.91%) ⬇️
superset/views/database/mixins.py 81.03% <0.00%> (-1.73%) ⬇️
superset/connectors/sqla/models.py 86.35% <0.00%> (-1.62%) ⬇️
superset/models/core.py 89.26% <0.00%> (-0.74%) ⬇️
superset/db_engine_specs/base.py 88.20% <0.00%> (-0.39%) ⬇️
superset/utils/core.py 89.98% <0.00%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffa55f7...c5f3dab. Read the comment docs.

@@ -322,10 +323,12 @@ def eval(self) -> datetime:
dttm = dttm.replace(
month=1, day=1, hour=0, minute=0, second=0, microsecond=0
)
if unit == "quarter":
dttm = datetime(dttm.year, 3 * pd.Timestamp(dttm).quarter - 2, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Try

dttm = pd.Period(pd.Timestamp(dttm), freq=FREQ_STR).to_timestamp()

@john-bodley
Copy link
Member Author

@betodealmeida

Also... the code in the parser doesn't even look like Python. Crazy.

I totally agree. I'm like whoa what is this.

@john-bodley john-bodley changed the title feature: Add quarter unit to datetrunc feat: Add quarter unit to datetrunc Nov 12, 2021
@john-bodley john-bodley merged commit bcef8fa into apache:master Nov 12, 2021
@john-bodley john-bodley deleted the john-bodley--datetrunc-quarter branch November 12, 2021 17:46
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
Co-authored-by: John Bodley <john.bodley@airbnb.com>
@a-cid
Copy link

a-cid commented Apr 4, 2022

This is would be a major usability improvement for my org and seems to be a small localized change. Any chance it can be included it in the next release? @villebro

@villebro
Copy link
Member

villebro commented Apr 5, 2022

@a-cid this PR is included in the 1.5 cut, so will be included in the forthcoming 1.5.0 release once the vote passes.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants