You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/07/08 00:46:37 UTC
(superset) branch master updated: fix(charts): fix time comparison crash when offsets share a numeric prefix (#39344)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 0efcd54250a fix(charts): fix time comparison crash when offsets share a numeric prefix (#39344)
0efcd54250a is described below
commit 0efcd54250ab116b5cef336c901b3521eb6ceea8
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Tue Jul 7 17:46:30 2026 -0700
fix(charts): fix time comparison crash when offsets share a numeric prefix (#39344)
Co-authored-by: Claude Opus 4.6 <no...@anthropic.com>
---
.../src/operators/renameOperator.ts | 2 +-
.../src/operators/utils/timeOffset.ts | 6 ++-
.../test/operators/renameOperator.test.ts | 24 ++++++++++
.../test/operators/utils/timeOffset.test.ts | 23 +++++++++
superset/models/helpers.py | 22 +++++++--
superset/utils/core.py | 4 +-
.../utils/pandas_postprocessing/contribution.py | 4 +-
tests/unit_tests/common/test_time_shifts.py | 54 ++++++++++++++++++++++
.../pandas_postprocessing/test_contribution.py | 33 +++++++++++++
tests/unit_tests/utils/test_core.py | 13 +++++-
10 files changed, 173 insertions(+), 12 deletions(-)
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts
index 86643e26822..5fbaf1723a5 100644
--- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts
+++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts
@@ -67,7 +67,7 @@ export const renameOperator: PostProcessingFactory<PostProcessingRename> = (
[...metricOffsetMap.entries()].forEach(
([metricWithOffset, metricOnly]) => {
const offsetLabel = timeOffsets.find(offset =>
- metricWithOffset.includes(offset),
+ metricWithOffset.endsWith(`${TIME_COMPARISON_SEPARATOR}${offset}`),
);
renamePairs.push([
formData.comparison_type === ComparisonType.Values
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts
index 9aae5445bc8..392c121e29c 100644
--- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts
+++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/timeOffset.ts
@@ -26,7 +26,7 @@ export const getTimeOffset = (
timeCompare.find(
timeOffset =>
// offset is represented as <offset>, group by list
- series.name.includes(`${timeOffset},`) ||
+ series.name.startsWith(`${timeOffset},`) ||
// offset is represented as <metric>__<offset>
series.name.includes(`__${timeOffset}`) ||
// offset is represented as <metric>, <offset>
@@ -50,7 +50,9 @@ export const getOriginalSeries = (
// offset in the middle: <metric>, <offset>, <dimension>
result = result.replace(`, ${compare},`, ',');
// offset at start: <offset>, <dimension>
- result = result.replace(`${compare},`, '');
+ if (result.startsWith(`${compare},`)) {
+ result = result.slice(`${compare},`.length);
+ }
// offset with double underscore: <metric>__<offset>
result = result.replace(`__${compare}`, '');
// offset at end: <metric>, <offset>
diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/renameOperator.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/renameOperator.test.ts
index 4ef3212c429..e07b7cf3c3f 100644
--- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/renameOperator.test.ts
+++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/renameOperator.test.ts
@@ -303,6 +303,30 @@ test('should add renameOperator if multiple metrics exist', () => {
});
});
+test('should correctly match offsets that share a numeric prefix', () => {
+ expect(
+ renameOperator(
+ {
+ ...formData,
+
+ comparison_type: ComparisonType.Values,
+ time_compare: ['1 year ago', '11 year ago'],
+ },
+ queryObject,
+ ),
+ ).toEqual({
+ operation: 'rename',
+ options: {
+ columns: {
+ 'count(*)__1 year ago': '1 year ago',
+ 'count(*)__11 year ago': '11 year ago',
+ },
+ inplace: true,
+ level: 0,
+ },
+ });
+});
+
test('should remove renameOperator', () => {
expect(
renameOperator(
diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts
index bec42af6c9b..fa6d92f5aca 100644
--- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts
+++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/timeOffset.test.ts
@@ -114,3 +114,26 @@ test('hasTimeOffset returns false when series name is not a string', () => {
const timeCompare = ['1 year ago'];
expect(hasTimeOffset(series, timeCompare)).toBe(false);
});
+
+test('getTimeOffset correctly matches offsets that share a numeric prefix', () => {
+ const timeCompare = ['1 year ago', '11 year ago'];
+ expect(
+ getTimeOffset({ name: '11 year ago, Alexander' }, timeCompare),
+ ).toEqual('11 year ago');
+ expect(getTimeOffset({ name: '1 year ago, Alexander' }, timeCompare)).toEqual(
+ '1 year ago',
+ );
+ expect(getTimeOffset({ name: 'Births__11 year ago' }, timeCompare)).toEqual(
+ '11 year ago',
+ );
+});
+
+test('getOriginalSeries correctly strips offsets that share a numeric prefix', () => {
+ const timeCompare = ['1 year ago', '11 year ago'];
+ expect(getOriginalSeries('11 year ago, Alexander', timeCompare)).toEqual(
+ 'Alexander',
+ );
+ expect(getOriginalSeries('1 year ago, Alexander', timeCompare)).toEqual(
+ 'Alexander',
+ );
+});
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 43f581b6055..a246f1f8bcc 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -2311,10 +2311,26 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
time_grain
)
- if join_column_producer and not time_grain:
- raise QueryObjectValidationError(
- _("Time Grain must be specified when using Time Shift.")
+ if not time_grain:
+ has_temporal_join_key = any(
+ pd.api.types.is_datetime64_any_dtype(df[key])
+ for key in join_keys
+ if key in df.columns
)
+ if has_temporal_join_key:
+ has_relative_offset = any(
+ not (
+ self.is_valid_date_range(offset)
+ and feature_flag_manager.is_feature_enabled(
+ "DATE_RANGE_TIMESHIFTS_ENABLED"
+ )
+ )
+ for offset in offset_dfs
+ )
+ if has_relative_offset:
+ raise QueryObjectValidationError(
+ _("Time Grain must be specified when using Time Comparison.")
+ )
for offset, offset_df in offset_dfs.items():
is_date_range_offset = self.is_valid_date_range(
diff --git a/superset/utils/core.py b/superset/utils/core.py
index e569a28b833..fe591e14253 100644
--- a/superset/utils/core.py
+++ b/superset/utils/core.py
@@ -1821,9 +1821,9 @@ def extract_dataframe_dtypes(
columns_by_name[column.column_name] = column
generic_types: list[GenericDataType] = []
- for column in df.columns:
+ for i, column in enumerate(df.columns):
column_object = columns_by_name.get(str(column))
- series = df[column]
+ series = df.iloc[:, i]
inferred_type: str = ""
if series.isna().all():
sql_type: Optional[str] = ""
diff --git a/superset/utils/pandas_postprocessing/contribution.py b/superset/utils/pandas_postprocessing/contribution.py
index ec6716fcba0..a2875c5d5b9 100644
--- a/superset/utils/pandas_postprocessing/contribution.py
+++ b/superset/utils/pandas_postprocessing/contribution.py
@@ -24,7 +24,7 @@ from flask_babel import gettext as _
from pandas import DataFrame, MultiIndex
from superset.exceptions import InvalidPostProcessingError
-from superset.utils.core import PostProcessingContributionOrientation
+from superset.utils.core import PostProcessingContributionOrientation, TIME_COMPARISON
from superset.utils.pandas_postprocessing.utils import validate_column_args
@@ -130,7 +130,7 @@ def get_column_groups(
time_shift = None
if time_shifts and isinstance(col_0, str):
for ts in time_shifts:
- if col_0.endswith(ts):
+ if col_0.endswith(TIME_COMPARISON + ts):
time_shift = ts
break
if time_shift is not None:
diff --git a/tests/unit_tests/common/test_time_shifts.py b/tests/unit_tests/common/test_time_shifts.py
index 9e9d4b20527..a34076393f2 100644
--- a/tests/unit_tests/common/test_time_shifts.py
+++ b/tests/unit_tests/common/test_time_shifts.py
@@ -14,6 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import pytest
from pandas import DataFrame, Series, Timestamp
from pandas.testing import assert_frame_equal
from pytest import fixture, mark # noqa: PT013
@@ -23,6 +24,7 @@ from superset.common.query_context import QueryContext
from superset.common.query_context_processor import QueryContextProcessor
from superset.connectors.sqla.models import BaseDatasource
from superset.constants import TimeGrain
+from superset.exceptions import QueryObjectValidationError
from superset.models.helpers import ExploreMixin
# Create processor and bind ExploreMixin methods to datasource
@@ -330,3 +332,55 @@ def test_join_offset_dfs_totals_query_no_dimensions():
)
assert_frame_equal(expected, result)
+
+
+def test_join_offset_dfs_raises_without_time_grain():
+ """Time comparison with relative offsets requires a time grain."""
+ df = DataFrame({"ds": [Timestamp("2021-01-01")], "D": [1]})
+ offset_df = DataFrame({"ds": [Timestamp("2021-02-01")], "B": [5]})
+ offset_dfs = {"1 year ago": offset_df}
+
+ with pytest.raises(
+ QueryObjectValidationError, match="Time Grain must be specified"
+ ):
+ query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["ds"]
+ )
+
+
+def test_join_offset_dfs_allows_non_temporal_join_without_time_grain():
+ """Time comparison without time grain is valid when join keys are non-temporal."""
+ df = DataFrame({"country": ["US", "UK"], "metric": [10, 20]})
+ offset_df = DataFrame({"country": ["US", "UK"], "metric__1 year ago": [8, 15]})
+ offset_dfs = {"1 year ago": offset_df}
+
+ result = query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["country"]
+ )
+ assert "metric__1 year ago" in result.columns
+
+
+def test_join_offset_dfs_raises_when_temporal_key_not_first():
+ """Temporal join key detection works even when it's not the first key."""
+ df = DataFrame(
+ {
+ "country": ["US", "UK"],
+ "ds": [Timestamp("2021-01-01"), Timestamp("2021-02-01")],
+ "D": [1, 2],
+ }
+ )
+ offset_df = DataFrame(
+ {
+ "country": ["US", "UK"],
+ "ds": [Timestamp("2021-03-01"), Timestamp("2021-04-01")],
+ "B": [5, 6],
+ }
+ )
+ offset_dfs = {"1 year ago": offset_df}
+
+ with pytest.raises(
+ QueryObjectValidationError, match="Time Grain must be specified"
+ ):
+ query_context_processor.join_offset_dfs(
+ df, offset_dfs, time_grain=None, join_keys=["country", "ds"]
+ )
diff --git a/tests/unit_tests/pandas_postprocessing/test_contribution.py b/tests/unit_tests/pandas_postprocessing/test_contribution.py
index 7853fc97f29..8d45e7c8f44 100644
--- a/tests/unit_tests/pandas_postprocessing/test_contribution.py
+++ b/tests/unit_tests/pandas_postprocessing/test_contribution.py
@@ -124,3 +124,36 @@ def test_contribution_with_time_shift_columns():
assert_array_equal(processed_df["a__1 week ago"].tolist(), [0.5, 0.5])
assert_array_equal(processed_df["b__1 week ago"].tolist(), [0.25, 0.25])
assert_array_equal(processed_df["c__1 week ago"].tolist(), [0.25, 0.25])
+
+
+def test_contribution_with_numeric_prefix_time_shifts():
+ """Time shifts like '2 weeks ago' and '22 weeks ago' share a numeric suffix;
+ columns must be grouped by their exact offset, not by suffix matching."""
+ df = DataFrame(
+ {
+ DTTM_ALIAS: [
+ datetime(2020, 7, 16, 14, 49),
+ datetime(2020, 7, 16, 14, 50),
+ ],
+ "a": [3, 6],
+ "b": [6, 3],
+ "a__2 weeks ago": [1, 1],
+ "b__2 weeks ago": [1, 1],
+ "a__22 weeks ago": [2, 4],
+ "b__22 weeks ago": [4, 2],
+ }
+ )
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.ROW,
+ time_shifts=["2 weeks ago", "22 weeks ago"],
+ )
+ # Non-time-shift columns: a=3,b=6 -> a=1/3, b=2/3; a=6,b=3 -> a=2/3, b=1/3
+ assert_array_equal(processed_df["a"].tolist(), [1 / 3, 2 / 3])
+ assert_array_equal(processed_df["b"].tolist(), [2 / 3, 1 / 3])
+ # "2 weeks ago" group: a=1,b=1 -> 0.5,0.5 each row
+ assert_array_equal(processed_df["a__2 weeks ago"].tolist(), [0.5, 0.5])
+ assert_array_equal(processed_df["b__2 weeks ago"].tolist(), [0.5, 0.5])
+ # "22 weeks ago" group: a=2,b=4 -> 1/3,2/3; a=4,b=2 -> 2/3,1/3
+ assert_array_equal(processed_df["a__22 weeks ago"].tolist(), [1 / 3, 2 / 3])
+ assert_array_equal(processed_df["b__22 weeks ago"].tolist(), [2 / 3, 1 / 3])
diff --git a/tests/unit_tests/utils/test_core.py b/tests/unit_tests/utils/test_core.py
index 85394ac9703..5285ffb998b 100644
--- a/tests/unit_tests/utils/test_core.py
+++ b/tests/unit_tests/utils/test_core.py
@@ -31,6 +31,7 @@ from superset.utils.core import (
cast_to_boolean,
check_is_safe_zip,
DateColumn,
+ extract_dataframe_dtypes,
FilterOperator,
generic_find_constraint_name,
generic_find_fk_constraint_name,
@@ -693,8 +694,9 @@ def test_get_user_agent(mocker: MockerFixture, app_context: None) -> None:
@with_config(
{
- "USER_AGENT_FUNC": lambda database,
- source: f"{database.database_name} {source.name}"
+ "USER_AGENT_FUNC": lambda database, source: (
+ f"{database.database_name} {source.name}"
+ )
}
)
def test_get_user_agent_custom(mocker: MockerFixture, app_context: None) -> None:
@@ -1847,3 +1849,10 @@ def test_sanitize_cookie_token_accepts_valid(token: str) -> None:
)
def test_sanitize_cookie_token_rejects_invalid(token: Optional[str]) -> None:
assert sanitize_cookie_token(token) is None
+
+
+def test_extract_dataframe_dtypes_with_duplicate_columns() -> None:
+ """extract_dataframe_dtypes should not crash on duplicate column names."""
+ df = pd.DataFrame([[1, 2, 3]], columns=["a", "b", "a"])
+ result = extract_dataframe_dtypes(df)
+ assert len(result) == 3