superset icon indicating copy to clipboard operation
superset copied to clipboard

fix(cal-heatmap): calendar heatmap rendering null month fix

Open Bunbunw opened this issue 3 weeks ago • 18 comments

User description

SUMMARY

This is a fix related to issue#21870.

Unexpected Behaviour: In calendar heatmap, when the end date is the start day of the month, an extra null month is rendered, the null month is either rendered before or after based on local time offset compared to UTC. Expected Behaviour: When rendering date, if first day of a month is selected as the end date, default going to 12AM, the month containing this date should not be rendered.

IMPLEMENTATION DETAILS FOR REFERENCE

Modified logic related to plugin calendar-heatmap month-day rendering behaviour:

  • cal-heatmaps.js contains a function named “getMonthDomain” responsible for returning time range for frontend render, replacing javascript Date methods with UTC equivalent to remove local time zone offset issue. Our rationale for this change is to preserve consistent time ranges between users in different time zones. We noticed our reproduction of the issue resulted in a null month prior to the time range of the chart, compared to the issuer’s example. This is because our time zone is in EST, which is 4-5 hours behind UTC depending on daylight savings. The issuer’s Superset image was in the Dutch language, so we assumed CET, an hour ahead of UTC. This is why the null month placement was different in our reproduction compared to theirs.
  • viz.py contains the backend class definition for “CalHeatmapViz”. Subtracted 1 day for end, when calculating range for first day of the month, to enforce exclusive end date, as desired for Superset’s Calendar Heatmap chart definition. This gets rid of null month rendering behaviour.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Setup steps, create a dataset using users:

Before any fix (Based in EST): Screenshot 2025-11-27 at 7 17 37 PM After adding only UTC functions: Screenshot 2025-11-27 at 7 17 56 PM After both UTC and viz.py updates: Screenshot 2025-11-27 at 7 18 17 PM

TESTING INSTRUCTIONS

Testing this is normal usage of superset, for details:

  1. Sync superset repo and start the application on localhost using docker compose up --build and login localhost.
  2. Take any dataset on your Superset instance with date-value data points and open a Calendar Heatmap chart with it. (For the following test cases, our examples used superset’s preset dataset main.users with the MIN(# tz_offset) metrics)
  3. Check what dates your data points occur.
  4. Here are some cases to attempt: a) Original issue case: 1st of a month to 1st of another month 2 months away, like March 1st to May 1st. Screenshot 2025-11-27 at 7 18 40 PM

b) Within same month: when selecting March 1st to March 31st, make sure the same month range is possible with no repeat of the null month issue. Screenshot 2025-11-27 at 7 20 04 PM

c) >1 year case: like January 1st to January 1st of subsequent year, to make sure a range with years considered is displaying correctly. Screenshot 2025-11-27 at 7 20 13 PM

