korbit-ai[bot] commented on code in PR #35342:
URL: https://github.com/apache/superset/pull/35342#discussion_r2392174667
##########
superset/models/helpers.py:
##########
@@ -871,6 +871,40 @@ def _process_sql_expression( # pylint:
disable=too-many-arguments
raise QueryObjectValidationError(ex.message) from ex
return expression
+ def _process_orderby_expression(
+ self,
+ expression: Optional[str],
+ database_id: int,
+ engine: str,
+ schema: str,
+ template_processor: Optional[BaseTemplateProcessor],
+ ) -> Optional[str]:
+ """
+ Validate and process an ORDER BY clause expression.
+
+ This requires prefixing the expression with a dummy SELECT statement,
so it can
+ be properly parsed and validated.
+ """
+ if expression:
+ expression = f"SELECT 1 ORDER BY {expression}"
+
+ if processed := self._process_sql_expression(
+ expression=expression,
+ database_id=self.database_id,
+ engine=self.database.backend,
+ schema=self.schema,
+ template_processor=template_processor,
+ ):
+ prefix, expression = re.split(
+ r"ORDER\s+BY",
+ processed,
+ maxsplit=1,
+ flags=re.IGNORECASE,
+ )
+ return expression.strip()
Review Comment:
### Method parameters ignored in favor of instance properties
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The method uses parameters from self (self.database_id,
self.database.backend, self.schema) instead of the parameters passed to the
method (database_id, engine, schema).
###### Why this matters
This inconsistency means the method ignores the parameters it receives and
always uses the instance's properties, which defeats the purpose of having
these as parameters and could lead to incorrect processing when different
database contexts are needed.
###### Suggested change ∙ *Feature Preview*
Use the method parameters instead of instance properties:
```python
if processed := self._process_sql_expression(
expression=expression,
database_id=database_id,
engine=engine,
schema=schema,
template_processor=template_processor,
):
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba44d882-a4e6-48eb-ba71-8fe3323ab92f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba44d882-a4e6-48eb-ba71-8fe3323ab92f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba44d882-a4e6-48eb-ba71-8fe3323ab92f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba44d882-a4e6-48eb-ba71-8fe3323ab92f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba44d882-a4e6-48eb-ba71-8fe3323ab92f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2f9e886e-2d91-4ecd-9107-6818a4bfe801 -->
[](2f9e886e-2d91-4ecd-9107-6818a4bfe801)
--
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]
