korbit-ai[bot] commented on code in PR #36050:
URL: https://github.com/apache/superset/pull/36050#discussion_r2508155563
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -470,6 +644,15 @@ export class TableRenderer extends Component {
namesMapping,
allowRenderHtml,
)}
+ <span
+ role="columnheader button"
+ tabIndex={0}
+ onClick={e => {
+ e.stopPropagation();
+ }}
Review Comment:
### Sort icon wrapper blocks sorting functionality <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sort icon span has an onClick handler that only calls stopPropagation()
but doesn't trigger any sorting functionality. The actual sorting is handled by
the getSortIcon function's onClick, but this outer handler prevents event
bubbling.
###### Why this matters
This creates a dead click zone where users can click on the sort icon area
but nothing happens, leading to a confusing user experience where sorting
appears broken.
###### Suggested change ā *Feature Preview*
Remove the outer onClick handler or make it call the sort function:
```jsx
<span
role="columnheader button"
tabIndex={0}
>
{visibleSortIcon && getSortIcon(i)}
</span>
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a44b0113-66dd-47b0-b737-e611b8fdcc4b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a44b0113-66dd-47b0-b737-e611b8fdcc4b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a44b0113-66dd-47b0-b737-e611b8fdcc4b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a44b0113-66dd-47b0-b737-e611b8fdcc4b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a44b0113-66dd-47b0-b737-e611b8fdcc4b)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:59d9dfdf-cb17-46d3-866f-e95f7b8971ed -->
[](59d9dfdf-cb17-46d3-866f-e95f7b8971ed)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +418,86 @@ export class TableRenderer extends Component {
return spans;
}
+ calculateGroups(pivotData, visibleColKey, columnIndex) {
+ const groups = {};
+ const rows = pivotData.rowKeys;
+ rows.forEach(rowKey => {
+ let current = groups;
+ let sumGroup = 0;
+ rowKey.forEach(key => {
+ if (!current[key]) {
+ current[key] = { currentVal: 0 };
+ }
+ sumGroup += pivotData
+ .getAggregator(rowKey, visibleColKey[columnIndex])
+ .value();
+ current[key].currentVal = sumGroup;
+ current = current[key];
+ });
+ });
+ return groups;
+ }
+
+ sortAndCacheData(groups, sortOrder, subtotals, maxRowIndex) {
+ const { rowEnabled, rowPartialOnTop } = subtotals;
+ const sortedGroups = sortHierarchicalObject(
+ groups,
+ sortOrder,
+ rowPartialOnTop,
+ );
+ return convertToArray(
+ sortedGroups,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ );
+ }
+
+ sortData(columnIndex, visibleColKeys, pivotData, maxRowIndex) {
+ this.setState(state => {
+ const { sortingOrder, activeSortColumn } = state;
+
+ const newSortingOrder = [];
+ let newDirection = 'asc';
+
+ if (activeSortColumn === columnIndex) {
+ newDirection = sortingOrder[columnIndex] === 'asc' ? 'desc' : 'asc';
+ }
+
+ newSortingOrder[columnIndex] = newDirection;
+
+ const cacheKey = `${columnIndex}-${newDirection}`;
+ let newRowKeys;
+ if (this.sortCache.has(cacheKey)) {
+ const cachedRowKeys = this.sortCache.get(cacheKey);
+ newRowKeys = cachedRowKeys;
+ } else {
+ const groups = this.calculateGroups(
+ pivotData,
+ visibleColKeys,
+ columnIndex,
+ );
+ const sortedRowKeys = this.sortAndCacheData(
+ groups,
+ newDirection,
+ pivotData.subtotals,
+ maxRowIndex,
+ );
+ this.sortCache.set(cacheKey, sortedRowKeys);
+ newRowKeys = sortedRowKeys;
+ }
+ this.cachedBasePivotSettings = {
+ ...this.cachedBasePivotSettings,
+ rowKeys: newRowKeys,
+ };
Review Comment:
### Direct mutation of cached pivot settings <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sortData method modifies cachedBasePivotSettings directly, but this
cached object is used throughout the component for rendering. This mutation can
cause inconsistent state between the original pivot data and the sorted data.
###### Why this matters
This direct mutation can lead to rendering issues where the component
displays sorted data even when it shouldn't, or fails to revert to original
data when sorting is cleared. It breaks the separation between original and
sorted data states.
###### Suggested change ā *Feature Preview*
Store sorted row keys in component state instead of mutating the cached
settings:
```jsx
return {
sortingOrder: newSortingOrder,
activeSortColumn: columnIndex,
sortedRowKeys: newRowKeys,
};
```
Then use `this.state.sortedRowKeys || this.cachedBasePivotSettings.rowKeys`
in the render method.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1093021e-884f-4219-8e49-83c6b0e42559/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1093021e-884f-4219-8e49-83c6b0e42559?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1093021e-884f-4219-8e49-83c6b0e42559?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1093021e-884f-4219-8e49-83c6b0e42559?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1093021e-884f-4219-8e49-83c6b0e42559)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:eb17698a-5dd1-4815-b086-e161837e94b1 -->
[](eb17698a-5dd1-4815-b086-e161837e94b1)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +418,86 @@ export class TableRenderer extends Component {
return spans;
}
+ calculateGroups(pivotData, visibleColKey, columnIndex) {
+ const groups = {};
+ const rows = pivotData.rowKeys;
+ rows.forEach(rowKey => {
+ let current = groups;
+ let sumGroup = 0;
+ rowKey.forEach(key => {
+ if (!current[key]) {
+ current[key] = { currentVal: 0 };
+ }
+ sumGroup += pivotData
+ .getAggregator(rowKey, visibleColKey[columnIndex])
+ .value();
+ current[key].currentVal = sumGroup;
+ current = current[key];
+ });
+ });
+ return groups;
+ }
+
+ sortAndCacheData(groups, sortOrder, subtotals, maxRowIndex) {
+ const { rowEnabled, rowPartialOnTop } = subtotals;
+ const sortedGroups = sortHierarchicalObject(
+ groups,
+ sortOrder,
+ rowPartialOnTop,
+ );
+ return convertToArray(
+ sortedGroups,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ );
+ }
+
+ sortData(columnIndex, visibleColKeys, pivotData, maxRowIndex) {
+ this.setState(state => {
+ const { sortingOrder, activeSortColumn } = state;
+
+ const newSortingOrder = [];
+ let newDirection = 'asc';
+
+ if (activeSortColumn === columnIndex) {
+ newDirection = sortingOrder[columnIndex] === 'asc' ? 'desc' : 'asc';
+ }
+
+ newSortingOrder[columnIndex] = newDirection;
Review Comment:
### Sorting state reset loses previous column sorts <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sortingOrder array is completely reset to an empty array on each sort
operation, which loses all previous sorting states for other columns. Only the
current column's sort direction is preserved.
###### Why this matters
Users will lose their sorting configuration for other columns when they sort
a new column, making multi-column sorting impossible and creating a poor user
experience.
###### Suggested change ā *Feature Preview*
Preserve existing sorting order and only update the current column:
```jsx
const newSortingOrder = [...sortingOrder];
let newDirection = 'asc';
if (activeSortColumn === columnIndex) {
newDirection = sortingOrder[columnIndex] === 'asc' ? 'desc' : 'asc';
}
newSortingOrder[columnIndex] = newDirection;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc9a9186-427a-4f4c-8031-1e86c2d3bc98/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc9a9186-427a-4f4c-8031-1e86c2d3bc98?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc9a9186-427a-4f4c-8031-1e86c2d3bc98?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc9a9186-427a-4f4c-8031-1e86c2d3bc98?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc9a9186-427a-4f4c-8031-1e86c2d3bc98)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4806b583-f114-4635-b723-49aaed61acb3 -->
[](4806b583-f114-4635-b723-49aaed61acb3)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -71,15 +74,82 @@ function displayHeaderCell(
);
}
+function sortHierarchicalObject(obj, objSort, rowPartialOnTop) {
+ const sortedKeys = Object.keys(obj).sort((a, b) => {
+ const valA = obj[a].currentVal || 0;
+ const valB = obj[b].currentVal || 0;
+ if (rowPartialOnTop) {
+ if (obj[a].currentVal !== undefined && obj[b].currentVal === undefined) {
+ return -1;
+ }
+ if (obj[b].currentVal !== undefined && obj[a].currentVal === undefined) {
+ return 1;
+ }
+ }
+ return objSort === 'asc' ? valA - valB : valB - valA;
+ });
+ return sortedKeys.reduce((acc, key) => {
+ acc[key] =
+ typeof obj[key] === 'object' && !Array.isArray(obj[key])
+ ? sortHierarchicalObject(obj[key], objSort, rowPartialOnTop)
+ : obj[key];
+ return acc;
+ }, {});
+}
+
+function convertToArray(
+ obj,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ parentKeys = [],
+ result = [],
+ flag = false,
+) {
Review Comment:
### Complex Function Signature with Mutable Defaults <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function has too many parameters and uses mutable default parameters
(result array), making it hard to understand and potentially causing side
effects.
###### Why this matters
Functions with many parameters are difficult to use correctly and maintain.
Mutable default parameters can lead to unexpected behavior across multiple
function calls.
###### Suggested change ā *Feature Preview*
Refactor to use an options object and avoid mutable defaults:
```javascript
function convertToArray(obj, options) {
const {
rowEnabled,
rowPartialOnTop,
maxRowIndex,
parentKeys = [],
} = options;
const result = [];
// ... implementation
return result;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7c33942-6902-4b7d-bf3d-7a3128f4acae/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7c33942-6902-4b7d-bf3d-7a3128f4acae?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7c33942-6902-4b7d-bf3d-7a3128f4acae?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7c33942-6902-4b7d-bf3d-7a3128f4acae?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7c33942-6902-4b7d-bf3d-7a3128f4acae)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:095355bd-6f47-4ed6-be6b-ae2c66c68f13 -->
[](095355bd-6f47-4ed6-be6b-ae2c66c68f13)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -71,15 +74,82 @@ function displayHeaderCell(
);
}
+function sortHierarchicalObject(obj, objSort, rowPartialOnTop) {
+ const sortedKeys = Object.keys(obj).sort((a, b) => {
+ const valA = obj[a].currentVal || 0;
+ const valB = obj[b].currentVal || 0;
+ if (rowPartialOnTop) {
+ if (obj[a].currentVal !== undefined && obj[b].currentVal === undefined) {
+ return -1;
+ }
+ if (obj[b].currentVal !== undefined && obj[a].currentVal === undefined) {
+ return 1;
+ }
+ }
+ return objSort === 'asc' ? valA - valB : valB - valA;
+ });
+ return sortedKeys.reduce((acc, key) => {
+ acc[key] =
+ typeof obj[key] === 'object' && !Array.isArray(obj[key])
+ ? sortHierarchicalObject(obj[key], objSort, rowPartialOnTop)
+ : obj[key];
+ return acc;
+ }, {});
+}
+
+function convertToArray(
+ obj,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ parentKeys = [],
+ result = [],
+ flag = false,
+) {
+ let updatedFlag = flag;
+ Object.keys(obj).forEach(key => {
+ if (key === 'currentVal') {
+ return;
+ }
+ if (rowEnabled && rowPartialOnTop && parentKeys.length < maxRowIndex - 1) {
+ result.push(parentKeys.length > 0 ? [...parentKeys, key] : [key]);
+ updatedFlag = true;
+ }
+ if (typeof obj[key] === 'object' && !Array.isArray(obj[key])) {
+ convertToArray(
+ obj[key],
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ [...parentKeys, key],
+ result,
+ );
+ }
+ if (
+ parentKeys.length >= maxRowIndex - 1 ||
+ (rowEnabled && !rowPartialOnTop)
+ ) {
+ if (!updatedFlag) {
+ result.push(parentKeys.length > 0 ? [...parentKeys, key] : [key]);
+ return;
+ }
+ }
+ if (parentKeys.length === 0 && maxRowIndex === 1) {
+ result.push([key]);
+ }
+ });
+ return result;
+}
+
export class TableRenderer extends Component {
constructor(props) {
super(props);
// We need state to record which entries are collapsed and which aren't.
// This is an object with flat-keys indicating if the corresponding rows
// should be collapsed.
- this.state = { collapsedRows: {}, collapsedCols: {} };
-
+ this.state = { collapsedRows: {}, collapsedCols: {}, sortingOrder: [] };
+ this.sortCache = new Map();
Review Comment:
### Unbounded cache growth in sortCache Map <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sortCache Map is initialized in the constructor but never cleared,
leading to unbounded memory growth as different sort operations accumulate
cached results.
###### Why this matters
Over time, as users perform various sorting operations, the cache will
continue to grow without any cleanup mechanism, potentially causing memory
leaks in long-running applications.
###### Suggested change ā *Feature Preview*
Implement cache size limits and/or cleanup mechanisms. Consider using a LRU
cache with a maximum size, or clear the cache when component unmounts or when
props change significantly:
```javascript
componentWillUnmount() {
this.sortCache.clear();
}
// Or implement LRU with size limit
if (this.sortCache.size > MAX_CACHE_SIZE) {
const firstKey = this.sortCache.keys().next().value;
this.sortCache.delete(firstKey);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83e387df-02e2-4d18-86d7-d3da65da588d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83e387df-02e2-4d18-86d7-d3da65da588d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83e387df-02e2-4d18-86d7-d3da65da588d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83e387df-02e2-4d18-86d7-d3da65da588d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83e387df-02e2-4d18-86d7-d3da65da588d)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a6782fd7-3a31-4496-9a01-d0607378c2d3 -->
[](a6782fd7-3a31-4496-9a01-d0607378c2d3)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +418,86 @@ export class TableRenderer extends Component {
return spans;
}
+ calculateGroups(pivotData, visibleColKey, columnIndex) {
+ const groups = {};
+ const rows = pivotData.rowKeys;
+ rows.forEach(rowKey => {
+ let current = groups;
+ let sumGroup = 0;
+ rowKey.forEach(key => {
+ if (!current[key]) {
+ current[key] = { currentVal: 0 };
+ }
+ sumGroup += pivotData
+ .getAggregator(rowKey, visibleColKey[columnIndex])
+ .value();
+ current[key].currentVal = sumGroup;
+ current = current[key];
+ });
+ });
+ return groups;
+ }
+
+ sortAndCacheData(groups, sortOrder, subtotals, maxRowIndex) {
+ const { rowEnabled, rowPartialOnTop } = subtotals;
+ const sortedGroups = sortHierarchicalObject(
+ groups,
+ sortOrder,
+ rowPartialOnTop,
+ );
+ return convertToArray(
+ sortedGroups,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ );
+ }
+
+ sortData(columnIndex, visibleColKeys, pivotData, maxRowIndex) {
+ this.setState(state => {
+ const { sortingOrder, activeSortColumn } = state;
+
+ const newSortingOrder = [];
+ let newDirection = 'asc';
+
+ if (activeSortColumn === columnIndex) {
+ newDirection = sortingOrder[columnIndex] === 'asc' ? 'desc' : 'asc';
+ }
+
+ newSortingOrder[columnIndex] = newDirection;
+
+ const cacheKey = `${columnIndex}-${newDirection}`;
+ let newRowKeys;
+ if (this.sortCache.has(cacheKey)) {
+ const cachedRowKeys = this.sortCache.get(cacheKey);
+ newRowKeys = cachedRowKeys;
+ } else {
+ const groups = this.calculateGroups(
+ pivotData,
+ visibleColKeys,
+ columnIndex,
+ );
Review Comment:
### Parameter name mismatch in calculateGroups call <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The calculateGroups method is called with visibleColKeys but uses
visibleColKey[columnIndex] internally, which assumes visibleColKeys is the
parameter name. However, the parameter is actually visibleColKey (singular),
creating a potential undefined access.
###### Why this matters
This mismatch between parameter names could cause runtime errors when trying
to access the column data for sorting calculations, leading to incorrect
sorting or application crashes.
###### Suggested change ā *Feature Preview*
Fix the parameter name consistency:
```jsx
calculateGroups(pivotData, visibleColKeys, columnIndex) {
// Method body uses visibleColKeys[columnIndex] correctly
}
```
Or update the method call to match the parameter name in the method
definition.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3084a096-ee53-4076-abf1-22dc53fec9df/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3084a096-ee53-4076-abf1-22dc53fec9df?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3084a096-ee53-4076-abf1-22dc53fec9df?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3084a096-ee53-4076-abf1-22dc53fec9df?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3084a096-ee53-4076-abf1-22dc53fec9df)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a03b25d0-7a2b-4e14-8275-bfa39576745a -->
[](a03b25d0-7a2b-4e14-8275-bfa39576745a)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -71,15 +74,82 @@ function displayHeaderCell(
);
}
+function sortHierarchicalObject(obj, objSort, rowPartialOnTop) {
+ const sortedKeys = Object.keys(obj).sort((a, b) => {
+ const valA = obj[a].currentVal || 0;
+ const valB = obj[b].currentVal || 0;
+ if (rowPartialOnTop) {
+ if (obj[a].currentVal !== undefined && obj[b].currentVal === undefined) {
+ return -1;
+ }
+ if (obj[b].currentVal !== undefined && obj[a].currentVal === undefined) {
+ return 1;
+ }
+ }
+ return objSort === 'asc' ? valA - valB : valB - valA;
+ });
+ return sortedKeys.reduce((acc, key) => {
+ acc[key] =
+ typeof obj[key] === 'object' && !Array.isArray(obj[key])
+ ? sortHierarchicalObject(obj[key], objSort, rowPartialOnTop)
+ : obj[key];
+ return acc;
+ }, {});
+}
Review Comment:
### Violation of Single Responsibility Principle in Sorting Function
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sorting function handles both sorting logic and data transformation in a
single function, violating the Single Responsibility Principle.
###### Why this matters
Mixing sorting and transformation logic makes the code harder to maintain,
test, and reuse. Future modifications to either sorting or transformation logic
will require changing the same function.
###### Suggested change ā *Feature Preview*
Split into two separate functions:
```javascript
function sortKeys(obj, objSort, rowPartialOnTop) {
return Object.keys(obj).sort((a, b) => {
// sorting logic here
});
}
function transformHierarchicalObject(obj, sortedKeys, objSort,
rowPartialOnTop) {
// transformation logic here
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a09af85f-4925-4a11-9670-7858fa0cc289/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a09af85f-4925-4a11-9670-7858fa0cc289?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a09af85f-4925-4a11-9670-7858fa0cc289?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a09af85f-4925-4a11-9670-7858fa0cc289?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a09af85f-4925-4a11-9670-7858fa0cc289)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cc0e138a-1147-427a-9c13-c55749d34afe -->
[](cc0e138a-1147-427a-9c13-c55749d34afe)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +418,86 @@ export class TableRenderer extends Component {
return spans;
}
+ calculateGroups(pivotData, visibleColKey, columnIndex) {
+ const groups = {};
+ const rows = pivotData.rowKeys;
+ rows.forEach(rowKey => {
+ let current = groups;
+ let sumGroup = 0;
+ rowKey.forEach(key => {
+ if (!current[key]) {
+ current[key] = { currentVal: 0 };
+ }
+ sumGroup += pivotData
+ .getAggregator(rowKey, visibleColKey[columnIndex])
+ .value();
+ current[key].currentVal = sumGroup;
+ current = current[key];
+ });
+ });
+ return groups;
+ }
+
+ sortAndCacheData(groups, sortOrder, subtotals, maxRowIndex) {
+ const { rowEnabled, rowPartialOnTop } = subtotals;
+ const sortedGroups = sortHierarchicalObject(
+ groups,
+ sortOrder,
+ rowPartialOnTop,
+ );
+ return convertToArray(
+ sortedGroups,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ );
+ }
+
+ sortData(columnIndex, visibleColKeys, pivotData, maxRowIndex) {
+ this.setState(state => {
+ const { sortingOrder, activeSortColumn } = state;
+
+ const newSortingOrder = [];
+ let newDirection = 'asc';
+
+ if (activeSortColumn === columnIndex) {
+ newDirection = sortingOrder[columnIndex] === 'asc' ? 'desc' : 'asc';
+ }
+
+ newSortingOrder[columnIndex] = newDirection;
Review Comment:
### Inefficient sorting state management loses previous column sorts
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sortData method creates a new empty array for newSortingOrder and only
sets one index, losing all previous sorting state for other columns
unnecessarily.
###### Why this matters
This approach discards sorting information for other columns, forcing
recalculation of sorts that were already performed, and wastes the cached sort
results.
###### Suggested change ā *Feature Preview*
Preserve existing sorting state and only update the specific column:
```javascript
const newSortingOrder = [...sortingOrder];
let newDirection = 'asc';
if (activeSortColumn === columnIndex) {
newDirection = sortingOrder[columnIndex] === 'asc' ? 'desc' : 'asc';
}
newSortingOrder[columnIndex] = newDirection;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c51cd50b-f430-4c90-81cc-bbd7587d0a9a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c51cd50b-f430-4c90-81cc-bbd7587d0a9a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c51cd50b-f430-4c90-81cc-bbd7587d0a9a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c51cd50b-f430-4c90-81cc-bbd7587d0a9a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c51cd50b-f430-4c90-81cc-bbd7587d0a9a)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f7158643-7a93-4c18-a038-7d692dde9dfd -->
[](f7158643-7a93-4c18-a038-7d692dde9dfd)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +418,86 @@ export class TableRenderer extends Component {
return spans;
}
+ calculateGroups(pivotData, visibleColKey, columnIndex) {
+ const groups = {};
+ const rows = pivotData.rowKeys;
+ rows.forEach(rowKey => {
+ let current = groups;
+ let sumGroup = 0;
+ rowKey.forEach(key => {
+ if (!current[key]) {
+ current[key] = { currentVal: 0 };
+ }
+ sumGroup += pivotData
+ .getAggregator(rowKey, visibleColKey[columnIndex])
+ .value();
+ current[key].currentVal = sumGroup;
+ current = current[key];
+ });
+ });
Review Comment:
### Inefficient O(nĀ²) aggregator calculations in calculateGroups
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The calculateGroups method has O(nĀ²) time complexity due to calling
getAggregator for every row-key combination in nested loops, causing redundant
aggregator value calculations.
###### Why this matters
This nested iteration with expensive aggregator calls will cause significant
performance degradation as the dataset size increases, especially with deep
hierarchical row structures.
###### Suggested change ā *Feature Preview*
Cache aggregator values or restructure to avoid redundant calculations.
Calculate the aggregator value once per rowKey and reuse it:
```javascript
rows.forEach(rowKey => {
const aggValue = pivotData.getAggregator(rowKey,
visibleColKey[columnIndex]).value();
let current = groups;
let sumGroup = 0;
rowKey.forEach(key => {
if (!current[key]) {
current[key] = { currentVal: 0 };
}
sumGroup += aggValue;
current[key].currentVal = sumGroup;
current = current[key];
});
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c7e8c3c-9ab9-4883-88f5-3afa7dd8d08d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c7e8c3c-9ab9-4883-88f5-3afa7dd8d08d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c7e8c3c-9ab9-4883-88f5-3afa7dd8d08d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c7e8c3c-9ab9-4883-88f5-3afa7dd8d08d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c7e8c3c-9ab9-4883-88f5-3afa7dd8d08d)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:38e454c7-87eb-4fa5-8def-7ab68ec6ae1b -->
[](38e454c7-87eb-4fa5-8def-7ab68ec6ae1b)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -71,15 +74,82 @@ function displayHeaderCell(
);
}
+function sortHierarchicalObject(obj, objSort, rowPartialOnTop) {
+ const sortedKeys = Object.keys(obj).sort((a, b) => {
+ const valA = obj[a].currentVal || 0;
+ const valB = obj[b].currentVal || 0;
+ if (rowPartialOnTop) {
+ if (obj[a].currentVal !== undefined && obj[b].currentVal === undefined) {
+ return -1;
+ }
+ if (obj[b].currentVal !== undefined && obj[a].currentVal === undefined) {
+ return 1;
+ }
+ }
+ return objSort === 'asc' ? valA - valB : valB - valA;
+ });
+ return sortedKeys.reduce((acc, key) => {
+ acc[key] =
+ typeof obj[key] === 'object' && !Array.isArray(obj[key])
+ ? sortHierarchicalObject(obj[key], objSort, rowPartialOnTop)
+ : obj[key];
+ return acc;
+ }, {});
+}
+
+function convertToArray(
+ obj,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ parentKeys = [],
+ result = [],
+ flag = false,
+) {
+ let updatedFlag = flag;
+ Object.keys(obj).forEach(key => {
+ if (key === 'currentVal') {
+ return;
+ }
+ if (rowEnabled && rowPartialOnTop && parentKeys.length < maxRowIndex - 1) {
+ result.push(parentKeys.length > 0 ? [...parentKeys, key] : [key]);
+ updatedFlag = true;
+ }
+ if (typeof obj[key] === 'object' && !Array.isArray(obj[key])) {
+ convertToArray(
+ obj[key],
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ [...parentKeys, key],
+ result,
+ );
+ }
Review Comment:
### Excessive array copying in convertToArray recursion <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The convertToArray function creates new arrays with spread operator
[...parentKeys, key] on every recursive call, leading to excessive memory
allocations and O(nĀ²) space complexity.
###### Why this matters
Each recursive call creates new array copies, resulting in significant
memory overhead and garbage collection pressure for deep hierarchical
structures.
###### Suggested change ā *Feature Preview*
Use array mutation instead of creating new arrays, and restore state after
recursion:
```javascript
parentKeys.push(key);
convertToArray(
obj[key],
rowEnabled,
rowPartialOnTop,
maxRowIndex,
parentKeys,
result,
);
parentKeys.pop();
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/621c072f-0c82-4a2c-99ad-e4a1c95d3df6/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/621c072f-0c82-4a2c-99ad-e4a1c95d3df6?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/621c072f-0c82-4a2c-99ad-e4a1c95d3df6?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/621c072f-0c82-4a2c-99ad-e4a1c95d3df6?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/621c072f-0c82-4a2c-99ad-e4a1c95d3df6)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:066fd9ff-7731-4612-bfd4-5f4ad019d8e8 -->
[](066fd9ff-7731-4612-bfd4-5f4ad019d8e8)
--
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]