ADDITIONAL INFORMATION

  • [x] Has associated issue: this pr is only related to and fixes #21870
  • [ ] Required feature flags:
  • [x] Changes UI
  • [ ] Includes DB Migration (follow approval process in [SIP-59] (https://github.com/apache/superset/issues/13351))
  • [ ] 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

CodeAnt-AI Description

Fix calendar heatmap extra empty month and make month ranges timezone-consistent

What Changed

  • Calendar heatmap no longer shows an extra empty/null month when the "Until" date is the first day of a month; that month is now excluded from the rendered range.
  • Month boundaries are calculated using UTC so users in different time zones see the same month range and the null-month location no longer depends on local offset.
  • Month sequences across leap years and year boundaries are handled correctly (e.g., Feb 29 on leap years and multi-year ranges).
  • Added unit tests covering first-of-month end dates, leap-year February, Date-object ranges, and multi-year ranges to prevent regressions.

Impact

✅ No extra blank month in calendar heatmap when "Until" is the first of a month ✅ Consistent calendar heatmap month ranges across time zones ✅ Correct month sequences for leap years and year-crossing selections

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Bunbunw avatar Nov 28 '25 00:11 Bunbunw

A few suggestions/questions (thanks, Claude!) which you can take on or let me know if it's crazy talk:

The viz.py change looks incomplete — they add diff_delta and diff_secs calculations, but diff_secs is never used. The original code presumably calculated these elsewhere, so this might break something. I'd want to see where diff_delta and diff_secs were originally defined to make sure this doesn't introduce a regression.

Test coverage is thin — the test only checks one case (numeric range). It should also test the Date range branch: const range = new Date(Date.UTC(2025, 1, 1)); // Feb 1st

Edge case for range_ = diff_delta.years * 12 + diff_delta.months + 1 — after subtracting a day from end, if the original end was exactly the first of a month, the +1 might now be off-by-one. Would want a quick sanity check on boundary cases.

Variable rename inconsistency — stopstopExclusive is helpful for clarity, but the test comment says "2025-02-31" which isn't a valid date.

Claude's recommendation: Request minor changes — ask the contributor to verify the viz.py logic (especially unused diff_secs), add one more test case for the Date range path, and fix the test comment. Then it's good to merge.

rusackas avatar Nov 28 '25 20:11 rusackas

Codecov Report

:x: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 67.77%. Comparing base (ab8352e) to head (c9cd60c). :warning: Report is 137 commits behind head on master.

Files with missing lines Patch % Lines
superset/viz.py 0.00% 2 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #36315       +/-   ##
===========================================
+ Coverage        0   67.77%   +67.77%     
===========================================
  Files           0      637      +637     
  Lines           0    47103    +47103     
  Branches        0     5131     +5131     
===========================================
+ Hits            0    31923    +31923     
- Misses          0    13903    +13903     
- Partials        0     1277     +1277     
Flag Coverage Δ
hive 43.50% <0.00%> (?)
mysql 66.83% <0.00%> (?)
postgres 66.88% <0.00%> (?)
presto 47.14% <0.00%> (?)
python 67.74% <0.00%> (?)
sqlite 66.60% <0.00%> (?)
unit 100.00% <ø> (?)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Nov 28 '25 20:11 codecov[bot]

viz.py diff_secs issue: Yeah, that diff_secs calculation should be removed, it does nothing. The diff_delta variable is originally defined before the if/else block checking what the domain is, like this: domain = form_data.get("domain_granularity") diff_delta = rdelta.relativedelta(end, start) diff_secs = (end - start).total_seconds() And nowhere else in viz.py. All we are doing is recalculating it within the month else if block (nowhere else) to enforce the exclusive end date that was not present beforehand.

Test coverage is lacking: Modified edge case tests of "end date being first of month" test to check each month in test range and length of array itself. Added additional tests for leap year, range passed as Date object, and time range crossing between years.

sanity check on boundary cases: image

Here are sample ranges, March 1st 2021 to March 30th 2021, May 1st 2021, and May 2nd 2021. All ranges render as intended, excluding the last day as the UI indicates.

invalid test date issue: Changed January 1st 2025 to February 31st 2025 test case, to March 1st 2025 to May 1st 2025. Also fixed spelling error.

CoolDude937 avatar Nov 29 '25 02:11 CoolDude937

Running CI 🤞

rusackas avatar Dec 01 '25 18:12 rusackas

Looks like pre-commit is failing, which might unblock the other CI workflows.

Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/development.txt
pre-commit install

A series of checks will now run when you make a git commit.

Alternatively it is possible to run pre-commit by running pre-commit manually:

pre-commit run --all-files

rusackas avatar Dec 01 '25 19:12 rusackas

okay, so just to confirm: -run pre-commit locally on my PC -see what bugs/issues it produces -fix and re-commit to fork, add to pull request?

Not sure how the last part works yet, but just wondering if that is what the workflow looks like, as I have not heard of pre-commit before.

CoolDude937 avatar Dec 01 '25 20:12 CoolDude937

Running pre-commit should change some files (fixing small formatting issues), and you can just commit/push those changes to this PR.

rusackas avatar Dec 01 '25 23:12 rusackas

I'm getting a lot of installation issues. It might be because I'm on a Windows, or because of my Python version, not sure: Getting requirements to build wheel ...error error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully. │ exit code: 1 ╰─> [52 lines of output] Compiling src/gevent/resolver/cares.pyx because it changed. [1/1] Cythonizing src/gevent/resolver/cares.pyx

  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  cdef tuple integer_types

  if sys.version_info[0] >= 3:
      integer_types = int,
  else:
      integer_types = (int, long)      
                            ^
  ------------------------------------------------------------
  src\gevent\libev\corecext.pyx:69:26: undeclared name not builtin: long

And this one, when I switched to WSL, where I think it was not able to install this "execvpe" package:
prettier (frontend)......................................................Failed - hook id: prettier-frontend

  • exit code: 1

            <3>WSL (18 - Relay) ERROR: CreateProcessCommon:798: execvpe(/bin/bash) failed: No such file or directory
          <3>WSL (16 - Relay) ERROR: CreateProcessCommon:798: execvpe(/bin/bash) failed: No such file or directory
        <3>WSL (14 - Relay) ERROR: CreateProcessCommon:798: execvpe(/bin/bash) failed: No such file or directory
      <3>WSL (10 - Relay) ERROR: CreateProcessCommon:798: execvpe(/bin/bash) failed: No such file or directory
    

Then I saw pyproject.toml in the root had Python 3.10, 3.11, and 3.12 specified, so I tried an environment in WSL with 3.10 and reinstalled the requirements.txt stuff, but then I ran into this running the install: Getting requirements to build editable ... error error: subprocess-exited-with-error

× Getting requirements to build editable did not run successfully. │ exit code: 1 ╰─> [42 lines of output] /tmp/pip-build-env-4pfzm92t/overlay/local/lib/python3.10/dist-packages/setuptools/config/_apply_pyprojecttoml.py:82: SetuptoolsDeprecationWarning: project.license as a TOML table is deprecated !!

          ********************************************************************************
          Please use a simple string containing a SPDX expression for `project.license`. You can also use `project.license-files`. (Both options available on setuptools>=77.0.0).

          By 2026-Feb-18, you need to update your project and remove deprecated calls
          or your builds will no longer be supported.

          See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
          ********************************************************************************

  !!
    corresp(dist, value, root_dir)
  /tmp/pip-build-env-4pfzm92t/overlay/local/lib/python3.10/dist-packages/setuptools/config/_apply_pyprojecttoml.py:61: SetuptoolsDeprecationWarning: License classifiers are deprecated.
  !!

          ********************************************************************************
          Please consider removing the following classifiers in favor of a SPDX license expression:   

          License :: OSI Approved :: Apache Software License

          See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
          ********************************************************************************

  !!
    dist._finalize_license_expression()
  /tmp/pip-build-env-4pfzm92t/overlay/local/lib/python3.10/dist-packages/setuptools/dist.py:759: SetuptoolsDeprecationWarning: License classifiers are deprecated.
  !!

          ********************************************************************************
          Please consider removing the following classifiers in favor of a SPDX license expression:   

          License :: OSI Approved :: Apache Software License

          See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
          ********************************************************************************

  !!
    self._finalize_license_expression()
  running egg_info
  error: Cannot update time stamp of directory 'src/apache_superset_core.egg-info'
  [end of output]         

So I think there are a lot of problems trying to do this on my PC. I am currently considering docker options, would this command do something similar to what you suggested: docker compose run --rm superset pre-commit run --all-files

CoolDude937 avatar Dec 02 '25 03:12 CoolDude937

Heya... sorry for the slow reply. I don't have Windows, nor does anyone I know at Preset. Officially Superset doesn't support Windows, and while some people DO find success with it, they haven't contributed any docs around their tips and tricks.

That said, it should absolutely work on Mac or Linux. Maybe @dosu has some recommendations here if it's able to chime in on PRs.

rusackas avatar Dec 02 '25 21:12 rusackas

You can run pre-commit hooks inside the Superset Docker container, and this is a reliable workaround for Windows/WSL environments where native setup is problematic. The dev image includes all necessary dependencies and mounts your local repo, so any formatting fixes or changes will be reflected in your files.

To do this, start your containers with docker compose up --build, then run:

docker compose exec superset_app bash
pre-commit run --all-files

Or, for a one-off command:

docker compose run --rm superset pre-commit run --all-files

This approach works on any OS and is functionally equivalent to running pre-commit on Mac or Linux. If you hit issues, make sure your repo is mounted correctly and the dev dependencies are installed in the container. For more details, see the Superset development docs.

To reply, just mention @dosu.


How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other

Chat with Dosu Open in Cursor Join Discord Share on X

dosubot[bot] avatar Dec 02 '25 21:12 dosubot[bot]

My teammate attempted the pre-commit commands on his MacOS, and got these errors:

First image: image

Second image: image

Third image: image

I think given the fact it did not run on my Windows PC and their MacOS PC, it could be a Python version mismatch. Is there a specific version of Python this should be run on? On my PC I am also trying Docker methods, and I am also failing to build mysqlclient.

I will continue to try and figure this out. I will give an update if I make progress.

CoolDude937 avatar Dec 04 '25 02:12 CoolDude937

CodeAnt AI is reviewing your PR.

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • [ ] Possible Negative/Incorrect Range
    Subtracting one day from end when domain == "month" can make end earlier than start. Using relativedelta and then computing the months from a potentially inverted interval may produce a negative or otherwise incorrect range_. This can lead to off-by-one or zero/negative ranges in edge cases (e.g., start == end == first-of-month), causing unexpected rendering behavior.

  • [ ] Lack of defensive checks / tests
    The change alters boundary logic for month ranges but there are no guards or tests in the same change. Recommend adding unit tests for edge cases (e.g., end is first day of month, start == end, timezone-aware datetimes) and defensive code to prevent negative ranges.

  • [ ] Timezone Inconsistency
    The new getMonthDomain uses UTC-based Date construction (Date.UTC / getUTCFullYear / getUTCMonth) to build month boundaries. Much of the rest of the code (e.g., month extraction/comparison) uses local getters/constructors (getFullYear/getMonth / new Date(...)). This mixing of UTC and local date methods can produce off-by-one-month behaviour (especially around midnight UTC in timezones behind/ahead of UTC). Verify all month-related code paths (domain keys, extractUnit, comparisons, parseDatas, getDomainKeys) are consistent with UTC handling or explicitly normalized to local timezone.

  • [ ] Range Ordering / Edge Cases
    getMonthDomain constructs a start and a stopExclusive but does not ensure start <= stopExclusive when range is a Date that might be before the provided d. d3.time.months expects a proper [start, stop) ordering — failing to normalize order may produce unexpected results for backward ranges or negative ranges. Also check numeric range negative values and ensure setUTCMonth handles them correctly.

  • [ ] Flaky / Timezone-sensitive test
    The tests verify month boundaries using UTC-based Dates and string containment of ISO strings. There is risk of flakiness if some environments construct Dates in local time or if implementations of getMonthDomain return Date objects with non-zero time components. Add assertions that are timezone-robust and add coverage for local-time Date inputs to ensure consistent behavior across environments.

  • [ ] Weak assertions
    Several assertions use toContain('YYYY-MM-DD') which is permissive and may hide off-by-hour bugs (e.g., '2025-03-01T23:00:00.000Z' still contains '2025-03-01'). Tests should assert full ISO strings or explicit UTC year/month/day equality to catch subtle timezone shifts.

  • [ ] Missing local Date coverage
    The test that exercises the Date-object-range case constructs Dates via Date.UTC. It would be valuable to add a test that passes Dates created with the local constructor (new Date(year, monthIndex, day)) to ensure the plugin is robust against local timezone offsets.

CodeAnt AI finished reviewing your PR.

These set of lines fired off once I pushed a fix for a comment that was still incorrect: auto-walrus..........................................(no files to check)Skipped
mypy (main)..........................................(no files to check)Skipped
mypy (superset-extensions-cli).......................(no files to check)Skipped
check docstring is first.............................(no files to check)Skipped
check for added large files..............................................Passed check yaml...........................................(no files to check)Skipped
debug statements (python)............................(no files to check)Skipped
fix end of files.........................................................Passed trim trailing whitespace.................................................Passed prettier (frontend)......................................................Passed oxlint (frontend)........................................................Passed custom rules (frontend)..................................................Passed eslint (docs)........................................(no files to check)Skipped
Type-Checking (Frontend).................................................Passed Blacklist............................................(no files to check)Skipped
Helm Docs............................................(no files to check)Skipped
ruff-format..........................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
pylint with custom Superset plugins..................(no files to check)Skipped
[issue-21870-newer-fix b71591a9f] still fixing comment 1 file changed, 6 insertions(+), 6 deletions(-)

I guess the issue was just that running manual pre-commit did not work? I hope this is sufficient for GitHub CI to pass. There are no fails, at the very least.

CoolDude937 avatar Dec 05 '25 04:12 CoolDude937

Running CI... we shall see!

rusackas avatar Dec 06 '25 00:12 rusackas

CodeAnt AI is running Incremental review

Sorry for not commenting, CI failed on cal-heatmap.js and its testing file, so I am doing an arbitrary comment change to push pre-commit on them again. I hope that is ok.

CoolDude937 avatar Dec 06 '25 01:12 CoolDude937

CodeAnt AI is running Incremental review

@rusackas it was a prettier reformatting issue on the cal-heatmap.js file. I pushed prettier changes (using work PC so that is why committer name is weird) and CI appears to have passed. Is it safe to merge now, or are there any other checks we need to make?

CoolDude937 avatar Dec 11 '25 19:12 CoolDude937

It seems there are still some issues that we might want to contend with:

  1. Removal of 1Math.min1/Math.max — The original code handled backward ranges gracefully. The new code assumes start < stopExclusive, which could break if someone passes a negative range number or a Date before d.

  2. Timezone inconsistency — getMonthDomain now uses UTC but getWeekDomain, getYearDomain still use local time. CodeAnt flagged this too. Could cause subtle mismatches.

  3. Test assertion strengthening — Using toContain('2025-03-01') is permissive. '2025-03-01T23:00:00.000Z' still passes that check even if there's an off-by-hour bug.

  4. No Python-side test coverage — The viz.py change has 0% patch coverage per Codecov. Not the end of the world, but there might be an edge case. If start == end and both are the first of a month, subtracting a day makes end < start, potentially yielding a zero or negative range.

This is super close... if you're using some AI IDE, I'll bet it can bang out these final touch-ups super quickly :D Again, thanks for your contribution and your patience with this!

rusackas avatar Dec 16 '25 21:12 rusackas