Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export function normalizeTimeColumn(
sqlExpression: formData.x_axis,
label: formData.x_axis,
expressionType: 'SQL',
isColumnReference: true,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export interface AdhocColumn {
optionName?: string;
sqlExpression: string;
expressionType: 'SQL';
isColumnReference?: boolean;
columnType?: 'BASE_AXIS' | 'SERIES';
timeGrain?: string;
datasourceWarning?: boolean;
Expand Down Expand Up @@ -74,6 +75,10 @@ export function isAdhocColumn(column?: any): column is AdhocColumn {
);
}

export function isAdhocColumnReference(column?: any): column is AdhocColumn {
return isAdhocColumn(column) && column?.isColumnReference === true;
}

export function isQueryFormColumn(column: any): column is QueryFormColumn {
return isPhysicalColumn(column) || isAdhocColumn(column);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ test('should support different columns for x-axis and granularity', () => {
{
timeGrain: 'P1Y',
columnType: 'BASE_AXIS',
isColumnReference: true,
sqlExpression: 'time_column_in_x_axis',
label: 'time_column_in_x_axis',
expressionType: 'SQL',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,36 +101,35 @@ describe('queryObject conversion', () => {

it('should convert queryObject', () => {
const { queries } = buildQuery({ ...formData, x_axis: 'time_column' });
expect(queries[0]).toEqual(
expect.objectContaining({
granularity: 'time_column',
time_range: '1 year ago : 2013',
extras: { having: '', where: '', time_grain_sqla: 'P1Y' },
columns: [
{
columnType: 'BASE_AXIS',
expressionType: 'SQL',
label: 'time_column',
sqlExpression: 'time_column',
timeGrain: 'P1Y',
},
'col1',
],
series_columns: ['col1'],
metrics: ['count(*)'],
post_processing: [
{
operation: 'pivot',
options: {
aggregates: { 'count(*)': { operator: 'mean' } },
columns: ['col1'],
drop_missing_columns: true,
index: ['time_column'],
},
expect(queries[0]).toMatchObject({
granularity: 'time_column',
time_range: '1 year ago : 2013',
extras: { having: '', where: '', time_grain_sqla: 'P1Y' },
columns: [
{
columnType: 'BASE_AXIS',
expressionType: 'SQL',
label: 'time_column',
sqlExpression: 'time_column',
timeGrain: 'P1Y',
isColumnReference: true,
},
'col1',
],
series_columns: ['col1'],
metrics: ['count(*)'],
post_processing: [
{
operation: 'pivot',
options: {
aggregates: { 'count(*)': { operator: 'mean' } },
columns: ['col1'],
drop_missing_columns: true,
index: ['time_column'],
},
{ operation: 'flatten' },
],
}),
);
},
{ operation: 'flatten' },
],
});
});
});
12 changes: 11 additions & 1 deletion superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1502,8 +1502,18 @@ def adhoc_column_to_sqla( # pylint: disable=too-many-locals
"""
label = utils.get_column_name(col)
try:
sql_expression = col["sqlExpression"]

# For column references, conditionally quote identifiers that need it
if col.get("isColumnReference"):
# Check if already quoted to avoid double-quoting
if not (
sql_expression.startswith('"') and sql_expression.endswith('"')
):

@betodealmeida betodealmeida Nov 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unfortunately this won't work, because not all databases use " for quoting identifiers; some use backticks (MySQL), some use brackets (SQL Server).

Why would this be quoted in the first place, can't we assume it's never quoted since it's selected from a dropdown? Or is this coming from somewhere else (adhoc input)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's a good question, if the column names can't be quoted this check is pointless and i can remove it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done, removed!

sql_expression = self.database.quote_identifier(sql_expression)

expression = self._process_select_expression(
expression=col["sqlExpression"],
expression=sql_expression,
database_id=self.database_id,
engine=self.database.backend,
schema=self.schema,
Expand Down
1 change: 1 addition & 0 deletions superset/superset_typing.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class AdhocColumn(TypedDict, total=False):
hasCustomLabel: Optional[bool]
label: str
sqlExpression: str
isColumnReference: Optional[bool]
columnType: Optional[Literal["BASE_AXIS", "SERIES"]]
timeGrain: Optional[str]

Expand Down
65 changes: 65 additions & 0 deletions tests/unit_tests/models/helpers_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
from sqlalchemy.orm.session import Session
from sqlalchemy.pool import StaticPool

from superset.superset_typing import AdhocColumn

if TYPE_CHECKING:
from superset.models.core import Database

Expand Down Expand Up @@ -1125,3 +1127,66 @@ def test_process_select_expression_end_to_end(database: Database) -> None:
assert expected.replace(" ", "").lower() in result.replace(" ", "").lower(), (
f"Expected '{expected}' to be in result '{result}' for input '{expression}'"
)


def test_adhoc_column_to_sqla_with_column_reference(database: Database) -> None:
"""
Test that adhoc_column_to_sqla
properly quotes column identifiers when isColumnReference is true.

This tests the fix for column names with spaces being properly quoted
before being processed by SQLGlot to prevent "column AS alias" misinterpretation.
"""
from superset.connectors.sqla.models import SqlaTable

table = SqlaTable(
table_name="test_table",
database=database,
)

# Test 1: Column reference with spaces should be quoted
col_with_spaces: AdhocColumn = {
"sqlExpression": "Customer Name",
"label": "Customer Name",
"isColumnReference": True,
}

result = table.adhoc_column_to_sqla(col_with_spaces)

# Should contain the quoted column name
assert result is not None
result_str = str(result)

assert '"Customer Name"' in result_str


def test_adhoc_column_to_sqla_column_reference_already_quoted(
database: Database,
) -> None:
"""
Test that adhoc_column_to_sqla handles already quoted column names correctly.

When isColumnReference is true but the column is already quoted,
it should not be double-quoted.
"""
from superset.connectors.sqla.models import SqlaTable

table = SqlaTable(
table_name="test_table",
database=database,
)

# Test already quoted column
already_quoted_col: AdhocColumn = {
"sqlExpression": '"Already Quoted"',
"label": "Already Quoted",
"isColumnReference": True,
}

result = table.adhoc_column_to_sqla(already_quoted_col)

# Should not be double-quoted
assert result is not None
result_str = str(result)
assert '""' not in result_str # No double quotes
assert '"Already Quoted"' in result_str
Loading