Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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' },
],
});
});
});
8 changes: 7 additions & 1 deletion superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1502,8 +1502,14 @@ 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"):
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
33 changes: 33 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,34 @@ 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
Loading