codeant-ai-for-open-source[bot] commented on code in PR #39452:
URL: https://github.com/apache/superset/pull/39452#discussion_r3525765823
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,310 @@ export interface DataEntry {
}
interface TTestTableProps {
- alpha: number;
+ alpha?: number;
data: DataEntry[];
groups: string[];
- liftValPrec: number;
+ liftValPrec?: number;
metric: string;
- pValPrec: number;
+ pValPrec?: number;
}
-interface TTestTableState {
- control: number;
- liftValues: (string | number)[];
- pValues: (string | number)[];
-}
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+ const computeLift = useCallback(
+ (values: DataPointValue[], controlValues: DataPointValue[]): string => {
+ // Compute the lift value between two time series
+ let sumValues = 0;
+ let sumControl = 0;
+ values.forEach((value, i) => {
+ sumValues += value.y;
+ sumControl += controlValues[i].y;
+ });
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
+ if (sumControl === 0) return 'NaN';
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
+ liftValPrec,
+ );
+ },
+ [liftValPrec],
+ );
- constructor(props: TTestTableProps) {
- super(props);
- this.state = {
- control: 0,
- liftValues: [],
- pValues: [],
- };
- }
+ const computePValue = useCallback(
+ (
+ values: DataPointValue[],
+ controlValues: DataPointValue[],
+ ): string | number => {
+ // Compute the p-value from Student's t-test
+ // between two time series
+ let diffSum = 0;
+ let diffSqSum = 0;
+ let finiteCount = 0;
+ values.forEach((value, i) => {
+ const diff = controlValues[i].y - value.y;
+ /* eslint-disable-next-line */
+ if (isFinite(diff)) {
+ finiteCount += 1;
+ diffSum += diff;
+ diffSqSum += diff * diff;
+ }
+ });
+ const tvalue = -Math.abs(
+ diffSum *
+ Math.sqrt(
+ (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
+ ),
+ );
+ try {
+ return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
+ pValPrec,
+ ); // two-sided test
+ } catch (error) {
+ return NaN;
+ }
+ },
+ [pValPrec],
+ );
- componentDidMount() {
- const { control } = this.state;
- this.computeTTest(control); // initially populate table
- }
+ const computeTTest = useCallback(
+ (controlIndex: number) => {
+ // Compute lift and p-values for each row
+ // against the selected control
+ const newPValues: (string | number)[] = [];
+ const newLiftValues: (string | number)[] = [];
+ if (!data) {
+ return;
+ }
+ for (let i = 0; i < data.length; i += 1) {
+ if (i === controlIndex) {
+ newPValues.push('control');
+ newLiftValues.push('control');
+ } else {
+ newPValues.push(
+ computePValue(data[i].values, data[controlIndex].values),
+ );
+ newLiftValues.push(
+ computeLift(data[i].values, data[controlIndex].values),
+ );
+ }
+ }
+ setControl(controlIndex);
+ setLiftValues(newLiftValues);
+ setPValues(newPValues);
+ },
+ [data, computeLift, computePValue],
+ );
- getLiftStatus(row: number): string {
- const { control, liftValues } = this.state;
- // Get a css class name for coloring
- if (row === control) {
- return 'control';
- }
- const liftVal = liftValues[row];
- if (Number.isNaN(liftVal) || !Number.isFinite(liftVal)) {
- return 'invalid'; // infinite or NaN values
+ // Recompute table when data or control row changes, keeping control index
in range
+ useEffect(() => {
+ if (!data || data.length === 0) {
+ setControl(0);
+ setLiftValues([]);
+ setPValues([]);
+ return;
}
- return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
- }
-
- getPValueStatus(row: number): string {
- const { control, pValues } = this.state;
- if (row === control) {
- return 'control';
- }
- const pVal = pValues[row];
- if (Number.isNaN(pVal) || !Number.isFinite(pVal)) {
- return 'invalid';
+ const safeControlIndex = Math.min(control, data.length - 1);
+ if (safeControlIndex !== control) {
+ setControl(safeControlIndex);
+ computeTTest(safeControlIndex);
+ } else {
+ computeTTest(control);
}
+ }, [computeTTest, control, data]);
- return ''; // p-values won't normally be colored
- }
+ const getLiftStatus = useCallback(
+ (row: number): string => {
+ // Get a css class name for coloring
+ if (row === control) {
+ return 'control';
+ }
+ const liftVal = liftValues[row];
+ const numericLiftVal = Number(liftVal);
+ if (Number.isNaN(numericLiftVal) || !Number.isFinite(numericLiftVal)) {
+ return 'invalid'; // infinite or NaN values
+ }
- getSignificance(row: number): string | boolean {
- const { control, pValues } = this.state;
- const { alpha } = this.props;
- // Color significant as green, else red
- if (row === control) {
- return 'control';
- }
+ return numericLiftVal >= 0 ? 'true' : 'false'; // green on true, red on
false
+ },
+ [control, liftValues],
+ );
- // p-values significant below set threshold
- return Number(pValues[row]) <= alpha;
- }
+ const getPValueStatus = useCallback(
+ (row: number): string => {
+ if (row === control) {
+ return 'control';
+ }
+ const pVal = pValues[row];
+ const numericPVal = Number(pVal);
+ if (Number.isNaN(numericPVal) || !Number.isFinite(numericPVal)) {
+ return 'invalid';
+ }
- computeLift(values: DataPointValue[], control: DataPointValue[]): string {
- const { liftValPrec } = this.props;
- // Compute the lift value between two time series
- let sumValues = 0;
- let sumControl = 0;
- values.forEach((value, i) => {
- sumValues += value.y;
- sumControl += control[i].y;
- });
+ return ''; // p-values won't normally be colored
+ },
+ [control, pValues],
+ );
- return (((sumValues - sumControl) / sumControl) *
100).toFixed(liftValPrec);
- }
-
- computePValue(
- values: DataPointValue[],
- control: DataPointValue[],
- ): string | number {
- const { pValPrec } = this.props;
- // Compute the p-value from Student's t-test
- // between two time series
- let diffSum = 0;
- let diffSqSum = 0;
- let finiteCount = 0;
- values.forEach((value, i) => {
- const diff = control[i].y - value.y;
- /* eslint-disable-next-line */
- if (isFinite(diff)) {
- finiteCount += 1;
- diffSum += diff;
- diffSqSum += diff * diff;
+ const getSignificance = useCallback(
+ (row: number): string | boolean => {
+ // Color significant as green, else red
+ if (row === control) {
+ return 'control';
}
- });
- const tvalue = -Math.abs(
- diffSum *
- Math.sqrt(
- (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
- ),
- );
- try {
- return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
- pValPrec,
- ); // two-sided test
- } catch (error) {
- return NaN;
- }
- }
- computeTTest(control: number) {
- // Compute lift and p-values for each row
- // against the selected control
- const { data } = this.props;
- const pValues: (string | number)[] = [];
- const liftValues: (string | number)[] = [];
- if (!data) {
- return;
- }
- for (let i = 0; i < data.length; i += 1) {
- if (i === control) {
- pValues.push('control');
- liftValues.push('control');
- } else {
- pValues.push(this.computePValue(data[i].values, data[control].values));
- liftValues.push(this.computeLift(data[i].values,
data[control].values));
- }
- }
- this.setState({ control, liftValues, pValues });
- }
+ // p-values significant below set threshold
+ return Number(pValues[row]) <= alpha;
+ },
+ [control, pValues, alpha],
+ );
- render() {
- const { data, metric, groups } = this.props;
- const { control, liftValues, pValues } = this.state;
+ const handleRowClick = useCallback(
+ (rowIndex: number) => {
+ computeTTest(rowIndex);
+ },
+ [computeTTest],
+ );
- if (!Array.isArray(groups) || groups.length === 0) {
- throw new Error('Group by param is required');
- }
+ if (!Array.isArray(groups) || groups.length === 0) {
+ throw new Error('Group by param is required');
+ }
- // Render column header for each group
- const columns = groups.map((group, i) => (
- <Th key={i} column={group}>
- {group}
- </Th>
- ));
- const numGroups = groups.length;
- // Columns for p-value, lift-value, and significance (true/false)
- columns.push(
- <Th key={numGroups + 1} column="pValue">
- p-value
- </Th>,
+ // Render column header for each group
+ const columns = groups.map((group, i) => (
+ <Th key={i} column={group}>
+ {group}
+ </Th>
+ ));
+ const numGroups = groups.length;
+ // Columns for p-value, lift-value, and significance (true/false)
+ columns.push(
+ <Th key={numGroups + 1} column="pValue">
+ p-value
+ </Th>,
+ );
+ columns.push(
+ <Th key={numGroups + 2} column="liftValue">
+ Lift %
+ </Th>,
+ );
+ columns.push(
+ <Th key={numGroups + 3} column="significant">
+ Significant
+ </Th>,
+ );
+
+ const rows = data.map((entry, i) => {
Review Comment:
**Suggestion:** The render path assumes `data` is always defined, but this
component is called with `data[metric]` from the parent and can receive
`undefined` when a metric key is missing. That will throw at runtime when
calling `.map`. Add a render-time guard (or default `data` to an empty array)
before building rows. [null pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Paired t-test table crashes on missing metric data.
⚠️ Visualization fails when backend omits metric key.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A Paired t-test Table chart is rendered using the plugin defined in
`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/index.ts:28-49`,
which
loads `PairedTTest` and passes props from `transformProps.ts:21-41`.
2. In `transformProps.ts:31-34`, the chart props include `data:
queriesData[0].data` and
`metrics` derived from `formData.metrics:24-29`, so `PairedTTest.tsx:23-31`
receives
`data` as a `Record<string, DataEntry[]>` and `metrics` as a list of metric
names.
3. For some metric name in `metrics`, the runtime `queriesData[0].data`
object lacks a
corresponding property (e.g., an incomplete or mismatched backend response),
so in
`PairedTTest.tsx:123-132` the JSX `{metrics.map((metric, i) => ( <TTestTable
...
data={data[metric]} /> ))}` passes `undefined` as the `data` prop to
`TTestTable`.
4. Inside `TTestTable.tsx`, the effect at `lines 138-154` safely
early-returns when
`!data`, but the render path still executes `const rows = data.map((entry,
i) => {` at
`line 237`, causing a runtime `TypeError: Cannot read properties of
undefined (reading
'map')` and the Paired t-test table fails to render.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=00d230d73aac4a22bcac63bf1d4dd9d2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=00d230d73aac4a22bcac63bf1d4dd9d2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx
**Line:** 237:237
**Comment:**
*Null Pointer: The render path assumes `data` is always defined, but
this component is called with `data[metric]` from the parent and can receive
`undefined` when a metric key is missing. That will throw at runtime when
calling `.map`. Add a render-time guard (or default `data` to an empty array)
before building rows.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=a843c53093765034f1f1da8a9c45fecd807cced212ec13a8f1d6269cdabe981e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=a843c53093765034f1f1da8a9c45fecd807cced212ec13a8f1d6269cdabe981e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,310 @@ export interface DataEntry {
}
interface TTestTableProps {
- alpha: number;
+ alpha?: number;
data: DataEntry[];
groups: string[];
- liftValPrec: number;
+ liftValPrec?: number;
metric: string;
- pValPrec: number;
+ pValPrec?: number;
}
-interface TTestTableState {
- control: number;
- liftValues: (string | number)[];
- pValues: (string | number)[];
-}
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+ const computeLift = useCallback(
+ (values: DataPointValue[], controlValues: DataPointValue[]): string => {
+ // Compute the lift value between two time series
+ let sumValues = 0;
+ let sumControl = 0;
+ values.forEach((value, i) => {
+ sumValues += value.y;
+ sumControl += controlValues[i].y;
+ });
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
+ if (sumControl === 0) return 'NaN';
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
+ liftValPrec,
+ );
+ },
+ [liftValPrec],
+ );
- constructor(props: TTestTableProps) {
- super(props);
- this.state = {
- control: 0,
- liftValues: [],
- pValues: [],
- };
- }
+ const computePValue = useCallback(
+ (
+ values: DataPointValue[],
+ controlValues: DataPointValue[],
+ ): string | number => {
+ // Compute the p-value from Student's t-test
+ // between two time series
+ let diffSum = 0;
+ let diffSqSum = 0;
+ let finiteCount = 0;
+ values.forEach((value, i) => {
+ const diff = controlValues[i].y - value.y;
+ /* eslint-disable-next-line */
+ if (isFinite(diff)) {
+ finiteCount += 1;
+ diffSum += diff;
+ diffSqSum += diff * diff;
+ }
+ });
+ const tvalue = -Math.abs(
+ diffSum *
+ Math.sqrt(
+ (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
+ ),
+ );
+ try {
+ return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
+ pValPrec,
+ ); // two-sided test
+ } catch (error) {
+ return NaN;
+ }
+ },
+ [pValPrec],
+ );
- componentDidMount() {
- const { control } = this.state;
- this.computeTTest(control); // initially populate table
- }
+ const computeTTest = useCallback(
+ (controlIndex: number) => {
+ // Compute lift and p-values for each row
+ // against the selected control
+ const newPValues: (string | number)[] = [];
+ const newLiftValues: (string | number)[] = [];
+ if (!data) {
+ return;
+ }
+ for (let i = 0; i < data.length; i += 1) {
+ if (i === controlIndex) {
+ newPValues.push('control');
+ newLiftValues.push('control');
+ } else {
+ newPValues.push(
+ computePValue(data[i].values, data[controlIndex].values),
+ );
+ newLiftValues.push(
+ computeLift(data[i].values, data[controlIndex].values),
+ );
+ }
+ }
+ setControl(controlIndex);
+ setLiftValues(newLiftValues);
+ setPValues(newPValues);
+ },
+ [data, computeLift, computePValue],
+ );
- getLiftStatus(row: number): string {
- const { control, liftValues } = this.state;
- // Get a css class name for coloring
- if (row === control) {
- return 'control';
- }
- const liftVal = liftValues[row];
- if (Number.isNaN(liftVal) || !Number.isFinite(liftVal)) {
- return 'invalid'; // infinite or NaN values
+ // Recompute table when data or control row changes, keeping control index
in range
+ useEffect(() => {
+ if (!data || data.length === 0) {
+ setControl(0);
+ setLiftValues([]);
+ setPValues([]);
+ return;
}
- return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
- }
-
- getPValueStatus(row: number): string {
- const { control, pValues } = this.state;
- if (row === control) {
- return 'control';
- }
- const pVal = pValues[row];
- if (Number.isNaN(pVal) || !Number.isFinite(pVal)) {
- return 'invalid';
+ const safeControlIndex = Math.min(control, data.length - 1);
+ if (safeControlIndex !== control) {
+ setControl(safeControlIndex);
+ computeTTest(safeControlIndex);
+ } else {
+ computeTTest(control);
}
+ }, [computeTTest, control, data]);
- return ''; // p-values won't normally be colored
- }
+ const getLiftStatus = useCallback(
+ (row: number): string => {
+ // Get a css class name for coloring
+ if (row === control) {
+ return 'control';
+ }
+ const liftVal = liftValues[row];
+ const numericLiftVal = Number(liftVal);
+ if (Number.isNaN(numericLiftVal) || !Number.isFinite(numericLiftVal)) {
+ return 'invalid'; // infinite or NaN values
+ }
- getSignificance(row: number): string | boolean {
- const { control, pValues } = this.state;
- const { alpha } = this.props;
- // Color significant as green, else red
- if (row === control) {
- return 'control';
- }
+ return numericLiftVal >= 0 ? 'true' : 'false'; // green on true, red on
false
+ },
+ [control, liftValues],
+ );
- // p-values significant below set threshold
- return Number(pValues[row]) <= alpha;
- }
+ const getPValueStatus = useCallback(
+ (row: number): string => {
+ if (row === control) {
+ return 'control';
+ }
+ const pVal = pValues[row];
+ const numericPVal = Number(pVal);
+ if (Number.isNaN(numericPVal) || !Number.isFinite(numericPVal)) {
+ return 'invalid';
+ }
- computeLift(values: DataPointValue[], control: DataPointValue[]): string {
- const { liftValPrec } = this.props;
- // Compute the lift value between two time series
- let sumValues = 0;
- let sumControl = 0;
- values.forEach((value, i) => {
- sumValues += value.y;
- sumControl += control[i].y;
- });
+ return ''; // p-values won't normally be colored
+ },
+ [control, pValues],
+ );
- return (((sumValues - sumControl) / sumControl) *
100).toFixed(liftValPrec);
- }
-
- computePValue(
- values: DataPointValue[],
- control: DataPointValue[],
- ): string | number {
- const { pValPrec } = this.props;
- // Compute the p-value from Student's t-test
- // between two time series
- let diffSum = 0;
- let diffSqSum = 0;
- let finiteCount = 0;
- values.forEach((value, i) => {
- const diff = control[i].y - value.y;
- /* eslint-disable-next-line */
- if (isFinite(diff)) {
- finiteCount += 1;
- diffSum += diff;
- diffSqSum += diff * diff;
+ const getSignificance = useCallback(
+ (row: number): string | boolean => {
+ // Color significant as green, else red
+ if (row === control) {
+ return 'control';
}
- });
- const tvalue = -Math.abs(
- diffSum *
- Math.sqrt(
- (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
- ),
- );
- try {
- return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
- pValPrec,
- ); // two-sided test
- } catch (error) {
- return NaN;
- }
- }
- computeTTest(control: number) {
- // Compute lift and p-values for each row
- // against the selected control
- const { data } = this.props;
- const pValues: (string | number)[] = [];
- const liftValues: (string | number)[] = [];
- if (!data) {
- return;
- }
- for (let i = 0; i < data.length; i += 1) {
- if (i === control) {
- pValues.push('control');
- liftValues.push('control');
- } else {
- pValues.push(this.computePValue(data[i].values, data[control].values));
- liftValues.push(this.computeLift(data[i].values,
data[control].values));
- }
- }
- this.setState({ control, liftValues, pValues });
- }
+ // p-values significant below set threshold
+ return Number(pValues[row]) <= alpha;
+ },
+ [control, pValues, alpha],
+ );
- render() {
- const { data, metric, groups } = this.props;
- const { control, liftValues, pValues } = this.state;
+ const handleRowClick = useCallback(
+ (rowIndex: number) => {
+ computeTTest(rowIndex);
+ },
+ [computeTTest],
+ );
Review Comment:
**Suggestion:** Row click already recomputes and sets all t-test state, but
the effect also recomputes again whenever `control` changes, causing duplicate
heavy calculations and an extra render per click. Keep one source of
recomputation (either set control in click and let the effect compute once, or
compute in click and make the effect react only to external prop changes).
[performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Row clicks perform duplicate t-test computations.
⚠️ Extra re-renders degrade responsiveness on large datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Paired t-test Table chart plugin loads `TTestTable` for each metric
via
`PairedTTest.tsx:109-133`, rendering rows and wiring `onClick={() =>
handleRowClick(i)}`
on each `Tr` at `TTestTable.tsx:270-275`.
2. On initial render, `useEffect` in `TTestTable.tsx:138-154` runs, calls
`computeTTest(control)` at `lines 147-152`, and sets `control`,
`liftValues`, and
`pValues` via `computeTTest:109-133`.
3. When a user clicks a data row, the `onClick` handler at
`TTestTable.tsx:270-275`
invokes `handleRowClick` defined at `lines 202-207`, which immediately calls
`computeTTest(rowIndex)` and again sets `control`, `liftValues`, and
`pValues`.
4. Because `computeTTest` updates `control`, the `useEffect` dependency
`[computeTTest,
control, data]` at `line 154` causes the effect to run again after the
click, which calls
`computeTTest(control)` a second time; this double invocation per click
recomputes the
t-test statistics and updates state twice, producing avoidable extra
calculations and an
extra render on every row interaction.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=65de7d5df3aa48dba967f50bc146c915&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=65de7d5df3aa48dba967f50bc146c915&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx
**Line:** 202:207
**Comment:**
*Performance: Row click already recomputes and sets all t-test state,
but the effect also recomputes again whenever `control` changes, causing
duplicate heavy calculations and an extra render per click. Keep one source of
recomputation (either set control in click and let the effect compute once, or
compute in click and make the effect react only to external prop changes).
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=9edcb6f9782485ccaa2e9a8ac9589b821cd41968d62886975c2469889e643a9e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=9edcb6f9782485ccaa2e9a8ac9589b821cd41968d62886975c2469889e643a9e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,310 @@ export interface DataEntry {
}
interface TTestTableProps {
- alpha: number;
+ alpha?: number;
data: DataEntry[];
groups: string[];
- liftValPrec: number;
+ liftValPrec?: number;
metric: string;
- pValPrec: number;
+ pValPrec?: number;
}
-interface TTestTableState {
- control: number;
- liftValues: (string | number)[];
- pValues: (string | number)[];
-}
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+ const computeLift = useCallback(
+ (values: DataPointValue[], controlValues: DataPointValue[]): string => {
+ // Compute the lift value between two time series
+ let sumValues = 0;
+ let sumControl = 0;
+ values.forEach((value, i) => {
+ sumValues += value.y;
+ sumControl += controlValues[i].y;
+ });
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
+ if (sumControl === 0) return 'NaN';
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
+ liftValPrec,
+ );
+ },
+ [liftValPrec],
+ );
- constructor(props: TTestTableProps) {
- super(props);
- this.state = {
- control: 0,
- liftValues: [],
- pValues: [],
- };
- }
+ const computePValue = useCallback(
+ (
+ values: DataPointValue[],
+ controlValues: DataPointValue[],
+ ): string | number => {
+ // Compute the p-value from Student's t-test
+ // between two time series
+ let diffSum = 0;
+ let diffSqSum = 0;
+ let finiteCount = 0;
+ values.forEach((value, i) => {
+ const diff = controlValues[i].y - value.y;
+ /* eslint-disable-next-line */
+ if (isFinite(diff)) {
+ finiteCount += 1;
+ diffSum += diff;
+ diffSqSum += diff * diff;
+ }
+ });
+ const tvalue = -Math.abs(
+ diffSum *
+ Math.sqrt(
+ (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
+ ),
+ );
+ try {
+ return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
+ pValPrec,
+ ); // two-sided test
+ } catch (error) {
+ return NaN;
+ }
+ },
+ [pValPrec],
+ );
- componentDidMount() {
- const { control } = this.state;
- this.computeTTest(control); // initially populate table
- }
+ const computeTTest = useCallback(
+ (controlIndex: number) => {
+ // Compute lift and p-values for each row
+ // against the selected control
+ const newPValues: (string | number)[] = [];
+ const newLiftValues: (string | number)[] = [];
+ if (!data) {
+ return;
+ }
+ for (let i = 0; i < data.length; i += 1) {
+ if (i === controlIndex) {
+ newPValues.push('control');
+ newLiftValues.push('control');
+ } else {
+ newPValues.push(
+ computePValue(data[i].values, data[controlIndex].values),
+ );
+ newLiftValues.push(
+ computeLift(data[i].values, data[controlIndex].values),
+ );
+ }
+ }
+ setControl(controlIndex);
+ setLiftValues(newLiftValues);
+ setPValues(newPValues);
+ },
+ [data, computeLift, computePValue],
+ );
- getLiftStatus(row: number): string {
- const { control, liftValues } = this.state;
- // Get a css class name for coloring
- if (row === control) {
- return 'control';
- }
- const liftVal = liftValues[row];
- if (Number.isNaN(liftVal) || !Number.isFinite(liftVal)) {
- return 'invalid'; // infinite or NaN values
+ // Recompute table when data or control row changes, keeping control index
in range
+ useEffect(() => {
+ if (!data || data.length === 0) {
+ setControl(0);
+ setLiftValues([]);
+ setPValues([]);
+ return;
}
- return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
- }
-
- getPValueStatus(row: number): string {
- const { control, pValues } = this.state;
- if (row === control) {
- return 'control';
- }
- const pVal = pValues[row];
- if (Number.isNaN(pVal) || !Number.isFinite(pVal)) {
- return 'invalid';
+ const safeControlIndex = Math.min(control, data.length - 1);
+ if (safeControlIndex !== control) {
+ setControl(safeControlIndex);
+ computeTTest(safeControlIndex);
+ } else {
+ computeTTest(control);
}
+ }, [computeTTest, control, data]);
- return ''; // p-values won't normally be colored
- }
+ const getLiftStatus = useCallback(
+ (row: number): string => {
+ // Get a css class name for coloring
+ if (row === control) {
+ return 'control';
+ }
+ const liftVal = liftValues[row];
+ const numericLiftVal = Number(liftVal);
+ if (Number.isNaN(numericLiftVal) || !Number.isFinite(numericLiftVal)) {
+ return 'invalid'; // infinite or NaN values
+ }
- getSignificance(row: number): string | boolean {
- const { control, pValues } = this.state;
- const { alpha } = this.props;
- // Color significant as green, else red
- if (row === control) {
- return 'control';
- }
+ return numericLiftVal >= 0 ? 'true' : 'false'; // green on true, red on
false
+ },
+ [control, liftValues],
+ );
- // p-values significant below set threshold
- return Number(pValues[row]) <= alpha;
- }
+ const getPValueStatus = useCallback(
+ (row: number): string => {
+ if (row === control) {
+ return 'control';
+ }
+ const pVal = pValues[row];
+ const numericPVal = Number(pVal);
+ if (Number.isNaN(numericPVal) || !Number.isFinite(numericPVal)) {
+ return 'invalid';
+ }
- computeLift(values: DataPointValue[], control: DataPointValue[]): string {
- const { liftValPrec } = this.props;
- // Compute the lift value between two time series
- let sumValues = 0;
- let sumControl = 0;
- values.forEach((value, i) => {
- sumValues += value.y;
- sumControl += control[i].y;
- });
+ return ''; // p-values won't normally be colored
+ },
+ [control, pValues],
+ );
- return (((sumValues - sumControl) / sumControl) *
100).toFixed(liftValPrec);
- }
-
- computePValue(
- values: DataPointValue[],
- control: DataPointValue[],
- ): string | number {
- const { pValPrec } = this.props;
- // Compute the p-value from Student's t-test
- // between two time series
- let diffSum = 0;
- let diffSqSum = 0;
- let finiteCount = 0;
- values.forEach((value, i) => {
- const diff = control[i].y - value.y;
- /* eslint-disable-next-line */
- if (isFinite(diff)) {
- finiteCount += 1;
- diffSum += diff;
- diffSqSum += diff * diff;
+ const getSignificance = useCallback(
+ (row: number): string | boolean => {
+ // Color significant as green, else red
+ if (row === control) {
+ return 'control';
}
- });
- const tvalue = -Math.abs(
- diffSum *
- Math.sqrt(
- (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
- ),
- );
- try {
- return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
- pValPrec,
- ); // two-sided test
- } catch (error) {
- return NaN;
- }
- }
- computeTTest(control: number) {
- // Compute lift and p-values for each row
- // against the selected control
- const { data } = this.props;
- const pValues: (string | number)[] = [];
- const liftValues: (string | number)[] = [];
- if (!data) {
- return;
- }
- for (let i = 0; i < data.length; i += 1) {
- if (i === control) {
- pValues.push('control');
- liftValues.push('control');
- } else {
- pValues.push(this.computePValue(data[i].values, data[control].values));
- liftValues.push(this.computeLift(data[i].values,
data[control].values));
- }
- }
- this.setState({ control, liftValues, pValues });
- }
+ // p-values significant below set threshold
+ return Number(pValues[row]) <= alpha;
+ },
+ [control, pValues, alpha],
+ );
- render() {
- const { data, metric, groups } = this.props;
- const { control, liftValues, pValues } = this.state;
+ const handleRowClick = useCallback(
+ (rowIndex: number) => {
+ computeTTest(rowIndex);
+ },
+ [computeTTest],
+ );
- if (!Array.isArray(groups) || groups.length === 0) {
- throw new Error('Group by param is required');
- }
+ if (!Array.isArray(groups) || groups.length === 0) {
+ throw new Error('Group by param is required');
+ }
- // Render column header for each group
- const columns = groups.map((group, i) => (
- <Th key={i} column={group}>
- {group}
- </Th>
- ));
- const numGroups = groups.length;
- // Columns for p-value, lift-value, and significance (true/false)
- columns.push(
- <Th key={numGroups + 1} column="pValue">
- p-value
- </Th>,
+ // Render column header for each group
+ const columns = groups.map((group, i) => (
+ <Th key={i} column={group}>
+ {group}
+ </Th>
+ ));
+ const numGroups = groups.length;
+ // Columns for p-value, lift-value, and significance (true/false)
+ columns.push(
+ <Th key={numGroups + 1} column="pValue">
+ p-value
+ </Th>,
+ );
+ columns.push(
+ <Th key={numGroups + 2} column="liftValue">
+ Lift %
+ </Th>,
+ );
+ columns.push(
+ <Th key={numGroups + 3} column="significant">
+ Significant
+ </Th>,
+ );
+
+ const rows = data.map((entry, i) => {
+ const values = groups.map(
+ (
+ group,
+ j, // group names
+ ) => <Td key={j} column={group} data={entry.group[j]} />,
);
- columns.push(
- <Th key={numGroups + 2} column="liftValue">
- Lift %
- </Th>,
+ values.push(
+ <Td
+ key={numGroups + 1}
+ className={getPValueStatus(i)}
+ column="pValue"
+ data={pValues[i]}
+ />,
);
- columns.push(
- <Th key={numGroups + 3} column="significant">
- Significant
- </Th>,
+ values.push(
+ <Td
+ key={numGroups + 2}
+ className={getLiftStatus(i)}
+ column="liftValue"
+ data={liftValues[i]}
+ />,
+ );
+ values.push(
+ <Td
+ key={numGroups + 3}
+ className={getSignificance(i).toString()}
+ column="significant"
+ data={getSignificance(i)}
+ />,
);
- const rows = data.map((entry, i) => {
- const values = groups.map(
- (
- group,
- j, // group names
- ) => <Td key={j} column={group} data={entry.group[j]} />,
- );
- values.push(
- <Td
- key={numGroups + 1}
- className={this.getPValueStatus(i)}
- column="pValue"
- data={pValues[i]}
- />,
- );
- values.push(
- <Td
- key={numGroups + 2}
- className={this.getLiftStatus(i)}
- column="liftValue"
- data={liftValues[i]}
- />,
- );
- values.push(
- <Td
- key={numGroups + 3}
- className={this.getSignificance(i).toString()}
- column="significant"
- data={this.getSignificance(i)}
- />,
- );
- return (
- <Tr
- key={i}
- className={i === control ? 'control' : ''}
- onClick={this.computeTTest.bind(this, i)}
- >
- {values}
- </Tr>
- );
- });
- // When sorted ascending, 'control' will always be at top
- type SortConfigItem =
- | string
- | { column: string; sortFunction: (a: string, b: string) => number };
- const sortConfig: SortConfigItem[] = (groups as SortConfigItem[]).concat([
- {
- column: 'pValue',
- sortFunction: (a: string, b: string) => {
- if (a === 'control') {
- return -1;
- }
- if (b === 'control') {
- return 1;
- }
+ return (
+ <Tr
+ key={i}
+ className={i === control ? 'control' : ''}
+ onClick={() => handleRowClick(i)}
+ >
+ {values}
+ </Tr>
+ );
+ });
- return a > b ? 1 : -1; // p-values ascending
+ // When sorted ascending, 'control' will always be at top
+ type SortConfigItem =
+ string | { column: string; sortFunction: (a: string, b: string) => number
};
+
+ const sortConfig: SortConfigItem[] = useMemo(
+ () =>
+ (groups as SortConfigItem[]).concat([
+ {
+ column: 'pValue',
+ sortFunction: (a: string, b: string) => {
+ if (a === 'control') {
+ return -1;
+ }
+ if (b === 'control') {
+ return 1;
+ }
+
+ return a > b ? 1 : -1; // p-values ascending
Review Comment:
**Suggestion:** The custom sort comparators never return `0` when values are
equal, which violates comparator contract and can produce unstable or incorrect
ordering when duplicate values exist. Return `0` for equality in each
comparator to ensure deterministic sorting behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Table sort may reorder equal p-value rows unexpectedly.
⚠️ Inconsistent ordering slightly degrades data interpretation.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The `sortConfig` array is created in `TTestTable.tsx:284-327` and passed
to the
Reactable `Table` via `sortable={sortConfig}` at `line 333`, controlling
sorting for the
`pValue`, `liftValue`, and `significant` columns.
2. For the `pValue` column, when two non-control rows share the same p-value
string, the
comparator at `lines 289-297` runs with arguments `(a, b)` and `(b, a)`; in
both cases,
because `a === b`, the expression `a > b ? 1 : -1` yields `-1`, so the
comparator claims
the first argument is less than the second regardless of order.
3. This violates comparator antisymmetry and equality semantics (compare(a,
b) and
compare(b, a) both return -1), so the underlying sorting algorithm in
Reactable/Array.sort
can produce inconsistent ordering of rows with equal p-values, e.g.,
equal-value rows may
flip order or produce non-deterministic ordering when toggling column sort.
4. Similar non-equality behavior exists in the `liftValue` comparator at
`lines 301-310`
and `significant` comparator at `lines 315-323`, making table ordering for
duplicate
values potentially unstable; adding explicit `if (a === b) return 0;`
branches would
restore deterministic, contract-compliant sorting.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1197452803ae462cac8a38c47cceeb3c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1197452803ae462cac8a38c47cceeb3c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx
**Line:** 297:297
**Comment:**
*Logic Error: The custom sort comparators never return `0` when values
are equal, which violates comparator contract and can produce unstable or
incorrect ordering when duplicate values exist. Return `0` for equality in each
comparator to ensure deterministic sorting behavior.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=457b94b187cf6d3374058a727666e794a4f4ca917d76343368e33499c4c57c14&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=457b94b187cf6d3374058a727666e794a4f4ca917d76343368e33499c4c57c14&reaction=dislike'>👎</a>
--
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]
