sadpandajoe commented on code in PR #41492:
URL: https://github.com/apache/superset/pull/41492#discussion_r3679975019
##########
superset/db_engine_specs/sybase.py:
##########
@@ -30,6 +30,11 @@ class SybaseEngineSpec(MssqlEngineSpec):
engine = "sybase"
engine_name = "SAP Sybase"
engine_aliases = {"sybase_sqlany"} # Support SQL Anywhere dialect too
+
+ # Unlike MSSQL, SAP ASE/SQL Anywhere quote identifiers with double quotes,
+ # not square brackets, so override the values inherited from
MssqlEngineSpec.
+ identifier_quote_start: str = '"'
Review Comment:
[major] This override makes SQL Lab insert double-quoted names for SAP ASE,
but the supported `sqlalchemy-sybase` adapter uses brackets and default ASE
sessions do not treat double quotes as identifiers. Please keep the inherited
`[`/`]` delimiters and update the Sybase test.
##########
superset/sqllab/utils.py:
##########
@@ -41,6 +41,7 @@
"disable_data_preview",
"disable_drill_to_detail",
"allow_multi_catalog",
+ "engine_information",
Review Comment:
[major] This allowlist entry is the only bridge that puts
`engine_information` into the initial SQL Lab store, but the added tests seed
that state manually or exercise other APIs. Please add a `/api/v1/sqllab/`
regression assertion that a non-ANSI database returns the expected
`identifier_quote` payload.
##########
superset/db_engine_specs/base.py:
##########
@@ -540,6 +540,15 @@ class BaseEngineSpec: # pylint:
disable=too-many-public-methods
arraysize = 0
max_column_name_length: int | None = None
+ # Characters used to quote identifiers (table/column names) that aren't
simple.
+ # Defaults to ANSI double quotes; dialects that differ override these —
e.g.
+ # MySQL/MariaDB use backticks and SQL Server uses square brackets. Embedded
+ # quote characters are escaped by doubling the closing character. These are
+ # surfaced to the client (see `get_public_information`) so identifier
quoting
+ # stays owned by the engine spec rather than duplicated per client.
+ identifier_quote_start: str = '"'
Review Comment:
[minor] Hive, Spark, and Databricks inherit this ANSI default even though
their registered dialects quote identifiers with backticks, so autocomplete
remains invalid for non-simple names on those engines. Please override the
shared Hive and Databricks base specs and add representative assertions.
##########
superset/databases/schemas.py:
##########
@@ -1191,6 +1200,12 @@ class EngineInformationSchema(Schema):
)
}
)
+ identifier_quote = fields.Nested(
Review Comment:
[minor] This documents the database API field, but the value is also added
to the SQL Lab bootstrap response while `SQLLabBootstrapSchema` still models
databases as `ImportV1DatabaseSchema`. Please use an
`EngineInformationSchema`-aware database shape for the bootstrap OpenAPI
contract too.
##########
superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts:
##########
@@ -210,6 +211,100 @@ test('quotes table identifiers that require quoting in
the inserted value', asyn
);
});
+test.each([
+ ['mysql', { start: '`', end: '`' }, '`COVID Vaccines`'],
+ ['mariadb', { start: '`', end: '`' }, '`COVID Vaccines`'],
+ ['mssql', { start: '[', end: ']' }, '[COVID Vaccines]'],
+ ['postgresql', { start: '"', end: '"' }, '"COVID Vaccines"'],
+])(
+ 'quotes table identifiers using the engine-provided quote characters for %s',
+ async (_dialect, identifierQuote, expectedValue) => {
+ const dbFunctionNamesApiRoute =
`glob:*/api/v1/database/${expectDbId}/function_names/`;
+ fetchMock.get(dbFunctionNamesApiRoute, fakeFunctionNamesApiResult);
+
+ const storeWithBackend = createStore(
+ {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ databases: {
+ [expectDbId]: {
+ engine_information: { identifier_quote: identifierQuote },
+ },
+ },
+ },
+ },
+ reducers,
+ );
+
+ act(() => {
+ storeWithBackend.dispatch(
+ tableApiUtil.upsertQueryData(
+ 'tables',
+ { dbId: expectDbId, schema: expectSchema },
+ {
+ options: [
+ {
+ value: 'COVID Vaccines',
+ label: 'COVID Vaccines',
+ type: 'table',
+ },
+ ],
+ hasMore: false,
+ },
+ ),
+ );
+ });
+
+ const { result } = renderHook(
+ () =>
+ useKeywords({
+ queryEditorId: 'testqueryid',
+ dbId: expectDbId,
+ schema: expectSchema,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store: storeWithBackend,
+ }),
+ },
+ );
+
+ await waitFor(() =>
+ expect(result.current).toContainEqual(
+ expect.objectContaining({
+ name: 'COVID Vaccines',
+ value: expectedValue,
+ meta: 'table',
+ }),
+ ),
+ );
+
+ // The caption inserted into the editor on selection is quoted with the
+ // same dialect-specific characters as `value`, not a hardcoded ANSI quote.
+ const tableKeyword = result.current.find(
+ keyword => keyword.meta === 'table' && keyword.name === 'COVID Vaccines',
+ );
+ const insertMatch = tableKeyword?.completer?.insertMatch;
Review Comment:
[minor] This directly invokes a custom completer that
`AceEditorProvider.toAceKeyword` drops in production, and it fabricates the
optional `caption`. Please exercise keyword conversion and selection through
the adapter boundary so the test validates the shipped Ace path.
--
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]
