You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gerbermichi (via GitHub)" <gi...@apache.org> on 2025/01/09 10:32:09 UTC
[PR] feat(Timeseries): Respect time grain and add i18n support for ECharts [superset]
gerbermichi opened a new pull request, #31765:
URL: https://github.com/apache/superset/pull/31765
### SUMMARY
Respect time grain and add i18n support for ECharts
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### BEFORE
<img width="2394" alt="image" src="https://github.com/user-attachments/assets/31f89bdd-cfd9-48c3-8841-5d24946e4a65" />
#### AFTER
<img width="3018" alt="image" src="https://github.com/user-attachments/assets/580d8320-62d3-4627-8640-95c1643dc366" />
### TESTING INSTRUCTIONS
Create a Timeseries or MixedTimeseries chart and use the time grain Quarter.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3051647493
@rusackas Ephemeral environment spinning up at http://35.92.241.166:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-4782729872
Thanks @gerbermichi, and enjoy the vacation! I've carried this forward in #41350 (kept you as co-author). Focused it on the tooltip grain since the x-axis part already landed via #38017, and tidied up the formatter and tests along the way. Going to close this in favor of that one, but holler if you'd rather pick it back up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "gerbermichi (via GitHub)" <gi...@apache.org>.
gerbermichi commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-2582067211
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #31765:
URL: https://github.com/apache/superset/pull/31765#discussion_r2224926766
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:
##########
@@ -76,24 +77,40 @@ export const getYAxisFormatter = (
export function getTooltipTimeFormatter(
format?: string,
+ timeGrain?: TimeGranularity,
): TimeFormatter | StringConstructor {
+ if (
+ timeGrain === TimeGranularity.QUARTER ||
+ timeGrain === TimeGranularity.MONTH ||
+ timeGrain === TimeGranularity.YEAR
+ ) {
+ return getTimeFormatter(undefined, timeGrain);
+ }
if (format === SMART_DATE_ID) {
- return getSmartDateVerboseFormatter();
+ return getTimeFormatter(SMART_DATE_VERBOSE_ID, timeGrain);
Review Comment:
With this change `getSmartDateVerboseFormatter` function is unused. Can we modify that function instead to pass the `timeGrain`.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -214,7 +215,11 @@ export default function transformProps(
percentageThreshold,
metrics = [],
metricsB = [],
- }: EchartsMixedTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData };
+ }: EchartsMixedTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ ...formData,
+ ...convertKeysToCamelCase(formData.extraFormData),
Review Comment:
Is this change with convertKeysToCamelCase necessary. What do we need and use from extraFormData?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #31765:
URL: https://github.com/apache/superset/pull/31765#discussion_r2194368577
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -633,4 +636,133 @@ describe('Does transformProps transform series correctly', () => {
'foo2, bar2': ['foo2', 'bar2'],
});
});
+
+ describe('should add the correct time formatter for the xAxis', () => {
+ const getXAxisFormatter = (timeGrainSqla: TimeGranularity) => {
+ const updatedChartPropsConfig = {
+ ...chartPropsConfig,
+ formData: {
+ ...chartPropsConfig.formData,
+ time_grain_sqla: timeGrainSqla,
+ xAxisTimeFormat: '%Y-%m-%d',
+ },
+ queriesData: [
+ {
+ data: chartPropsConfig.queriesData[0].data,
+ colnames: ['San Francisco', 'New York', '__timestamp'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ ],
+ };
+ const { echartOptions } = transformProps(
+ new ChartProps(updatedChartPropsConfig) as EchartsTimeseriesChartProps,
+ );
+ const xAxis = echartOptions.xAxis as {
+ axisLabel: { formatter: TimeFormatter };
+ };
+ return xAxis.axisLabel.formatter;
+ };
+
+ it('should work for days', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.DAY);
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07'),
+ );
+ });
+
+ it('should work for weeks', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.WEEK);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07 — 2021-01-13'),
+ );
+ });
+
+ it('should work for months', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.MONTH);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Jan'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for quarters', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Q1'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for years', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringMatching('2021'));
+ });
+ });
+
+ describe('should add the correct time formatter for the tooltip', () => {
+ const getTooltipFormatter = (timeGrainSqla: TimeGranularity) => {
+ const updatedChartPropsConfig = {
+ ...chartPropsConfig,
+ formData: {
+ ...chartPropsConfig.formData,
+ time_grain_sqla: timeGrainSqla,
+ tooltipTimeFormat: '%Y-%m-%d',
+ },
+ queriesData: [
+ {
+ data: chartPropsConfig.queriesData[0].data,
+ colnames: ['San Francisco', 'New York', '__timestamp'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ ],
+ };
+
+ return transformProps(
+ new ChartProps(updatedChartPropsConfig) as EchartsTimeseriesChartProps,
+ ).xValueFormatter;
+ };
+
+ it('should work for days', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.DAY);
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07'),
+ );
+ });
+
+ it('should work for weeks', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.WEEK);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07 — 2021-01-13'),
+ );
+ });
+
+ it('should work for months', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.MONTH);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Jan'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for quarters', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Q1'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for years', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.QUARTER);
Review Comment:
In the 'should work for years' tooltip test, you’re passing TimeGranularity.QUARTER. It should be TimeGranularity.YEAR to test the year formatter.
```suggestion
const formatter = getTooltipFormatter(TimeGranularity.YEAR);
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -162,3 +169,154 @@ it('should transform chart props for viz', () => {
}),
);
});
+
+describe('should add the correct time formatter for the xAxis', () => {
+ const getXAxisFormatter = (timeGrainSqla: TimeGranularity) => {
+ const updatedChartPropsConfig = {
+ ...chartPropsConfig,
+ formData: {
+ ...chartPropsConfig.formData,
+ time_grain_sqla: timeGrainSqla,
+ xAxisTimeFormat: '%Y-%m-%d',
+ },
+ queriesData: [
+ {
+ data: chartPropsConfig.queriesData[0].data,
+ label_map: chartPropsConfig.queriesData[0].label_map,
+ colnames: ['Boy', 'Girl', 'ds'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ {
+ data: chartPropsConfig.queriesData[1].data,
+ label_map: chartPropsConfig.queriesData[1].label_map,
+ colnames: ['Boy', 'Girl', 'ds'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ ],
+ };
+ const { echartOptions } = transformProps(
+ new ChartProps(updatedChartPropsConfig) as EchartsMixedTimeseriesProps,
+ );
+ const xAxis = echartOptions.xAxis as {
+ axisLabel: { formatter: TimeFormatter };
+ };
+ return xAxis.axisLabel.formatter;
+ };
+
+ it('should work for days', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.DAY);
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07'),
+ );
+ });
+
+ it('should work for weeks', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.WEEK);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07 — 2021-01-13'),
+ );
+ });
+
+ it('should work for months', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.MONTH);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Jan'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for quarters', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Q1'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for years', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
Review Comment:
In the 'should work for years' xAxis test, you’re using TimeGranularity.QUARTER. Update to TimeGranularity.YEAR to cover the year case.
```suggestion
const formatter = getXAxisFormatter(TimeGranularity.YEAR);
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -162,3 +169,154 @@ it('should transform chart props for viz', () => {
}),
);
});
+
+describe('should add the correct time formatter for the xAxis', () => {
+ const getXAxisFormatter = (timeGrainSqla: TimeGranularity) => {
+ const updatedChartPropsConfig = {
+ ...chartPropsConfig,
+ formData: {
+ ...chartPropsConfig.formData,
+ time_grain_sqla: timeGrainSqla,
+ xAxisTimeFormat: '%Y-%m-%d',
+ },
+ queriesData: [
+ {
+ data: chartPropsConfig.queriesData[0].data,
+ label_map: chartPropsConfig.queriesData[0].label_map,
+ colnames: ['Boy', 'Girl', 'ds'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ {
+ data: chartPropsConfig.queriesData[1].data,
+ label_map: chartPropsConfig.queriesData[1].label_map,
+ colnames: ['Boy', 'Girl', 'ds'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ ],
+ };
+ const { echartOptions } = transformProps(
+ new ChartProps(updatedChartPropsConfig) as EchartsMixedTimeseriesProps,
+ );
+ const xAxis = echartOptions.xAxis as {
+ axisLabel: { formatter: TimeFormatter };
+ };
+ return xAxis.axisLabel.formatter;
+ };
+
+ it('should work for days', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.DAY);
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07'),
+ );
+ });
+
+ it('should work for weeks', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.WEEK);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07 — 2021-01-13'),
+ );
+ });
+
+ it('should work for months', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.MONTH);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Jan'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for quarters', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Q1'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for years', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringMatching('2021'));
+ });
+});
+
+describe('should add the correct time formatter for the tooltip', () => {
+ const getTooltipFormatter = (timeGrainSqla: TimeGranularity) => {
+ const updatedChartPropsConfig = {
+ ...chartPropsConfig,
+ formData: {
+ ...chartPropsConfig.formData,
+ time_grain_sqla: timeGrainSqla,
+ tooltipTimeFormat: '%Y-%m-%d',
+ },
+ queriesData: [
+ {
+ data: chartPropsConfig.queriesData[0].data,
+ label_map: chartPropsConfig.queriesData[0].label_map,
+ colnames: ['Boy', 'Girl', 'ds'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ {
+ data: chartPropsConfig.queriesData[1].data,
+ label_map: chartPropsConfig.queriesData[1].label_map,
+ colnames: ['Boy', 'Girl', 'ds'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ ],
+ };
+
+ return transformProps(
+ new ChartProps(updatedChartPropsConfig) as EchartsMixedTimeseriesProps,
+ ).xValueFormatter;
+ };
+
+ it('should work for days', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.DAY);
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07'),
+ );
+ });
+
+ it('should work for weeks', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.WEEK);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07 — 2021-01-13'),
+ );
+ });
+
+ it('should work for months', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.MONTH);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Jan'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for quarters', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Q1'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for years', () => {
+ const formatter = getTooltipFormatter(TimeGranularity.QUARTER);
Review Comment:
In the 'should work for years' tooltip test, you’re passing TimeGranularity.QUARTER instead of TimeGranularity.YEAR.
```suggestion
const formatter = getTooltipFormatter(TimeGranularity.YEAR);
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -633,4 +636,133 @@ describe('Does transformProps transform series correctly', () => {
'foo2, bar2': ['foo2', 'bar2'],
});
});
+
+ describe('should add the correct time formatter for the xAxis', () => {
+ const getXAxisFormatter = (timeGrainSqla: TimeGranularity) => {
+ const updatedChartPropsConfig = {
+ ...chartPropsConfig,
+ formData: {
+ ...chartPropsConfig.formData,
+ time_grain_sqla: timeGrainSqla,
+ xAxisTimeFormat: '%Y-%m-%d',
+ },
+ queriesData: [
+ {
+ data: chartPropsConfig.queriesData[0].data,
+ colnames: ['San Francisco', 'New York', '__timestamp'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ ],
+ };
+ const { echartOptions } = transformProps(
+ new ChartProps(updatedChartPropsConfig) as EchartsTimeseriesChartProps,
+ );
+ const xAxis = echartOptions.xAxis as {
+ axisLabel: { formatter: TimeFormatter };
+ };
+ return xAxis.axisLabel.formatter;
+ };
+
+ it('should work for days', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.DAY);
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07'),
+ );
+ });
+
+ it('should work for weeks', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.WEEK);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(
+ expect.stringMatching('2021-01-07 — 2021-01-13'),
+ );
+ });
+
+ it('should work for months', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.MONTH);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Jan'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for quarters', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
+ expect(formatter).toBeDefined();
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('Q1'));
+ expect(formatter(1610000000000)).toEqual(expect.stringContaining('2021'));
+ });
+
+ it('should work for years', () => {
+ const formatter = getXAxisFormatter(TimeGranularity.QUARTER);
Review Comment:
In the 'should work for years' xAxis test, you’re passing TimeGranularity.QUARTER. It should be TimeGranularity.YEAR to verify the year formatter.
```suggestion
const formatter = getXAxisFormatter(TimeGranularity.YEAR);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #31765: feat(Timeseries): Respect time grain
URL: https://github.com/apache/superset/pull/31765
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-4782383508
@gerbermichi thanks for the note, and no worries at all. The time-grain formatting is worth keeping, so I'll carry it forward on a fresh PR, splitting it off from the i18n/breaking bits. Holler if you're back and want to pick it back up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3625674517
CodeAnt AI finished reviewing your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain and add i18n support for ECharts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31765:
URL: https://github.com/apache/superset/pull/31765#discussion_r1908521611
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -126,7 +126,163 @@ function Echart(
useEffect(() => {
if (!divRef.current) return;
if (!chartRef.current) {
- chartRef.current = init(divRef.current);
+ chartRef.current = init(divRef.current, null, {
+ locale: {
+ time: {
+ month: [
+ t('January'),
+ t('February'),
+ t('March'),
+ t('April'),
+ t('May'),
+ t('June'),
+ t('July'),
+ t('August'),
+ t('September'),
+ t('October'),
+ t('November'),
+ t('December'),
+ ],
+ monthAbbr: [
+ t('Jan'),
+ t('Feb'),
+ t('Mar'),
+ t('Apr'),
+ t('May'),
+ t('Jun'),
+ t('Jul'),
+ t('Aug'),
+ t('Sep'),
+ t('Oct'),
+ t('Nov'),
+ t('Dec'),
+ ],
+ dayOfWeek: [
+ t('Sunday'),
+ t('Monday'),
+ t('Tuesday'),
+ t('Wednesday'),
+ t('Thursday'),
+ t('Friday'),
+ t('Saturday'),
+ ],
+ dayOfWeekAbbr: [
+ t('Sun'),
+ t('Mon'),
+ t('Tue'),
+ t('Wed'),
+ t('Thu'),
+ t('Fri'),
+ t('Sat'),
+ ],
+ },
+ legend: {
+ selector: {
+ all: t('All'),
+ inverse: t('Inv'),
+ },
+ },
+ toolbox: {
+ brush: {
+ title: {
+ rect: t('Box Select'),
+ polygon: t('Lasso Select'),
+ lineX: t('Horizontally Select'),
+ lineY: t('Vertically Select'),
+ keep: t('Keep Selections'),
+ clear: t('Clear Selections'),
+ },
+ },
+ dataView: {
+ title: t('Data View'),
+ lang: [t('Data View'), t('Close'), 'Refresh'],
+ },
Review Comment:
### Missing Translation Wrapper <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'Refresh' string in the dataView.lang array is not wrapped in a translation function t(), unlike other UI strings.
###### Why this matters
This will cause inconsistency in localization where the 'Refresh' text will always appear in English regardless of the user's selected language.
###### Suggested change
Wrap 'Refresh' string with the translation function t():
```typescript
dataView: {
title: t('Data View'),
lang: [t('Data View'), t('Close'), t('Refresh')],
},
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:3c13b7a7-c254-422c-adcf-cfcba8539c0a -->
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:
##########
@@ -76,24 +77,40 @@ export const getYAxisFormatter = (
export function getTooltipTimeFormatter(
format?: string,
+ timeGrain?: TimeGranularity,
): TimeFormatter | StringConstructor {
+ if (
+ timeGrain === TimeGranularity.QUARTER ||
+ timeGrain === TimeGranularity.MONTH ||
+ timeGrain === TimeGranularity.YEAR
+ ) {
+ return getTimeFormatter(undefined, timeGrain);
+ }
if (format === SMART_DATE_ID) {
- return getSmartDateDetailedFormatter();
+ return getTimeFormatter(SMART_DATE_DETAILED_ID, timeGrain);
}
Review Comment:
### Incorrect Override of Smart Date Format <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
When SMART_DATE_ID format is specified, the code overrides it with SMART_DATE_DETAILED_ID, which contradicts the user's explicit format choice.
###### Why this matters
Users selecting SMART_DATE_ID format will unexpectedly receive a different formatting style than requested, potentially causing confusion and inconsistency in the application's behavior.
###### Suggested change
Respect the user's format choice by modifying the code to:
```typescript
if (format === SMART_DATE_ID) {
return getTimeFormatter(SMART_DATE_ID, timeGrain);
}
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:fc3c285b-d1ab-4f73-b7c2-9b2121abb449 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3727999411
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3625645242
CodeAnt AI is reviewing your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain and add i18n support for ECharts [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-2581011022
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3727772426
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 8ecc70c0d38219184b54b85b133c50a552e46f96 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6960b7f43e47ce0008eb4fe9 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-31765--superset-docs-preview.netlify.app](https://deploy-preview-31765--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-31765--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3625667411
## Nitpicks 🔍
<table>
<tr><td>🔒 <strong>No security issues identified</strong></td></tr>
<tr><td>⚡ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-88cfb8074eec39fa78997341a017c9b8251c54d19a2ab8e188b9e8bcfad7e00bR481-R496'><strong>Possible runtime error</strong></a><br>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.<br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-210e76eb52ab194e19d0ef3bc8485c5dd0fda48a6af1c72a4376c5e448dfeb8fR78-R96'><strong>Format precedence</strong></a><br>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.<br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-210e76eb52ab194e19d0ef3bc8485c5dd0fda48a6af1c72a4376c5e448dfeb8fR98-R116'><strong>X axis behavior change</strong></a><br>`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.<br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-210e76eb52ab194e19d0ef3bc8485c5dd0fda48a6af1c72a4376c5e448dfeb8fR41-R42'><strong>SMART_DATE_VERBOSE usage</strong></a><br>`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.<br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-2ada607a5c3e8ab7bee07307fefb6ab7236948f5bed812126793074bdc4d89c6R332-R481'><strong>Timezone-dependent / flaky tests</strong></a><br>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.<br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-788715ca89a2182b9d9f2edec52fbb5f3e8e5cb70c49bf166992193a81eb87f6R631-R758'><strong>Timezone-dependent test</strong></a><br>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.<br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-788715ca89a2182b9d9f2edec52fbb5f3e8e5cb70c49bf166992193a81eb87f6R652-R659'><strong>Fragile xAxis access</strong></a><br>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.<br>
- [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-2ada607a5c3e8ab7bee07307fefb6ab7236948f5bed812126793074bdc4d89c6R332-R481'><strong>Duplication in test setup</strong></a><br>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.<br>
</td></tr>
</table>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-4665740548
Thanks @gerbermichi — this has been open a while obviously, with a bunch of conflicts and unresolved bot suggestions. Since it's tagged risk:breaking-change, it'd also help to spell out the user-facing behavior change and whether it needs an UPDATING.md entry so reviewers can weigh it. Let us know if you're still able to pick it back up, but I'll mark it as draft for now while it awaits work. Holler if I can help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "gerbermichi (via GitHub)" <gi...@apache.org>.
gerbermichi commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-2783010539
@villebro can you have a look at this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3051624699
@rusackas Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/16163980410). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #31765:
URL: https://github.com/apache/superset/pull/31765#discussion_r2597583372
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -543,11 +543,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
+ )
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getXAxisFormatter(xAxisTimeFormat)
+ ? getXAxisFormatter(
+ xAxisTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** Runtime error / wrong source: the same unsafe access to `formData.extraFormData.time_grain_sqla` is used for the x-axis formatter; it may throw if `extraFormData` is absent and should read from `chartProps.rawFormData` to respect time-grain overrides — switch to `chartProps.rawFormData?.extraFormData?.time_grain_sqla`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ?? timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same issue as the tooltip formatter: accessing formData.extraFormData can throw if extraFormData is undefined and won't pick up overrides from rawFormData. Switching to chartProps.rawFormData?.extraFormData?.time_grain_sqla both prevents the TypeError and respects the intended override behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 555:555
**Comment:**
*Possible Bug: Runtime error / wrong source: the same unsafe access to `formData.extraFormData.time_grain_sqla` is used for the x-axis formatter; it may throw if `extraFormData` is absent and should read from `chartProps.rawFormData` to respect time-grain overrides — switch to `chartProps.rawFormData?.extraFormData?.time_grain_sqla`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -481,11 +480,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** Accessing `formData.extraFormData.time_grain_sqla` can throw if `extraFormData` is undefined (property access on undefined). Also `time_grain_sqla` belongs to the raw form data; use the raw form data and optional chaining to safely read it and fall back to the existing `timeGrainSqla` variable. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ?? timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing code does access formData.extraFormData without any optional chaining, which will throw if extraFormData is undefined at runtime. Using optional chaining (or accessing rawFormData which always exists on chartProps) is a safe, minimal fix that prevents a real runtime exception. This is a functional correctness bug, not a stylistic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 485:485
**Comment:**
*Possible Bug: Accessing `formData.extraFormData.time_grain_sqla` can throw if `extraFormData` is undefined (property access on undefined). Also `time_grain_sqla` belongs to the raw form data; use the raw form data and optional chaining to safely read it and fall back to the existing `timeGrainSqla` variable.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -543,11 +543,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** Runtime error / wrong source: accessing `formData.extraFormData.time_grain_sqla` can throw when `extraFormData` is undefined and also `extraFormData` lives on `rawFormData` (not `formData`) — use `chartProps.rawFormData?.extraFormData?.time_grain_sqla` to both avoid a TypeError and read the correct override. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ?? timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid bugfix: formData may not contain extraFormData and the canonical place for user overrides is rawFormData. Using chartProps.rawFormData?.extraFormData?.time_grain_sqla avoids a potential TypeError and ensures we respect the original override. The suggested change fixes a real runtime-null access and corrects the data source.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 548:548
**Comment:**
*Null Pointer: Runtime error / wrong source: accessing `formData.extraFormData.time_grain_sqla` can throw when `extraFormData` is undefined and also `extraFormData` lives on `rawFormData` (not `formData`) — use `chartProps.rawFormData?.extraFormData?.time_grain_sqla` to both avoid a TypeError and read the correct override.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -16,8 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ChartProps, VizType } from '@superset-ui/core';
import { supersetTheme } from '@apache-superset/core/ui';
+import {
+ ChartProps,
+ TimeFormatter,
+ TimeGranularity,
+ VizType,
+} from '@superset-ui/core';
Review Comment:
**Suggestion:** `TimeFormatter` is a type, not a runtime value; importing it alongside runtime exports can lead to unnecessary runtime bindings or bundler issues—import it using `import type` (or a separate type-only import) to avoid emitting a value-level import. [type error]
**Severity Level:** Minor ⚠️
```suggestion
import { ChartProps, TimeGranularity, VizType } from '@superset-ui/core';
import type { TimeFormatter } from '@superset-ui/core';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, useful suggestion: `TimeFormatter` is only used as a type in the test and should be imported with `import type` to avoid accidentally emitting a runtime import (especially important with Babel's TS handling). The improved code fixes a real nicety and is low risk.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 20:25
**Comment:**
*Type Error: `TimeFormatter` is a type, not a runtime value; importing it alongside runtime exports can lead to unnecessary runtime bindings or bundler issues—import it using `import type` (or a separate type-only import) to avoid emitting a value-level import.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -481,11 +480,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
+ )
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getXAxisFormatter(xAxisTimeFormat)
+ ? getXAxisFormatter(
+ xAxisTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** The code reads `formData.extraFormData.time_grain_sqla` which may not exist on `formData`; this will throw. Read the time grain from the chart's raw form data with optional chaining so the expression never throws and still falls back to `timeGrainSqla`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ?? timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same issue as the tooltip formatter: accessing formData.extraFormData.time_grain_sqla without optional chaining can throw if extraFormData is undefined. Switching to safe access (e.g., chartProps.rawFormData?.extraFormData?.time_grain_sqla or formData.extraFormData?.time_grain_sqla) prevents a real crash and is appropriate here.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 492:492
**Comment:**
*Possible Bug: The code reads `formData.extraFormData.time_grain_sqla` which may not exist on `formData`; this will throw. Read the time grain from the chart's raw form data with optional chaining so the expression never throws and still falls back to `timeGrainSqla`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:
##########
@@ -76,24 +77,40 @@ export const getYAxisFormatter = (
export function getTooltipTimeFormatter(
format?: string,
+ timeGrain?: TimeGranularity,
): TimeFormatter | StringConstructor {
+ if (
+ timeGrain === TimeGranularity.QUARTER ||
+ timeGrain === TimeGranularity.MONTH ||
+ timeGrain === TimeGranularity.YEAR
+ ) {
+ return getTimeFormatter(undefined, timeGrain);
+ }
if (format === SMART_DATE_ID) {
- return getSmartDateVerboseFormatter();
+ return getSmartDateVerboseFormatter(timeGrain);
Review Comment:
**Suggestion:** Logical ordering bug in `getTooltipTimeFormatter`: the new early time-grain check runs before the `SMART_DATE_ID` check, so when callers request `SMART_DATE_ID` with a QUARTER/MONTH/YEAR time grain the function will return the generic time-grain formatter instead of the verbose smart-date formatter. Reorder checks to honor explicit `SMART_DATE_ID` requests first. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
// If caller explicitly requested SMART_DATE_ID, respect that first
if (format === SMART_DATE_ID) {
return getSmartDateVerboseFormatter(timeGrain);
}
// Otherwise, if a coarse time grain is specified prefer the grain-specific formatter
if (
timeGrain === TimeGranularity.QUARTER ||
timeGrain === TimeGranularity.MONTH ||
timeGrain === TimeGranularity.YEAR
) {
return getTimeFormatter(undefined, timeGrain);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a legitimate logic bug: an explicit request for SMART_DATE_ID should be honored even when a coarse timeGrain is supplied. Reordering the checks so SMART_DATE_ID is handled first preserves the caller's explicit intent. The proposed change is minimal and local and fixes observable incorrect behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
**Line:** 82:90
**Comment:**
*Logic Error: Logical ordering bug in `getTooltipTimeFormatter`: the new early time-grain check runs before the `SMART_DATE_ID` check, so when callers request `SMART_DATE_ID` with a QUARTER/MONTH/YEAR time grain the function will return the generic time-grain formatter instead of the verbose smart-date formatter. Reorder checks to honor explicit `SMART_DATE_ID` requests first.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "gerbermichi (via GitHub)" <gi...@apache.org>.
gerbermichi commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3460152764
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "gerbermichi (via GitHub)" <gi...@apache.org>.
gerbermichi commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-4781465938
Hi @rusackas, apologies for going quiet – I’m currently on vacation and won’t be able to carry this forward in the near term. Feel free to take the time-grain formatting onto a fresh PR, that’s totally fine by me. Splitting it out from the i18n change and adding the UPDATING.md note per @villebro makes sense. Thanks for picking it up!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain and add i18n support for ECharts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-2580986227
You need to run the pre-commit hook to pass CI.
Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). 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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3181061821
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "gerbermichi (via GitHub)" <gi...@apache.org>.
gerbermichi commented on code in PR #31765:
URL: https://github.com/apache/superset/pull/31765#discussion_r2238779964
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -214,7 +215,11 @@ export default function transformProps(
percentageThreshold,
metrics = [],
metricsB = [],
- }: EchartsMixedTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData };
+ }: EchartsMixedTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ ...formData,
+ ...convertKeysToCamelCase(formData.extraFormData),
Review Comment:
I need the actual `time_grain_sqla` from the filter in the dashboard. The `timeGrainSqla` from the `formData` has the value set when configuring the chart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-3727657535
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Timeseries): Respect time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31765:
URL: https://github.com/apache/superset/pull/31765#issuecomment-4781127736
@gerbermichi the time-grain formatting is a nice fix, but it's bundled with an i18n change and tagged breaking, so per villebro it needs splitting out and an UPDATING.md note. You've been quiet, so I may pick the formatting half up on a fresh PR, let me know if you'd rather carry it forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org