korbit-ai[bot] commented on code in PR #35553:
URL: https://github.com/apache/superset/pull/35553#discussion_r2410322004
##########
superset/connectors/sqla/models.py:
##########
@@ -905,7 +905,21 @@ def get_sqla_col(
msg=msg,
)
) from ex
- col = literal_column(expression, type_=type_)
+ # Quote the expression if it contains spaces to prevent SQL
parsing issues
+ # For example, "Test column" should not be interpreted as "Test AS
column"
+ if (
+ expression
+ and " " in expression
+ and not any(
+ keyword in expression.upper()
+ for keyword in ["SELECT", "CASE", "WHEN", "AS", "FROM",
"WHERE"]
+ )
+ ):
Review Comment:
### Inefficient repeated string uppercasing in keyword detection
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The expression.upper() method is called for every keyword check, creating
multiple uppercase string copies unnecessarily.
###### Why this matters
This creates O(n*m) string operations where n is the number of keywords and
m is the length of the expression, causing unnecessary memory allocations and
CPU cycles for each column processing.
###### Suggested change ∙ *Feature Preview*
Cache the uppercased expression once before the loop:
```python
if (
expression
and " " in expression
and (expression_upper := expression.upper())
and not any(
keyword in expression_upper
for keyword in ["SELECT", "CASE", "WHEN", "AS", "FROM", "WHERE"]
)
):
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c0ce6e-4308-4875-b092-3bc045d8f62f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c0ce6e-4308-4875-b092-3bc045d8f62f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c0ce6e-4308-4875-b092-3bc045d8f62f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c0ce6e-4308-4875-b092-3bc045d8f62f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5c0ce6e-4308-4875-b092-3bc045d8f62f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:34c9c687-c38a-4a5b-aec7-92cb2ebb2d49 -->
[](34c9c687-c38a-4a5b-aec7-92cb2ebb2d49)
##########
superset/models/helpers.py:
##########
@@ -886,6 +886,15 @@ def _process_select_expression(
be properly parsed and validated.
"""
if expression:
+ # Quote column names with spaces to prevent parsing issues
+ # For example, "Test column" should not be parsed as "Test AS
column"
+ if " " in expression and not any(
+ keyword in expression.upper()
+ for keyword in ["SELECT", "CASE", "WHEN", "AS", "FROM",
"WHERE"]
+ ):
+ # This looks like a column name with spaces, quote it
+ expression = f'"{expression}"'
Review Comment:
### Hard-coded quote character not engine-specific <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Hard-coded double quotes may not be compatible with all database engines
that use different identifier quoting styles.
###### Why this matters
Different database engines use different quoting characters (e.g., MySQL
uses backticks, SQL Server uses square brackets). Using hard-coded double
quotes could cause SQL syntax errors on engines that don't support this quoting
style.
###### Suggested change ∙ *Feature Preview*
Use the database engine's specific identifier quoting method:
```python
# Get the appropriate quote character for this database engine
with self.database.get_sqla_engine() as engine:
quote_char = engine.dialect.identifier_preparer.quote
expression = f'{quote_char}{expression}{quote_char}'
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/355b865f-82b1-44fe-9b05-6da811242044/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/355b865f-82b1-44fe-9b05-6da811242044?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/355b865f-82b1-44fe-9b05-6da811242044?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/355b865f-82b1-44fe-9b05-6da811242044?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/355b865f-82b1-44fe-9b05-6da811242044)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3a93e7c2-664d-4c5b-8064-6064fb512fcf -->
[](3a93e7c2-664d-4c5b-8064-6064fb512fcf)
--
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]
