-
Notifications
You must be signed in to change notification settings - Fork 15.2k
fix(Timeseries): Temporal x axis defensive check for missing data #33723
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
base: master
Are you sure you want to change the base?
Conversation
@msyavuz Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Direct console usage instead of logging framework ▹ view | 🧠 Not in standard |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
console.warn( | ||
`X-axis data type was missing from the backend response. ` + | ||
`Successfully inferred a 'temporal' type from the data.`, | ||
); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@msyavuz Ephemeral environment spinning up at http://34.212.29.103:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
if ( | ||
firstValue !== null && | ||
!Number.isNaN(new Date(firstValue as string | number).getTime()) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check does require us to manually check if the first value is a data type or not from the x Axis.
But the datasource has a list of all time columns under granularitySqla
, so we can bring a boolean from transformProps.ts
— basically isXAxisTemporal
.
let timeColumnsSet;
let isXAxisTemporal = false;
if (Array.isArray(datasource.granularitySqla) && datasource.granularitySqla.length > 0) {
timeColumnsSet = new Set(datasource.granularitySqla.flat());
}
if (timeColumnsSet && timeColumnsSet.has(formData?.xAxis)) {
isXAxisTemporal = true;
}
The code will be as above, and we can just use the boolean like this:
if (xAxisDataType === undefined && isXAxisTemporal) {
xAxisDataType = GenericDataType.Temporal;
}
I’ve tried this in the past — this approach works without fail, since datasource?.granularitySqla
will always have relevant temporal columns.

Attatched screenshot shows datasource & its time columns under granularitysqla
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an elegant solution! But since i can't reproduce the problem, i went with the simpler one that should always exist. I can do a refactor to something similar to yours after we confirm this works.
SUMMARY
There is an intermittent issue with temporal x-axis that causes charts to display NaN or epoch time instead of correct formats. There is a possible diversion from the happy path where coltype got from backend is corrupted that can cause this. This pr aims to fix that by having defensive checks in frontend to display correct formats.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION