superset icon indicating copy to clipboard operation
superset copied to clipboard

feat(Timeseries): Respect time grain

Open gerbermichi opened this issue 11 months ago • 11 comments

User description

SUMMARY

Respect time grain in timeseries and mixed timeseries chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

image

AFTER

image

TESTING INSTRUCTIONS

Create a Timeseries or MixedTimeseries chart and use the time grain Quarter.

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [ ] Required feature flags:
  • [x] 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

CodeAnt-AI Description

Respect time grain for x-axis and tooltip date formatting in Timeseries and MixedTimeseries charts

What Changed

  • X-axis labels and tooltip date strings now honor the chart's selected time grain (day, week, month, quarter, year), showing appropriate ranges and concise labels (for example: weekly ranges, "Jan 2021" for months, "Q1 2021" for quarters, "2021" for years).
  • Time grain overrides supplied to the chart (for example, via dashboard/extra form overrides) are applied to both x-axis and tooltip formatting.
  • The SMART_DATE verbose formatter now produces verbose date strings that match the selected time grain.
  • Unit tests added for Timeseries and MixedTimeseries to validate formatting across days, weeks, months, quarters, and years.

Impact

✅ Clearer time labels for weekly/monthly/quarterly/yearly charts ✅ Consistent tooltip dates with configured time grain ✅ Fewer incorrect date displays when dashboards override time grain

💡 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.

gerbermichi avatar Jan 09 '25 10:01 gerbermichi

You need to run the pre-commit hook to pass CI.

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 Jan 09 '25 18:01 rusackas

@gerbermichi thanks for this PR! A few questions/requests:

  1. The PR description is very light on details for all the changes in the PR. Would it be possible to cover all changes in the description to provide more context to reviewers?
  2. I see there's a change for both i18n and time grain formatting in this PR - would it be possible to split the PR into two different PRs?
  3. I know testing i18n related features can be challenging, but would it be possible to add unit tests for the time grain formatting? Adding a test that failed before but passes after the changes tends to be a great way of showing what the current issue is, how the added/changed logic addresses that, and finally protect against future regressions.

villebro avatar Jan 09 '25 18:01 villebro

@villebro thank you for your feedback. I removed the i18n part and will add it in a new PR.

I also added tests for the time grain part to ensure that it works as expected.

gerbermichi avatar Jan 10 '25 08:01 gerbermichi

@villebro can you have a look at this PR?

gerbermichi avatar Apr 07 '25 11:04 gerbermichi

@rusackas Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

github-actions[bot] avatar Jul 09 '25 08:07 github-actions[bot]

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

github-actions[bot] avatar Jul 09 '25 08:07 github-actions[bot]

Some feedback from Claude:

Breaking Change Risk: The convertKeysToCamelCase(formData.extraFormData) could be a breaking change if:

Other components expect snake_case keys from extraFormData There are existing visualizations that rely on the current key format The conversion could cause naming conflicts

Format Override Behavior: When timeGrain is QUARTER, MONTH, or YEAR, the code completely ignores the user-specified format and uses a default formatter. This might surprise users who expect their custom format to be applied.

Undefined Format Handling: The code passes undefined as the format to getTimeFormatter for certain time grains, which needs verification that this is handled properly.

rusackas avatar Aug 12 '25 21:08 rusackas

Some feedback from Claude:

Breaking Change Risk: The convertKeysToCamelCase(formData.extraFormData) could be a breaking change if:

Other components expect snake_case keys from extraFormData There are existing visualizations that rely on the current key format The conversion could cause naming conflicts

Format Override Behavior: When timeGrain is QUARTER, MONTH, or YEAR, the code completely ignores the user-specified format and uses a default formatter. This might surprise users who expect their custom format to be applied.

Undefined Format Handling: The code passes undefined as the format to getTimeFormatter for certain time grains, which needs verification that this is handled properly.

I changed the code so that it does not need to convert the keys. Now I use formData.extraFormData.time_grain_sqla directly.

Could you have another look?

gerbermichi avatar Oct 29 '25 07:10 gerbermichi

CodeAnt AI is reviewing your PR.

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • [ ] Possible runtime error
    The new calls pass formData.extraFormData.time_grain_sqla directly. If formData.extraFormData is undefined at runtime, this will throw. Use safe optional chaining or ensure extraFormData exists before accessing its properties.

  • [ ] Format precedence
    The new timeGrain early-return in both tooltip and x-axis formatters takes precedence over any explicit format argument. If a caller specifies an explicit time format string, it will be ignored when timeGrain is QUARTER/MONTH/YEAR. Confirm whether timeGrain should override an explicit format or only apply when format is absent or equal to SMART_DATE_ID.

  • [ ] X axis behavior change
    getXAxisFormatter used to return undefined for !format so the charting lib could apply defaults. With the new timeGrain branch it now returns a concrete formatter for QUARTER/MONTH/YEAR even when format is absent, changing the default rendering behaviour. Ensure this is intentional.

  • [ ] SMART_DATE_VERBOSE usage
    getSmartDateVerboseFormatter now accepts a timeGrain and forwards it to getTimeFormatter. Verify that getTimeFormatter(SMART_DATE_VERBOSE_ID, timeGrain) produces the intended verbose smart-date output for all supported time grains and that @superset-ui/core supports that combination.

  • [ ] Timezone-dependent / flaky tests
    The new tests call the time formatter with hardcoded epoch milliseconds (e.g. 1610000000000) and assert exact formatted strings. Date/time formatting can be influenced by the environment timezone or locale, which can make these expectations flaky on CI or developer machines. Verify tests are deterministic across environments (use UTC-based timestamps, Date.UTC, or mock the timezone) and consider asserting patterns rather than exact strings where appropriate.

  • [ ] Timezone-dependent test
    The new tests assert formatted date strings using a raw epoch number (1610000000000). Formatting can vary by environment timezone/locale and lead to flaky CI tests. Use UTC-based timestamps or construct dates with Date.UTC / moment.utc to make expectations deterministic.

  • [ ] Fragile xAxis access
    The test assumes echartOptions.xAxis is an object with axisLabel.formatter. In some chart configs xAxis can be an array or undefined — this will throw during test runtime. Add a defensive guard or normalize the value before accessing formatter.

  • [ ] Duplication in test setup
    The tests for xAxis and tooltip formatters repeat very similar setup: constructing an updatedChartPropsConfig with modified formData and queriesData (colnames/coltypes). This duplication makes maintenance harder and increases the chance of inconsistent changes. Extract a small helper to build chart props for a given time grain/format to avoid divergence and reduce copy/paste.

CodeAnt AI finished reviewing your PR.