korbit-ai[bot] commented on code in PR #35350:
URL: https://github.com/apache/superset/pull/35350#discussion_r2393070903
##########
superset/connectors/sqla/models.py:
##########
@@ -1403,13 +1403,19 @@ def get_sqla_table(self) -> TableClause:
# project.dataset.table format
if self.catalog and
self.database.db_engine_spec.supports_cross_catalog_queries:
# SQLAlchemy doesn't have built-in catalog support for TableClause,
- # so we need to construct the full identifier manually
+ # so we need to construct the full identifier manually with proper
quoting
+ catalog_quoted = self.quote_identifier(self.catalog)
+ table_quoted = self.quote_identifier(self.table_name)
Review Comment:
### Missing quote_identifier method <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code calls `self.quote_identifier()` method but this method is not
defined in the SqlaTable class or its parent classes.
###### Why this matters
This will cause an AttributeError at runtime when the method is called,
breaking the cross-catalog query functionality entirely.
###### Suggested change ∙ *Feature Preview*
The `quote_identifier` method needs to be implemented or the code should use
the database's quoting mechanism. Replace with:
```python
catalog_quoted =
self.database.inspector.dialect.identifier_preparer.quote(self.catalog)
table_quoted =
self.database.inspector.dialect.identifier_preparer.quote(self.table_name)
```
Or implement the missing method:
```python
def quote_identifier(self, identifier: str) -> str:
return
self.database.inspector.dialect.identifier_preparer.quote(identifier)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67fd6071-ba86-4396-ac56-35f5354b05d4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67fd6071-ba86-4396-ac56-35f5354b05d4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67fd6071-ba86-4396-ac56-35f5354b05d4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67fd6071-ba86-4396-ac56-35f5354b05d4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67fd6071-ba86-4396-ac56-35f5354b05d4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f1956875-c4b3-4fa6-9b01-2f590c9a4f1b -->
[](f1956875-c4b3-4fa6-9b01-2f590c9a4f1b)
##########
superset/connectors/sqla/models.py:
##########
@@ -1403,13 +1403,19 @@ def get_sqla_table(self) -> TableClause:
# project.dataset.table format
if self.catalog and
self.database.db_engine_spec.supports_cross_catalog_queries:
# SQLAlchemy doesn't have built-in catalog support for TableClause,
- # so we need to construct the full identifier manually
+ # so we need to construct the full identifier manually with proper
quoting
+ catalog_quoted = self.quote_identifier(self.catalog)
+ table_quoted = self.quote_identifier(self.table_name)
+
if self.schema:
- full_name = f"{self.catalog}.{self.schema}.{self.table_name}"
+ schema_quoted = self.quote_identifier(self.schema)
+ full_name = f"{catalog_quoted}.{schema_quoted}.{table_quoted}"
else:
- full_name = f"{self.catalog}.{self.table_name}"
+ full_name = f"{catalog_quoted}.{table_quoted}"
Review Comment:
### Inefficient individual identifier quoting calls <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple individual calls to quote_identifier() method instead of batching
the quoting operations, potentially causing unnecessary overhead when
processing multiple identifiers.
###### Why this matters
Each quote_identifier() call may involve string processing, regex
operations, or database-specific formatting logic. Making separate calls for
each identifier component creates redundant overhead that could be minimized
through batching.
###### Suggested change ∙ *Feature Preview*
Consider batching the identifier quoting operations if the quote_identifier
method supports it, or cache the quoting logic if these identifiers are used
frequently. Alternatively, if the identifiers are static for a table instance,
consider caching the quoted versions as properties to avoid repeated
computation.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34f58ce3-aeb4-4318-897d-58d969f1b757/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34f58ce3-aeb4-4318-897d-58d969f1b757?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34f58ce3-aeb4-4318-897d-58d969f1b757?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34f58ce3-aeb4-4318-897d-58d969f1b757?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34f58ce3-aeb4-4318-897d-58d969f1b757)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:98e8ad66-77d6-48e5-a08f-17c71ca82460 -->
[](98e8ad66-77d6-48e5-a08f-17c71ca82460)
--
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]
