Skip to content

feat(explore): add interactive table query building with SQL visibility #36772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
eschutho wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

feat(explore): add interactive table query building with SQL visibility #36772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76c49ef
feat(explore): add Query tab to show generated SQL in data panel
claude Dec 20, 2025
a1ea1af
feat(table): add column header context menu for interactive query bui…
claude Dec 20, 2025
4b2e6d1
feat(ag-grid): add Group By and Add Metric column interactions
claude Dec 20, 2025
ab04af5
fix(table): remove unused imports in ColumnHeaderContextMenu
claude Dec 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 125 additions & 4 deletions ...t-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@
* under the License.
*/

import { useRef, useState } from 'react';
import { t } from '@superset-ui/core';
import { ArrowDownOutlined, ArrowUpOutlined } from '@ant-design/icons';
import { useRef, useState, useCallback } from 'react';
import { t, ensureIsArray } from '@superset-ui/core';
import {
ArrowDownOutlined,
ArrowUpOutlined,
GroupOutlined,
CalculatorOutlined,
EyeInvisibleOutlined,
} from '@ant-design/icons';
import FilterIcon from './Filter';
import KebabMenu from './KebabMenu';
import {
Expand Down Expand Up @@ -61,11 +67,14 @@ const CustomHeader: React.FC<CustomHeaderParams> = ({
column,
api,
}) => {
const { initialSortState, onColumnHeaderClicked } = context;
const { initialSortState, onColumnHeaderClicked, setControlValue, formData } =
context;
const colId = column?.getColId();
const colDef = column?.getColDef() as CustomColDef;
const userColDef = column.getUserProvidedColDef() as UserProvidedColDef;
const isPercentMetric = colDef?.context?.isPercentMetric;
const isMetric = colDef?.context?.isMetric;
const isNumeric = colDef?.context?.isNumeric;

const [isFilterVisible, setFilterVisible] = useState(false);
const [isMenuVisible, setMenuVisible] = useState(false);
Expand Down Expand Up @@ -119,13 +128,58 @@ const CustomHeader: React.FC<CustomHeaderParams> = ({
setMenuVisible(!isMenuVisible);
};

// Column interaction handlers for Explore
const handleGroupBy = useCallback(() => {
if (!setControlValue || !formData) return;
const currentGroupby = ensureIsArray(formData.groupby);
if (!currentGroupby.includes(colId)) {
setControlValue('groupby', [...currentGroupby, colId]);
}
setMenuVisible(false);
}, [setControlValue, formData, colId]);

const handleAddMetric = useCallback(
(aggregate: string) => {
if (!setControlValue || !formData) return;
const currentMetrics = ensureIsArray(formData.metrics);
const metricLabel = `${aggregate}(${colId})`;
const newMetric = {
aggregate,
column: { column_name: colId },
expressionType: 'SIMPLE',
label: metricLabel,
};
setControlValue('metrics', [...currentMetrics, newMetric]);
setMenuVisible(false);
},
[setControlValue, formData, colId],
);

const handleHideColumn = useCallback(() => {
if (!setControlValue || !formData) return;
const currentColumnConfig = formData.column_config || {};
setControlValue('column_config', {
...currentColumnConfig,
[colId]: {
...currentColumnConfig[colId],
visible: false,
},
});
setMenuVisible(false);
}, [setControlValue, formData, colId]);
Comment on lines +132 to +169
Copy link
Contributor

@bito-code-review bito-code-review bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing colId validation in handlers

The column interaction handlers use colId without checking if it's defined. Since colId = column?.getColId() can be undefined, this could add undefined to groupby or create invalid metrics, corrupting form data.

Code Review Run #48c584

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them


const isCurrentColSorted = currentSort?.colId === colId;
const currentDirection = isCurrentColSorted ? currentSort?.sort : null;
const shouldShowAsc =
!isTimeComparison && (!currentDirection || currentDirection === 'desc');
const shouldShowDesc =
!isTimeComparison && (!currentDirection || currentDirection === 'asc');

// Only show column interaction options in Explore context (when setControlValue is available)
const showColumnInteractions = !!setControlValue;
const canGroupBy = showColumnInteractions && !isMetric && !isPercentMetric;
const canAddMetric = showColumnInteractions && !isMetric && !isPercentMetric;

const menuContent = (
<MenuContainer>
{shouldShowAsc && (
Expand All @@ -143,6 +197,73 @@ const CustomHeader: React.FC<CustomHeaderParams> = ({
<span style={{ fontSize: 16 }}>↻</span> {t('Clear Sort')}
</div>
)}

{/* Column interaction options for Explore */}
{canGroupBy && (
<>
<div className="menu-divider" />
<div onClick={handleGroupBy} className="menu-item">
<GroupOutlined /> {t('Group by this column')}
</div>
</>
)}
{canAddMetric && (
<>
<div className="menu-item menu-submenu">
<CalculatorOutlined /> {t('Add as metric')}
<div className="submenu">
{isNumeric && (
<>
<div
onClick={() => handleAddMetric('SUM')}
className="menu-item"
>
{t('SUM')}
</div>
<div
onClick={() => handleAddMetric('AVG')}
className="menu-item"
>
{t('AVG')}
</div>
<div
onClick={() => handleAddMetric('MIN')}
className="menu-item"
>
{t('MIN')}
</div>
<div
onClick={() => handleAddMetric('MAX')}
className="menu-item"
>
{t('MAX')}
</div>
</>
)}
<div
onClick={() => handleAddMetric('COUNT')}
className="menu-item"
>
{t('COUNT')}
</div>
<div
onClick={() => handleAddMetric('COUNT_DISTINCT')}
className="menu-item"
>
{t('COUNT DISTINCT')}
</div>
</div>
</div>
</>
)}
{showColumnInteractions && (
<>
<div className="menu-divider" />
<div onClick={handleHideColumn} className="menu-item">
<EyeInvisibleOutlined /> {t('Hide column')}
</div>
</>
)}
</MenuContainer>
);

Expand Down
9 changes: 8 additions & 1 deletion superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ import { SearchOutlined } from '@ant-design/icons';
import { debounce, isEqual } from 'lodash';
import Pagination from './components/Pagination';
import SearchSelectDropdown from './components/SearchSelectDropdown';
import { SearchOption, SortByItem } from '../types';
import { SearchOption, SortByItem, TableChartFormData } from '../types';
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The three identifiers SearchOption, SortByItem, and TableChartFormData are types; importing them as runtime values may cause bundling/runtime problems if they are exported only as types. Change this to a type-only import to ensure the import is erased during compilation. [type error]

Severity Level: Minor ⚠️

Suggested change
import { SearchOption, SortByItem, TableChartFormData } from '../types';
import type { SearchOption, SortByItem, TableChartFormData } from '../types';
Why it matters? ⭐

Those identifiers are used only as TypeScript types in this file (props and related types). Converting to import type { ... } removes unnecessary runtime imports and is the correct, idiomatic change.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 56:56
**Comment:**
	*Type Error: The three identifiers `SearchOption`, `SortByItem`, and `TableChartFormData` are types; importing them as runtime values may cause bundling/runtime problems if they are exported only as types. Change this to a type-only import to ensure the import is erased during compilation.

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.

import { HandlerFunction } from '@superset-ui/core';
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Importing HandlerFunction as a value import can cause runtime bundling/import issues if HandlerFunction is exported only as a TypeScript type (not a runtime value). Use a type-only import to ensure the import is erased at compile time and avoid potential runtime errors. [type error]

Severity Level: Minor ⚠️

Suggested change
import { HandlerFunction } from '@superset-ui/core';
import type { HandlerFunction } from '@superset-ui/core';
Why it matters? ⭐

This is a valid, useful change — HandlerFunction is only used as a prop type in this file, so using import type avoids emitting a runtime import and prevents potential bundling/runtime errors or warnings with type-only exports. It's safe and low-risk.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 57:57
**Comment:**
	*Type Error: Importing `HandlerFunction` as a value import can cause runtime bundling/import issues if `HandlerFunction` is exported only as a TypeScript type (not a runtime value). Use a type-only import to ensure the import is erased at compile time and avoid potential runtime errors.

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.

import getInitialSortState, { shouldSort } from '../utils/getInitialSortState';
import { PAGE_SIZE_OPTIONS } from '../consts';

Expand Down Expand Up @@ -102,6 +103,8 @@ export interface AgGridTableProps {
onColumnStateChange?: (state: AgGridChartStateWithMetadata) => void;
gridRef?: RefObject<AgGridReact>;
chartState?: AgGridChartState;
setControlValue?: HandlerFunction;
formData?: TableChartFormData;
}

ModuleRegistry.registerModules([AllCommunityModule, ClientSideRowModelModule]);
Expand Down Expand Up @@ -138,6 +141,8 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
width,
onColumnStateChange,
chartState,
setControlValue,
formData,
}) => {
const gridRef = useRef<AgGridReact>(null);
const inputRef = useRef<HTMLInputElement>(null);
Expand Down Expand Up @@ -520,6 +525,8 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
serverPaginationData?.sortBy || [],
),
isActiveFilterValue,
setControlValue,
formData,
}}
/>
{serverPagination && (
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ export default function TableChart<D extends DataRecord = DataRecord>(
width,
onChartStateChange,
chartState,
setControlValue,
formData,
} = props;

const [searchOptions, setSearchOptions] = useState<SearchOption[]>([]);
Expand Down Expand Up @@ -302,6 +304,8 @@ export default function TableChart<D extends DataRecord = DataRecord>(
width={width}
onColumnStateChange={handleColumnStateChange}
chartState={chartState}
setControlValue={setControlValue}
formData={formData}
/>
</StyledChartContainer>
);
Expand Down
22 changes: 22 additions & 0 deletions superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,28 @@ export const MenuContainer = styled.div`
background-color: ${theme.colorBorderSecondary};
margin: ${theme.sizeUnit}px 0;
}

.menu-submenu {
position: relative;

.submenu {
display: none;
position: absolute;
left: 100%;
top: 0;
min-width: ${theme.sizeUnit * 35}px;
background: var(--ag-menu-background-color, ${theme.colorBgBase});
border: var(--ag-menu-border, 1px solid ${theme.colorBorderSecondary});
box-shadow: var(--ag-menu-shadow, ${theme.boxShadow});
border-radius: ${theme.borderRadius}px;
padding: ${theme.sizeUnit}px 0;
z-index: 100;
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Hardcoded stacking order: the submenu uses a fixed z-index: 100 which can collide with other components; make it configurable via a CSS variable with a sensible fallback so it can be adjusted where the menu is used. [possible bug]

Severity Level: Critical 🚨

Suggested change
z-index: 100;
z-index: var(--ag-submenu-z-index, 100);
Why it matters? ⭐

Making the z-index configurable via a CSS variable (with a sensible fallback) prevents stacking collisions
with other components and is a low-risk improvement. The proposed improved code does exactly that.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx
**Line:** 157:157
**Comment:**
	*Possible Bug: Hardcoded stacking order: the submenu uses a fixed `z-index: 100` which can collide with other components; make it configurable via a CSS variable with a sensible fallback so it can be adjusted where the menu is used.

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.

}

&:hover .submenu {
display: block;
}
}
`}
`;

Expand Down
3 changes: 2 additions & 1 deletion superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ const transformProps = (
queriesData = [],
ownState: serverPaginationData,
filterState,
hooks: { setDataMask = () => {}, onChartStateChange },
hooks: { setDataMask = () => {}, onChartStateChange, setControlValue },
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Destructuring hooks directly from chartProps without a default for the parent object will throw if chartProps.hooks is undefined (TypeError: cannot destructure property of 'undefined'). Provide a default empty object for hooks in the destructuring so the nested defaults are applied safely. [null pointer]

Severity Level: Minor ⚠️

Suggested change
hooks: { setDataMask = () => {}, onChartStateChange, setControlValue },
hooks: { setDataMask = () => {}, onChartStateChange, setControlValue } = {},
Why it matters? ⭐

Accurate. If chartProps.hooks is undefined, destructuring its properties will throw at runtime. Defaulting the parent object (hooks = {}) is a small, defensive change that prevents a TypeError and is safe even if the type normally guarantees hooks exists.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 472:472
**Comment:**
	*Null Pointer: Destructuring `hooks` directly from `chartProps` without a default for the parent object will throw if `chartProps.hooks` is undefined (TypeError: cannot destructure property of 'undefined'). Provide a default empty object for `hooks` in the destructuring so the nested defaults are applied safely.

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.

emitCrossFilters,
theme,
} = chartProps;
Expand Down Expand Up @@ -739,6 +739,7 @@ const transformProps = (
formData,
chartState: serverPaginationData?.chartState,
onChartStateChange,
setControlValue,
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The returned props now include setControlValue (added to the returned object) but it may be undefined; returning an undefined value that callers will invoke can cause runtime errors. Ensure the returned property is always a function by coalescing to a noop when setControlValue is falsy. [possible bug]

Severity Level: Critical 🚨

Suggested change
setControlValue,
setControlValue: setControlValue ?? (() => {}),
Why it matters? ⭐

Valid: ensuring the returned prop is always a function (coalesce to a noop) prevents consumers from having to null-check before calling it. This duplicates the safety of providing a default during destructuring, but is a safe fallback if you can't change the destructuring.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 742:742
**Comment:**
	*Possible Bug: The returned props now include `setControlValue` (added to the returned object) but it may be undefined; returning an undefined value that callers will invoke can cause runtime errors. Ensure the returned property is always a function by coalescing to a noop when `setControlValue` is falsy.

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.

};
};

Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
IHeaderParams,
CustomCellRendererProps,
} from '@superset-ui/core/components/ThemedAgGridReact';
import { HandlerFunction } from '@superset-ui/core';
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Non-type import of HandlerFunction can emit a runtime import for a symbol that may only exist as a TypeScript type and not as a JS value; that can produce a runtime "undefined" import or bundler/runtime error. Import type-only declarations using import type to ensure no runtime import is emitted. [type error]

Severity Level: Minor ⚠️

Suggested change
import { HandlerFunction } from '@superset-ui/core';
import type { HandlerFunction } from '@superset-ui/core';
Why it matters? ⭐

HandlerFunction is only used in type positions in this file (interfaces), so switching to import type avoids emitting a runtime import for a symbol that is only a TS type. This reduces bundle ambiguity and is the correct modern TS usage.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts
**Line:** 46:46
**Comment:**
	*Type Error: Non-type import of `HandlerFunction` can emit a runtime import for a symbol that may only exist as a TypeScript type and not as a JS value; that can produce a runtime "undefined" import or bundler/runtime error. Import type-only declarations using `import type` to ensure no runtime import is emitted.

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.


export type CustomFormatter = (value: DataRecordValue) => string;

Expand Down Expand Up @@ -179,6 +180,7 @@ export interface AgGridTableChartTransformedProps<
formData: TableChartFormData;
onChartStateChange?: (chartState: JsonObject) => void;
chartState?: AgGridChartState;
setControlValue?: HandlerFunction;
}

export enum ColorSchemeEnum {
Expand All @@ -194,6 +196,8 @@ export interface SortState {
export interface CustomContext {
initialSortState: SortState[];
onColumnHeaderClicked: (args: { column: SortState }) => void;
setControlValue?: HandlerFunction;
formData?: TableChartFormData;
}

export interface CustomHeaderParams extends IHeaderParams {
Expand Down
Loading
Loading