rusackas commented on code in PR #44657: URL: https://github.com/apache/superset/pull/44657#discussion_r4257033998
########## 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: Added a test that defers chart 1's tag GET, switches to untagged chart 2, then resolves the late response and asserts the notice stays absent (1f78fc7). I could not run jest here, so CI is the first real run of it. ########## 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 is already covered by 485812e: the test now asserts concrete per-reducer row and column summary values for the series-only aggregations (Count, Count Unique Values, List Unique Values, First, Last) instead of just a non-empty pivot. The sample variance/stdev gap with single-observation cells is tracked separately in #45215. ########## 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: Agreed on the variance/stdev path: that test still only asserts non-empty, and with one observation per (nation, gender) cell the summaries all collapse to 0. (485812e tightened the neighbouring series-only reducer test, but not this one.) The underlying leaf-pass gap is #45215, so I would rather pin concrete values once that is fixed than assert 0.0s here. -- 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]
