korbit-ai[bot] commented on code in PR #34592:
URL: https://github.com/apache/superset/pull/34592#discussion_r2259688353
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:
##########
@@ -110,6 +109,27 @@ export default function transformProps(chartProps:
ChartProps<QueryFormData>) {
const { selectedFilters } = filterState;
const granularity = extractTimegrain(rawFormData);
+ const groupbyCombinations = buildGroupbyCombinations(
+ formData as PivotTableQueryFormData,
+ );
+
+ // in case of inconsistency it will only not display some columns
+ const queryLength = Math.min(queriesData.length, groupbyCombinations.length);
+ const data: QueryData[] = [];
+ for (let i = 0; i < queryLength; i += 1) {
+ data.push({
+ data: queriesData[i].data,
+ groupby: groupbyCombinations[i],
+ });
+ }
+
+ // main query is the query with all columns -> with the longest colnames
+ const mainQuery = queriesData.reduce((main_query, query) =>
+ query.colnames.length > main_query.colnames.length ? query : main_query,
+ );
Review Comment:
### Unsafe reduce on potentially empty array <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The reduce operation assumes queriesData array is not empty, but there's no
check for an empty array which would cause the reduce to fail.
###### Why this matters
If queriesData is empty, the reduce operation will throw a TypeError,
causing the application to crash.
###### Suggested change ∙ *Feature Preview*
Add a check for empty array and provide a default value:
```typescript
const mainQuery = queriesData.length > 0
? queriesData.reduce((main_query, query) =>
query.colnames.length > main_query.colnames.length ? query :
main_query)
: queriesData[0] || { colnames: [], coltypes: [] };
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0926366-5287-4055-be47-8816338b39a0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0926366-5287-4055-be47-8816338b39a0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0926366-5287-4055-be47-8816338b39a0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0926366-5287-4055-be47-8816338b39a0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0926366-5287-4055-be47-8816338b39a0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:926e35fd-9e49-4067-be96-c0aa9688df84 -->
[](926e35fd-9e49-4067-be96-c0aa9688df84)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.js:
##########
@@ -602,6 +269,23 @@ const derivers = {
// can be used in objects.
const flatKey = attrVals => attrVals.join(String.fromCharCode(0));
+const cellValue = (formatter = usFmt) =>
+ function ([attr]) {
+ return function () {
+ return {
+ val: 0,
+ push(record) {
+ this.val = record[attr];
+ },
+ value() {
+ return this.val;
+ },
+ format: fmtNonString(formatter),
+ numInputs: typeof attr !== 'undefined' ? 0 : 1,
+ };
+ };
+ };
Review Comment:
### Excessive Function Nesting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The cellValue function employs nested function returns (triple nesting)
which makes the code harder to understand and maintain.
###### Why this matters
Deep function nesting increases cognitive complexity and makes it difficult
to trace the execution flow and debug issues.
###### Suggested change ∙ *Feature Preview*
Flatten the function structure by using a class or simpler function
composition:
```javascript
class CellValueAggregator {
constructor(formatter = usFmt, attr) {
this.formatter = formatter;
this.attr = attr;
this.val = 0;
}
push(record) {
this.val = record[this.attr];
}
value() {
return this.val;
}
}
const cellValue = (formatter = usFmt) => (attr) => new
CellValueAggregator(formatter, attr);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/de11d397-2cf4-446f-92ef-03393aa0b8f7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/de11d397-2cf4-446f-92ef-03393aa0b8f7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/de11d397-2cf4-446f-92ef-03393aa0b8f7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/de11d397-2cf4-446f-92ef-03393aa0b8f7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/de11d397-2cf4-446f-92ef-03393aa0b8f7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:328af6c7-38cc-452a-bd39-b442c6438111 -->
[](328af6c7-38cc-452a-bd39-b442c6438111)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts:
##########
@@ -0,0 +1,53 @@
+/**
+ * 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 { MetricsLayoutEnum, PivotTableQueryFormData } from '../types';
+
+export default function buildGroupbyCombinations(
+ formData: PivotTableQueryFormData,
+) {
+ let columns = formData.groupbyColumns;
+ let rows = formData.groupbyRows;
+
+ [rows, columns] = formData.transposePivot ? [columns, rows] : [rows,
columns];
+
+ const rows_combinations = [[], ...rows.map((_, i) => rows.slice(0, i + 1))];
+ const cols_combinations = [
+ [],
+ ...columns.map((_, i) => columns.slice(0, i + 1)),
+ ];
+
+ let groupbyCombinations = rows_combinations.flatMap(row =>
+ cols_combinations.map(col => ({ rows: row, columns: col })),
+ );
Review Comment:
### Excessive Intermediate Array Creation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using flatMap and map together creates intermediate arrays and objects for
each combination, leading to unnecessary memory allocations.
###### Why this matters
The nested mapping operations create temporary arrays and objects that need
to be garbage collected, impacting performance with large datasets.
###### Suggested change ∙ *Feature Preview*
Combine the combination generation into a single loop that builds the final
array directly, avoiding intermediate collections:
```typescript
const groupbyCombinations = [];
for (let i = 0; i <= rows.length; i++) {
for (let j = 0; j <= columns.length; j++) {
groupbyCombinations.push({
rows: i === 0 ? [] : rows.slice(0, i),
columns: j === 0 ? [] : columns.slice(0, j)
});
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/adeb80a5-6e7d-4b76-ad6e-7b0bb57188c4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/adeb80a5-6e7d-4b76-ad6e-7b0bb57188c4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/adeb80a5-6e7d-4b76-ad6e-7b0bb57188c4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/adeb80a5-6e7d-4b76-ad6e-7b0bb57188c4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/adeb80a5-6e7d-4b76-ad6e-7b0bb57188c4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:650f04d6-7727-41f5-b2c6-f6dcdfb22bb7 -->
[](650f04d6-7727-41f5-b2c6-f6dcdfb22bb7)
--
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]
