codeant-ai-for-open-source[bot] commented on code in PR #38080:
URL: https://github.com/apache/superset/pull/38080#discussion_r2824767505
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -92,6 +92,128 @@ const numberFormat = function (optsIn?:
NumberFormatOptions): Formatter {
};
};
+/**
+ * Safely converts any value to a number for aggregation purposes
+ * Handles null/undefined, strings, and non-numeric values
+ */
+function toAggregationNumber(value: unknown): number {
+ if (value == null) return 0;
+ if (typeof value === 'number') return value;
Review Comment:
**Suggestion:** When aggregation values are NaN (for example from an average
over zero elements), `toAggregationNumber` returns NaN unchanged, so
`auto_agg_sum` becomes NaN and the hierarchical comparator uses `sumA > sumB ?
1 : -1`, which yields `-1` in both directions when comparing NaN to a finite
number; this breaks the comparator contract and can lead to inconsistent or
unpredictable sort order for hierarchical value sorting. Coercing NaN to 0 in
`toAggregationNumber` ensures group sums are always comparable and the
comparator remains antisymmetric. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Hierarchical value sorting can yield inconsistent row ordering.
- ⚠️ Pivot table rows with NaN aggregates sort unpredictably.
- ⚠️ Affects charts using 'Sum over Sum' aggregator.
```
</details>
```suggestion
if (typeof value === 'number') {
return Number.isNaN(value) ? 0 : value;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Instantiate `PivotData` (constructor at `utilities.ts:897-936`) with
props including:
- `aggregatorName: 'Sum over Sum'` (uses
`baseAggregatorTemplates.sumOverSum` defined
around `utilities.ts:540-572`),
- `rows: ['group']`, `vals: ['num', 'den']`,
- `rowOrder: 'value_a_to_z'`,
- `data` where, for at least one group value (e.g. `"G"`), all records
have `den`
missing or non-numeric so `Number(record[den])` is `NaN`.
2. During construction, `PivotData.processRecord()`
(`utilities.ts:1019-1068`) is called
via `PivotData.forEachRecord()`. For each record with `group === 'G'`, the
row total
aggregator for `['G']` is created from `sumOverSum` and `push()` is called,
leaving
`sumNum === 0` and `sumDenom === 0` for that row.
3. When any caller later requests sorted row keys via `getRowKeys()`
(`utilities.ts:1003-1007`), `sortKeys()` (`utilities.ts:1009-1044`) runs. For
`this.props.rowOrder === 'value_a_to_z'`, it calls:
`groupingValueSort(this.rowKeys, vr, this.subtotals.rowPartialOnTop,
true);`
where `vr = (r, c) => this.getAggregator(r, c).value()`. For row key
`['G']`,
`vr(['G'], [])` returns `NaN` from the `sumOverSum` aggregator (0 / 0).
4. Inside `buildGroupAggregates()` (`utilities.ts:119-142`), for key `['G']`
the code
executes:
`childNode.auto_agg_sum += toAggregationNumber(dataFunc(key, []));`
with `dataFunc(key, []) === NaN`. Current `toAggregationNumber()`
(`utilities.ts:99-107`) sees a `number` and returns `NaN` unchanged, so
`auto_agg_sum`
becomes `NaN`. Later, `createHierarchicalComparator()`
(`utilities.ts:147-195`)
compares group sums using `(sumA > sumB ? 1 : -1) * orderBasis`. When
either `sumA` or
`sumB` is `NaN`, both `comparator(a, b)` and `comparator(b, a)` evaluate
to `-1 *
orderBasis`, violating antisymmetry and causing `Array.prototype.sort()`
to produce
inconsistent/unpredictable ordering of the pivot table rows.
```
</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-pivot-table/src/react-pivottable/utilities.ts
**Line:** 101:101
**Comment:**
*Logic Error: When aggregation values are NaN (for example from an
average over zero elements), `toAggregationNumber` returns NaN unchanged, so
`auto_agg_sum` becomes NaN and the hierarchical comparator uses `sumA > sumB ?
1 : -1`, which yields `-1` in both directions when comparing NaN to a finite
number; this breaks the comparator contract and can lead to inconsistent or
unpredictable sort order for hierarchical value sorting. Coercing NaN to 0 in
`toAggregationNumber` ensures group sums are always comparable and the
comparator remains antisymmetric.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=0be3d2c7171a5ecaebe145ea572e451103c1de1f9050ba66ae8e51d355ea393c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=0be3d2c7171a5ecaebe145ea572e451103c1de1f9050ba66ae8e51d355ea393c&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]
