Skip to content

fix(SQL Lab): handle columns without names#38986

Merged
sfirke merged 4 commits intoapache:masterfrom
sfirke:fix-23848
Apr 6, 2026
Merged

fix(SQL Lab): handle columns without names#38986
sfirke merged 4 commits intoapache:masterfrom
sfirke:fix-23848

Conversation

@sfirke
Copy link
Copy Markdown
Member

@sfirke sfirke commented Mar 31, 2026

User description

SUMMARY

Fix #23848, a three-year-old bug affecting Microsoft SQL Server.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE - 6.0.0

image

AFTER

Note that it avoids the collision with my first column explicitly named _col_1:

image

TESTING INSTRUCTIONS

Run a query like SELECT COUNT(*) FROM my_table on a DB that returns (No column name) and see that it no longer errors.

ADDITIONAL INFORMATION

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Mar 31, 2026

Code Review Agent Run #fc8209

Actionable Suggestions - 0
Additional Suggestions - 10
  • superset/mcp_service/chart/schemas.py - 1
    • Inconsistent field description · Line 876-882
      The description for the 'metric' field states it 'Must include an aggregate function (e.g., SUM, COUNT)', but the validation logic in `validate_metric_aggregate` allows saved metrics as well. This inconsistency could confuse users. Consider updating the description to: 'The metric to display as a big number. Must include an aggregate function (e.g., SUM, COUNT) or reference a saved metric from the dataset.'
  • superset/mcp_service/chart/tool/list_charts.py - 1
    • Inconsistent default columns · Line 50-50
      Adding "changed_on" to DEFAULT_CHART_COLUMNS creates inconsistency with CHART_DEFAULT_COLUMNS in schema_discovery.py, which still only includes "changed_on_humanized". For consistency with dataset and dashboard tools that include both raw and humanized timestamps in their defaults, update the schema discovery defaults as well.
  • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/SmoothLine/controlPanel.test.ts - 1
    • Incomplete visibility test coverage · Line 72-101
      The test suite includes visibility assertions for x_axis_number_format but omits them for x_axis_time_format, even though the controlPanel defines visibility logic for both. This gap could allow visibility bugs to go undetected.
      Code suggestion
       @@ -101,0 +102,12 @@
       test('x_axis_time_format should be visible for temporal columns', () => {
         const visibilityFn = timeFormatControl?.config?.visibility;
         expect(visibilityFn(mockControls('date', GenericDataType.Temporal))).toBe(
           true,
         );
       });
      
       test('x_axis_time_format should be hidden for numeric columns', () => {
         const visibilityFn = timeFormatControl?.config?.visibility;
         expect(visibilityFn(mockControls('year', GenericDataType.Numeric))).toBe(
           false,
         );
       });
      
       test('x_axis_time_format should be hidden for string columns', () => {
         const visibilityFn = timeFormatControl?.config?.visibility;
         expect(visibilityFn(mockControls('name', GenericDataType.String))).toBe(
           false,
         );
       });
  • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Step/controlPanel.test.ts - 1
    • Add explicit return type · Line 25-25
      The `getControl` function lacks an explicit return type annotation, which goes against the project's coding standards for TypeScript. Adding a return type improves code clarity and enables better type checking.
  • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts - 1
    • Use named constants for boundaries · Line 1403-1403
      The boundary test hardcodes height 100, which should use TIMESERIES_CONSTANTS.compactChartHeight for maintainability if the constant changes.
  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx - 1
    • Missing test case for ISO timestamp in row header · Line 1130-1133
      The row header date formatter test is missing the ISO timestamp case that exists in the column header test. Since both use identical date formatting logic, the row test should include the same test cases for complete coverage.
      Code suggestion
       @@ -1130,4 +1130,5 @@
      - test.each([
      -   ['numeric timestamp string', '1700000000000', 1700000000000],
      -   ['non-numeric date string', 'Dec. 16 2020', 'Dec. 16 2020'],
      - ])(
      + test.each([
      +   ['numeric timestamp string', '1700000000000', 1700000000000],
      +   ['non-numeric date string', 'Dec. 16 2020', 'Dec. 16 2020'],
      +   ['ISO timestamp string', '2024-01-15T00:00:00Z', '2024-01-15T00:00:00Z'],
      + ])(
  • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Line/controlPanel.test.ts - 1
    • TypeScript Type Safety · Line 25-70
      The test uses 'any' types for control variables and lacks a return type on getControl, which violates the codebase's TypeScript standards requiring proper types instead of 'any'. While the test functions correctly, updating to ControlSetItem | null improves type safety and consistency.
      Code suggestion
       @@ -19,1 +19,1 @@
      - import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
      + import { ControlPanelsContainerProps, ControlSetItem } from '@superset-ui/chart-controls/types';
       @@ -25,1 +25,1 @@
      - const getControl = (controlName: string) => {
      + const getControl = (controlName: string): ControlSetItem | null => {
       @@ -69,1 +69,1 @@
      - const timeFormatControl: any = getControl('x_axis_time_format');
      + const timeFormatControl: ControlSetItem | null = getControl('x_axis_time_format');
       @@ -70,1 +70,1 @@
      - const numberFormatControl: any = getControl('x_axis_number_format');
      + const numberFormatControl: ControlSetItem | null = getControl('x_axis_number_format');
  • tests/unit_tests/mcp_service/chart/test_big_number_chart.py - 2
    • Strengthen test assertion · Line 386-386
      The test assertion checks for 'metric' substring in primary_insight, but exact matching ensures the function returns the expected insight string, reducing false positives.
      Code suggestion
       @@ -386,1 +386,1 @@
      -        assert "metric" in result.primary_insight.lower()
      +        assert result.primary_insight == "Highlights a single key metric value as a prominent number"
    • Strengthen test assertion · Line 399-399
      The test assertion checks for 'trend' substring in primary_insight, but exact matching ensures the function returns the expected insight string, reducing false positives.
      Code suggestion
       @@ -399,1 +399,1 @@
      -        assert "trend" in result.primary_insight.lower()
      +        assert result.primary_insight == "Displays a key metric with a trendline showing how the value changes over time"
  • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts - 1
    • Replace 'any' types and add return hints · Line 25-25
      The test file uses 'any' types and lacks explicit return type hints, violating project standards that prohibit 'any' and require type annotations for consistency and static checking.
      Code suggestion
       @@ -19,1 +19,1 @@
      - import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
      + import { ControlPanelsContainerProps, CustomControlItem } from '@superset-ui/chart-controls/types';
       @@ -25,1 +25,1 @@
      - const getControl = (controlName: string) => {
      + const getControl = (controlName: string): CustomControlItem | null => {
       @@ -69,1 +69,1 @@
      - const timeFormatControl: any = getControl('x_axis_time_format');
      + const timeFormatControl: CustomControlItem | null = getControl('x_axis_time_format');
       @@ -70,1 +70,1 @@
      - const numberFormatControl: any = getControl('x_axis_number_format');
      + const numberFormatControl: CustomControlItem | null = getControl('x_axis_number_format');
Review Details
  • Files reviewed - 45 · Commit Range: 07e96bb..c615331
    • docs/yarn.lock
    • superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/constants.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Line/controlPanel.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/SmoothLine/controlPanel.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Step/controlPanel.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
    • superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
    • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx
    • superset-frontend/src/components/AlteredSliceTag/utils/index.ts
    • superset-frontend/src/components/AlteredSliceTag/utils/utils.test.ts
    • superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.test.tsx
    • superset-frontend/src/core/sqlLab/sqlLab.test.ts
    • superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
    • superset-frontend/src/explore/components/ControlPanelsContainer.test.tsx
    • superset/mcp_service/app.py
    • superset/mcp_service/chart/chart_utils.py
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/schemas.py
    • superset/mcp_service/chart/tool/generate_chart.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • superset/mcp_service/chart/tool/list_charts.py
    • superset/mcp_service/chart/validation/schema_validator.py
    • superset/mcp_service/dashboard/schemas.py
    • superset/mcp_service/dashboard/tool/list_dashboards.py
    • superset/mcp_service/dataset/schemas.py
    • superset/mcp_service/dataset/tool/list_datasets.py
    • superset/mcp_service/sql_lab/tool/execute_sql.py
    • superset/result_set.py
    • tests/unit_tests/mcp_service/chart/test_big_number_chart.py
    • tests/unit_tests/mcp_service/chart/test_chart_utils.py
    • tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py
    • tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py
    • tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
    • tests/unit_tests/result_set_test.py
  • Files skipped - 4
    • .github/workflows/ephemeral-env.yml - Reason: Filter setting
    • UPDATING.md - Reason: Filter setting
    • docs/package.json - Reason: Filter setting
    • superset-websocket/utils/client-ws-app/package-lock.json - Reason: Filter setting
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot Bot added the sqllab Namespace | Anything related to the SQL Lab label Mar 31, 2026
@codeant-ai-for-open-source codeant-ai-for-open-source Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Mar 31, 2026
SQL Server returns an empty-string column name in cursor.description for
any un-aliased expression (e.g. SELECT COUNT(*), SELECT DATEPART(...)).
An empty field name is illegal in NumPy structured arrays and PyArrow
tables, so SupersetResultSet raised "ValueError: no field of name",
surfacing as a GENERIC_DB_ENGINE_ERROR in SQL Lab and in the adhoc
column type-probe path.

Fix: replace empty column names with synthetic names (_col_0, _col_1,
...) during cursor.description parsing in SupersetResultSet.__init__.
This covers all code paths that use SupersetResultSet – SQL Lab,
Database.load_into_dataframe(), SQLExecutor, and get_columns_description
(adhoc column type probing) – in one place.

Fixes apache#23848
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 31, 2026

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit c615331
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/69cc08eb1d31db0008b6c50a
😎 Deploy Preview https://deploy-preview-38986--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.41%. Comparing base (25eea29) to head (f54ece6).
⚠️ Report is 27 commits behind head on master.

Files with missing lines Patch % Lines
superset/result_set.py 55.55% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #38986      +/-   ##
==========================================
- Coverage   64.41%   64.41%   -0.01%     
==========================================
  Files        2536     2536              
  Lines      131155   131172      +17     
  Branches    30450    30453       +3     
==========================================
+ Hits        84485    84494       +9     
- Misses      45207    45214       +7     
- Partials     1463     1464       +1     
Flag Coverage Δ
hive 40.07% <55.55%> (+<0.01%) ⬆️
mysql 60.91% <55.55%> (-0.01%) ⬇️
postgres 60.99% <55.55%> (-0.01%) ⬇️
presto 40.09% <55.55%> (+<0.01%) ⬆️
python 62.58% <55.55%> (-0.01%) ⬇️
sqlite 60.61% <55.55%> (-0.01%) ⬇️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sfirke sfirke removed the size:XXL This PR changes 1000+ lines, ignoring generated files label Mar 31, 2026
@sfirke sfirke requested review from justinpark and removed request for dpgaspar, eschutho, geido, kgabryje, michael-s-molina, nytai and villebro March 31, 2026 18:14
@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Mar 31, 2026

Code Review Agent Run #f92481

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 198c919..198c919
    • superset/result_set.py
    • tests/unit_tests/result_set_test.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a SQL Lab result set bug (notably impacting Microsoft SQL Server) where unnamed columns in cursor.description could cause failures when building NumPy/PyArrow-backed result tables.

Changes:

  • Substitute synthetic column names (_col_<index>) when cursor.description provides an empty column name.
  • Add unit tests ensuring empty (and multiple empty) column names are replaced and remain usable in Pandas conversion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
superset/result_set.py Generates synthetic names for empty column names before creating NumPy/PyArrow tables.
tests/unit_tests/result_set_test.py Adds regression tests covering single and multiple empty column names.

Comment thread superset/result_set.py Outdated
Comment thread tests/unit_tests/result_set_test.py
Comment thread tests/unit_tests/result_set_test.py
Copy link
Copy Markdown
Member

@sadpandajoe sadpandajoe left a comment

Choose a reason for hiding this comment

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

LGTM

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Apr 2, 2026
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 2, 2026

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 123bf08
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/69ce989ab578400008a5f144
😎 Deploy Preview https://deploy-preview-38986--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 2, 2026

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit f54ece6
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/69cea34ea0aaf40008b5862d
😎 Deploy Preview https://deploy-preview-38986--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@sfirke
Copy link
Copy Markdown
Member Author

sfirke commented Apr 2, 2026

@michael-s-molina I completed that suggestion from Copilot and updated my "AFTER" screenshot to reflect that new behavior

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Apr 2, 2026

Code Review Agent Run #5f4c7e

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 198c919..f54ece6
    • superset/result_set.py
    • tests/unit_tests/result_set_test.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@sfirke
Copy link
Copy Markdown
Member Author

sfirke commented Apr 3, 2026

Alternatively, if the longer fix resulting from the reviewer feedback is too clunky or introduces performance concerns, a compromise would be to make the replacement names longer & odder to lessen the chance of collisions. E.g., instead of _col_0 the first unnamed column could become __unnamed_col_0. I'm fine either way.

@michael-s-molina
Copy link
Copy Markdown
Member

michael-s-molina commented Apr 6, 2026

a compromise would be to make the replacement names longer & odder to lessen the chance of collisions

The current approach is safer and the limited number of columns should not cause performance problems.

@sfirke sfirke merged commit 12eb40d into apache:master Apr 6, 2026
65 checks passed
@sfirke
Copy link
Copy Markdown
Member Author

sfirke commented Apr 6, 2026

Thanks for the careful reviews @michael-s-molina !

@sfirke sfirke deleted the fix-23848 branch April 6, 2026 14:09
michael-s-molina pushed a commit that referenced this pull request Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L sqllab Namespace | Anything related to the SQL Lab

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Creation of a dimension using SQL Server data warehouse fails with 'no field of name'

4 participants