bito-code-review[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3556438821
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -103,35 +150,41 @@ test('TableRenderer renders row headers from pivot data',
() => {
});
test('TableRenderer renders aggregated cell values', () => {
- const props = buildDefaultProps();
+ const props = buildDefaultProps({
+ data: TAGGED_COUNT_DATA,
+ vals: ['value'],
+ });
renderWithTheme(<TableRenderer {...props} />);
- // With "Count" aggregator, each cell (row x col intersection) should
- // contain "1" because each combination appears exactly once.
+ // Each leaf cell (row x col intersection) holds the DB-computed value "1".
const cells = screen.getAllByRole('gridcell');
const cellTexts = cells.map(cell => cell.textContent);
- // There should be cell values of "1" for each of the four intersections
- // (blue+circle, blue+square, red+circle, red+square).
- const onesCount = cellTexts.filter(text => text === '1').length;
+ // There should be a "1" leaf cell for each of the four intersections
+ // (blue+circle, blue+square, red+circle, red+square). The default formatter
+ // renders with two decimals in this test (production applies the metric's
+ // own format).
+ const onesCount = cellTexts.filter(text => text === '1.00').length;
expect(onesCount).toBeGreaterThanOrEqual(4);
});
test('TableRenderer renders row totals when rowTotals is enabled', () => {
const props = buildDefaultProps({
+ data: TAGGED_COUNT_DATA,
+ vals: ['value'],
tableOptions: { rowTotals: true, colTotals: true },
});
renderWithTheme(<TableRenderer {...props} />);
- // Row totals column should show "2" for each color (blue has 2 records,
- // red has 2 records).
+ // Row totals column should show the DB-computed "2" for each color (blue has
+ // 2 records, red has 2 records).
const totalCells = screen
.getAllByRole('gridcell')
.filter(cell => cell.classList.contains('pvtTotal'));
expect(totalCells.length).toBeGreaterThan(0);
const totalValues = totalCells.map(cell => cell.textContent);
- expect(totalValues).toContain('2');
+ expect(totalValues).toContain('2.00');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Count aggregator assertion mismatch</b></div>
<div id="fix">
Same formatter mismatch as line 167: count aggregator uses usFmtInt (0
decimals), but assertion expects '2.00' (2 decimals).
</div>
</div>
<small><i>Code Review Run #275caa</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -103,35 +150,41 @@ test('TableRenderer renders row headers from pivot data',
() => {
});
test('TableRenderer renders aggregated cell values', () => {
- const props = buildDefaultProps();
+ const props = buildDefaultProps({
+ data: TAGGED_COUNT_DATA,
+ vals: ['value'],
+ });
renderWithTheme(<TableRenderer {...props} />);
- // With "Count" aggregator, each cell (row x col intersection) should
- // contain "1" because each combination appears exactly once.
+ // Each leaf cell (row x col intersection) holds the DB-computed value "1".
const cells = screen.getAllByRole('gridcell');
const cellTexts = cells.map(cell => cell.textContent);
- // There should be cell values of "1" for each of the four intersections
- // (blue+circle, blue+square, red+circle, red+square).
- const onesCount = cellTexts.filter(text => text === '1').length;
+ // There should be a "1" leaf cell for each of the four intersections
+ // (blue+circle, blue+square, red+circle, red+square). The default formatter
+ // renders with two decimals in this test (production applies the metric's
+ // own format).
+ const onesCount = cellTexts.filter(text => text === '1.00').length;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Count aggregator assertion mismatch</b></div>
<div id="fix">
The test's `aggregatorsFactory` passes `aggregatorTemplates.count()` without
a formatter, so it defaults to `usFmtInt` (0 decimal places). The assertion
expects `'1.00'` but count will produce `'1'`. The same issue appears on line
187 for `'2.00'` vs `'2'`.
</div>
</div>
<small><i>Code Review Run #275caa</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -930,14 +962,10 @@ class PivotData {
'PivotData',
);
- const aggregatorsFactory = this.props.aggregatorsFactory as (
- fmt: unknown,
- ) => Record<string, (vals: unknown) => (...args: unknown[]) => Aggregator>;
- const aggregatorName = this.props.aggregatorName as string;
const vals = this.props.vals as string[];
- this.aggregator = aggregatorsFactory(this.props.defaultFormatter)[
- aggregatorName
- ](vals);
+ // Values come pre-aggregated from the database (one query per rollup
level),
+ // so the pivot stores them verbatim via `cellValue` instead of
aggregating.
+ this.aggregator = cellValue(this.props.defaultFormatter as
Formatter)(vals);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing function export</b></div>
<div id="fix">
The new `cellValue` aggregator is used internally but not exported. This
breaks consistency with `aggregators` and `aggregatorTemplates` which are
exported. Consider exporting it alongside the other aggregators for parity and
easier unit testing.
</div>
</div>
<small><i>Code Review Run #275caa</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
