codeant-ai-for-open-source[bot] commented on code in PR #37396:
URL: https://github.com/apache/superset/pull/37396#discussion_r3481281749
##########
superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx:
##########
@@ -17,61 +17,38 @@
* under the License.
*/
import { useMemo, useCallback, useRef, useState } from 'react';
-import {
- getTimeFormatter,
- safeHtmlSpan,
- TimeFormats,
- getMetricLabel,
- QueryFormMetric,
-} from '@superset-ui/core';
-import { t } from '@apache-superset/core/translation';
+import { getTimeFormatter, safeHtmlSpan, TimeFormats } from
'@superset-ui/core';
import { Constants } from '@superset-ui/core/components';
import { GenericDataType } from '@apache-superset/core/common';
import type { IRowNode } from 'ag-grid-community';
const timeFormatter = getTimeFormatter(TimeFormats.DATABASE_DATETIME);
-const CONTRIBUTION_SUFFIX = '__contribution';
+/**
+ * Builds Grid column definitions from query result metadata.
+ * Assumes {@link colnames}, {@link coltypes} and {@link collabels}
+ * have the same length and align. Only columns present in the first
+ * data row are included.
+ */
export function useGridColumns(
colnames: string[] | undefined,
coltypes: GenericDataType[] | undefined,
+ collabels: string[] | undefined,
data: Record<string, any>[] | undefined,
- columnDisplayNames?: Record<string, string>,
) {
return useMemo(
() =>
colnames && data?.length
? colnames
- .filter((column: string) => Object.keys(data[0]).includes(column))
- .map((key, index) => {
- const colType = coltypes?.[index];
-
- const rawHeader = columnDisplayNames?.[key] ?? key;
- let cleaned = rawHeader;
- let suffix = '';
-
- if (rawHeader.endsWith(CONTRIBUTION_SUFFIX)) {
- cleaned = rawHeader.slice(
- 0,
- rawHeader.length - CONTRIBUTION_SUFFIX.length,
- );
- suffix = ` (${t('contribution')})`;
- }
-
- try {
- const parsed = JSON.parse(cleaned);
- if (parsed && typeof parsed === 'object') {
- cleaned = getMetricLabel(parsed as QueryFormMetric);
- }
- } catch {
- /* not a JSON-encoded metric โ keep original display name */
- }
-
- const cleanHeader = `${cleaned}${suffix}`;
+ .map((column, originalIndex) => [column, originalIndex] as const)
+ .filter(([column]) => Object.keys(data[0]).includes(column))
Review Comment:
**Suggestion:** `Object.keys(data[0])` is recomputed for every column during
filtering, creating avoidable O(nยฒ) work for wide result sets. Precompute the
first-row key set once before filtering and check membership against that
cached set to avoid repeated allocations and scans. [performance]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Extra key scans during Explore results grid initialization.
- โ ๏ธ Minor slowdown for wide tables with many columns.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In Explore's Samples or Results pane, a query is executed so `SamplesPane`
(`superset-frontend/src/explore/components/DataTablesPane/components/SamplesPane.tsx`,
lines 89-120) or `useResultsPane`
(`superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx`,
lines 175-190) populates `colnames` and `data` and then calls
`useGridColumns(colnames,
coltypes, collabels, data)` (SamplesPane line 133, SingleQueryResultPane
line 67).
2. Inside `useGridColumns`
(`superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx`),
the column names are processed starting at line 42: `colnames &&
data?.length ?
colnames.map(...).filter(([column]) =>
Object.keys(data[0]).includes(column))...`.
3. For each entry in `colnames`, the filter line
`Object.keys(data[0]).includes(column)`
at line 44 allocates a fresh array of keys from `data[0]` and scans it
linearly with
`.includes(column)`, repeating this work once per column.
4. On every query rendering that uses `GridTable` with these columns (see
`GridTable` at
`superset-frontend/src/components/GridTable/index.tsx`, where `columns` are
mapped into
ag-grid `columnDefs` at lines 104-120), wide result sets with many columns
incur repeated
`Object.keys(data[0])` allocations and scans, introducing avoidable O(nยฒ)
overhead in
column construction before the grid is displayed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=910d0c6cf7134022a03448109e68ff7a&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=910d0c6cf7134022a03448109e68ff7a&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/src/explore/components/DataTablesPane/components/useGridResultTable.tsx
**Line:** 44:44
**Comment:**
*Performance: `Object.keys(data[0])` is recomputed for every column
during filtering, creating avoidable O(nยฒ) work for wide result sets.
Precompute the first-row key set once before filtering and check membership
against that cached set to avoid repeated allocations and scans.
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%2F37396&comment_hash=97a888d16f2c93201bb3445dfbcbc4577b2e3581b7a06912ba098f9ccd42a09c&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37396&comment_hash=97a888d16f2c93201bb3445dfbcbc4577b2e3581b7a06912ba098f9ccd42a09c&reaction=dislike'>๐</a>
##########
superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx:
##########
@@ -17,61 +17,38 @@
* under the License.
*/
import { useMemo, useCallback, useRef, useState } from 'react';
-import {
- getTimeFormatter,
- safeHtmlSpan,
- TimeFormats,
- getMetricLabel,
- QueryFormMetric,
-} from '@superset-ui/core';
-import { t } from '@apache-superset/core/translation';
+import { getTimeFormatter, safeHtmlSpan, TimeFormats } from
'@superset-ui/core';
import { Constants } from '@superset-ui/core/components';
import { GenericDataType } from '@apache-superset/core/common';
import type { IRowNode } from 'ag-grid-community';
const timeFormatter = getTimeFormatter(TimeFormats.DATABASE_DATETIME);
-const CONTRIBUTION_SUFFIX = '__contribution';
+/**
+ * Builds Grid column definitions from query result metadata.
+ * Assumes {@link colnames}, {@link coltypes} and {@link collabels}
+ * have the same length and align. Only columns present in the first
+ * data row are included.
+ */
export function useGridColumns(
colnames: string[] | undefined,
coltypes: GenericDataType[] | undefined,
+ collabels: string[] | undefined,
data: Record<string, any>[] | undefined,
- columnDisplayNames?: Record<string, string>,
) {
return useMemo(
() =>
colnames && data?.length
? colnames
- .filter((column: string) => Object.keys(data[0]).includes(column))
- .map((key, index) => {
- const colType = coltypes?.[index];
-
- const rawHeader = columnDisplayNames?.[key] ?? key;
- let cleaned = rawHeader;
- let suffix = '';
-
- if (rawHeader.endsWith(CONTRIBUTION_SUFFIX)) {
- cleaned = rawHeader.slice(
- 0,
- rawHeader.length - CONTRIBUTION_SUFFIX.length,
- );
- suffix = ` (${t('contribution')})`;
- }
-
- try {
- const parsed = JSON.parse(cleaned);
- if (parsed && typeof parsed === 'object') {
- cleaned = getMetricLabel(parsed as QueryFormMetric);
- }
- } catch {
- /* not a JSON-encoded metric โ keep original display name */
- }
-
- const cleanHeader = `${cleaned}${suffix}`;
+ .map((column, originalIndex) => [column, originalIndex] as const)
+ .filter(([column]) => Object.keys(data[0]).includes(column))
+ .map(([key, originalIndex]) => {
+ const colType = coltypes?.[originalIndex];
+ const headerLabel = collabels?.[originalIndex];
return {
label: key,
- headerName: cleanHeader,
+ headerName: headerLabel,
Review Comment:
**Suggestion:** `headerName` is now assigned directly from `collabels`,
which can be `undefined` when labels are missing or shorter than `colnames`.
Other grid integrations (like the header menu's hidden-column list) read
`headerName` directly, so those entries become blank. Set a deterministic
fallback (`column` name) when no display label is present. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Hidden-column unhide list shows blank entries for unlabeled columns.
- โ ๏ธ Grid table headers inconsistent when label arrays are incomplete.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Run any query in Explore that returns results so `useResultsPane` in
`superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx`
(lines 175-190) receives a `QueryResultInterface` with `colnames` populated
but
`collabels` either missing or shorter than `colnames` (no validation is
performed before
passing through).
2. `useResultsPane` at lines 175-190 passes `result.colnames`,
`result.collabels`, and
`result.data` into `SingleQueryResultPane` (props `colnames`, `collabels`,
`data`) in
`superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx`
(lines 49-63).
3. Inside `SingleQueryResultPane`, `useGridColumns(colnames, coltypes,
collabels, data)`
is called at line 67, which reaches `useGridColumns` in
`superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx`
(lines 33-51); here `const headerLabel = collabels?.[originalIndex];` (line
47) can be
`undefined` for columns without labels, and `headerName: headerLabel` (line
51) sets the
column definition's `headerName` to `undefined`.
4. `GridTable` in `superset-frontend/src/components/GridTable/index.tsx`
constructs
ag-grid column definitions at lines 104-110 using `{ label, headerName, ...
}`, and
`HeaderMenu` in `superset-frontend/src/components/GridTable/HeaderMenu.tsx`
builds the
"Unhide" menu using `c.getColDef().headerName` at line 78; for columns where
`headerName`
was `undefined`, these menu entries render with blank labels, producing
unnamed columns in
the hidden-columns list and potentially in other header-driven UI.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f2df40710d1d48918377da523b5ef2f0&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=f2df40710d1d48918377da523b5ef2f0&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/src/explore/components/DataTablesPane/components/useGridResultTable.tsx
**Line:** 47:51
**Comment:**
*Api Mismatch: `headerName` is now assigned directly from `collabels`,
which can be `undefined` when labels are missing or shorter than `colnames`.
Other grid integrations (like the header menu's hidden-column list) read
`headerName` directly, so those entries become blank. Set a deterministic
fallback (`column` name) when no display label is present.
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%2F37396&comment_hash=8e534583a8c8c070c888a192023a72a0fcf6251d4007861b21a0b253c55fce99&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37396&comment_hash=8e534583a8c8c070c888a192023a72a0fcf6251d4007861b21a0b253c55fce99&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]
