sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4253021020
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -2144,6 +2172,190 @@ def
test_pivot_table_v2_actual_values_uses_database_rollups():
assert pivoted.loc[("US",), ("AVG(num)", "boy")] == 10
[email protected](
+ "aggregate_function,expected",
+ [
+ ("Sample Variance", 50.0),
+ ("Sample Standard Deviation", 50.0**0.5),
+ ],
+)
+def test_pivot_table_v2_sample_statistics_aggregations(
+ aggregate_function: str, expected: float
+):
+ """Sample variance/standard deviation must compute for two values at one
+ pivot address (10 and 20 give variance 50 and standard deviation ~7.071),
+ not raise on a misspelled ``pd.series`` or a non-callable tuple.
+ """
+ df = pd.DataFrame(
+ {
+ "nation": ["US", "US"],
+ "gender": ["boy", "boy"],
+ "SUM(num)": [10, 20],
+ }
+ )
+ form_data = {
+ "groupbyRows": ["nation"],
+ "groupbyColumns": ["gender"],
+ "metrics": ["SUM(num)"],
+ "aggregateFunction": aggregate_function,
+ }
+
+ pivoted = pivot_table_v2(df, form_data, apply_number_format=False)
+
+ assert pivoted.loc[("US",), ("SUM(num)", "boy")] == pytest.approx(expected)
+
+
[email protected](
+ "aggregate_function",
+ ["Sample Variance", "Sample Standard Deviation"],
+)
+def test_pivot_table_v2_sample_statistics_with_summaries(aggregate_function:
str):
+ """Row and column summaries invoke the reducer with ``axis``; the sample
+ statistics must handle the DataFrame/axis form, not just a Series.
+ """
+ df = pd.DataFrame(
+ {
+ "nation": ["US", "US", "FR", "FR"],
+ "gender": ["boy", "girl", "boy", "girl"],
+ "SUM(num)": [10, 20, 30, 50],
+ }
+ )
+ form_data = {
+ "groupbyRows": ["nation"],
+ "groupbyColumns": ["gender"],
+ "metrics": ["SUM(num)"],
+ "aggregateFunction": aggregate_function,
+ "rowTotals": True,
+ "colTotals": True,
+ }
+
+ pivoted = pivot_table_v2(df, form_data, apply_number_format=False)
+
+ assert not pivoted.empty
Review Comment:
This only asserts the pivot is non-empty, and each (nation, gender) cell
holds a single observation, so the leaves reduce to 0 before the summary pass.
A DataFrame/`axis` reducer that returned 0 for every summary would still pass,
so the new sample variance/standard deviation path isn't actually pinned. Could
this assert concrete summary values, e.g. leaves 10/20/30/50 giving row
variances 50 and 200 and column variances 200 and 450 (square roots for the
standard deviation)?
##########
superset-frontend/src/explore/components/LegacyAggregationAlert.tsx:
##########
@@ -0,0 +1,111 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useState, useCallback } from 'react';
+import { useSelector } from 'react-redux';
+import { t } from '@apache-superset/core/translation';
+import { logging } from '@apache-superset/core/utils';
+import { css } from '@apache-superset/core/theme';
+import { TagType } from 'src/components';
+import { fetchTags, deleteTaggedObjects } from 'src/features/tags/tags';
+import { LEGACY_AGGREGATION_TAG } from 'src/explore/constants';
+import { ExploreAlert } from './ExploreAlert';
+
+interface LegacyAggregationAlertProps {
+ sliceId?: number;
+}
+
+export const LegacyAggregationAlert = ({
+ sliceId,
+}: LegacyAggregationAlertProps) => {
+ const [tag, setTag] = useState<TagType | null>(null);
+ // A same-slice save is the other way (alongside this component's own
+ // "Accept" button) a legacy aggregation tag can go away -- saveModalActions
+ // deletes it server-side as part of a pivot_table_v2 save. This object
+ // changes identity on every successful save, so including it below
+ // re-fetches after a save and drops a tag the save already cleared,
+ // instead of leaving this component's local state stale until the user
+ // navigates away and back.
+ const lastSaveResult = useSelector(
+ (state: { saveModal?: { data?: unknown } }) => state.saveModal?.data,
+ );
+
+ useEffect(() => {
+ setTag(null);
+ if (!sliceId) {
+ return undefined;
+ }
+ // Guards the async callbacks below against a stale in-flight request --
+ // e.g. the user switches charts (`sliceId` changes) or saves again
+ // (`lastSaveResult` changes) before the first request resolves.
+ let cancelled = false;
+ fetchTags(
+ { objectType: 'chart', objectId: sliceId },
+ (tags: TagType[]) => {
+ if (cancelled) {
Review Comment:
The delayed-DELETE test covers the Accept race, but every tag GET in
`LegacyAggregationAlert.test.tsx` resolves before the chart changes, so this
`cancelled` guard is untested; deleting it keeps the suite green. If tagged
chart A's GET is still pending when the user opens untagged chart B, A's late
response sets A's tag and shows the notice on B. Could a test defer A's GET,
switch to B, resolve B, then resolve A, and assert the notice stays absent?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
