korbit-ai[bot] commented on code in PR #34583:
URL: https://github.com/apache/superset/pull/34583#discussion_r2258112341
##########
superset/db_engine_specs/mssql.py:
##########
@@ -163,6 +164,31 @@ def extract_error_message(cls, ex: Exception) -> str:
)
return f"{cls.engine} error: {cls._extract_error_message(ex)}"
+ @classmethod
+ def adjust_query_for_offset(cls, qry: Select) -> Select:
+ """
+ Modify SQLAlchemy query to add ORDER BY when OFFSET is present.
+
+ MSSQL requires an ORDER BY clause when using OFFSET. If no ORDER BY
+ is present but OFFSET is specified, we add a default ORDER BY clause.
+ """
+ # Check if this query has OFFSET but no ORDER BY by inspecting query
structure
+ try:
+ # Try to access the internal query structure safely
+ has_offset = getattr(qry, "_offset", None) is not None
+ has_order_by = getattr(qry, "_order_by", None) is not None
Review Comment:
### Inefficient SQLAlchemy Query Inspection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using getattr() to access internal SQLAlchemy query attributes is
inefficient and could break with SQLAlchemy version changes.
###### Why this matters
Accessing internal attributes directly adds overhead from attribute lookups
and exception handling, while being fragile across SQLAlchemy versions.
###### Suggested change ∙ *Feature Preview*
Use SQLAlchemy's public API to inspect the query structure. For example:
```python
if qry.offset_clause is not None and not qry.order_by_clauses:
qry = qry.order_by(cast(literal("1"), String))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6eca4cd4-aa23-4d38-9ba1-fc2f7e646b88/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6eca4cd4-aa23-4d38-9ba1-fc2f7e646b88?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6eca4cd4-aa23-4d38-9ba1-fc2f7e646b88?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6eca4cd4-aa23-4d38-9ba1-fc2f7e646b88?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6eca4cd4-aa23-4d38-9ba1-fc2f7e646b88)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4354f66e-9e1b-4ede-93bd-8ee4f90d9a00 -->
[](4354f66e-9e1b-4ede-93bd-8ee4f90d9a00)
##########
superset/models/core.py:
##########
@@ -734,6 +734,10 @@ def compile_sqla_query(
schema: str | None = None,
is_virtual: bool = False,
) -> str:
+ # Allow engine specs to modify queries before compilation
+ if hasattr(self.db_engine_spec, "adjust_query_for_offset"):
+ qry = self.db_engine_spec.adjust_query_for_offset(qry)
Review Comment:
### Missing OFFSET verification before query adjustment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The implementation only checks if the method exists but doesn't verify if
the query actually contains an OFFSET clause before applying the transformation.
###### Why this matters
If the adjustment is applied to queries without OFFSET, it could
unnecessarily add ORDER BY clauses to queries that don't require them,
potentially impacting query performance.
###### Suggested change ∙ *Feature Preview*
Add a check to verify if the query contains an OFFSET clause before applying
the transformation:
```python
if (hasattr(self.db_engine_spec, "adjust_query_for_offset") and
self.db_engine_spec.has_offset(qry)):
qry = self.db_engine_spec.adjust_query_for_offset(qry)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9f97e36-65a1-4fb8-a330-300ce72211e9/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9f97e36-65a1-4fb8-a330-300ce72211e9?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9f97e36-65a1-4fb8-a330-300ce72211e9?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9f97e36-65a1-4fb8-a330-300ce72211e9?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9f97e36-65a1-4fb8-a330-300ce72211e9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4e414559-19ae-4d81-944c-247cf086548f -->
[](4e414559-19ae-4d81-944c-247cf086548f)
##########
superset/db_engine_specs/mssql.py:
##########
@@ -163,6 +164,31 @@
)
return f"{cls.engine} error: {cls._extract_error_message(ex)}"
+ @classmethod
+ def adjust_query_for_offset(cls, qry: Select) -> Select:
+ """
+ Modify SQLAlchemy query to add ORDER BY when OFFSET is present.
+
+ MSSQL requires an ORDER BY clause when using OFFSET. If no ORDER BY
+ is present but OFFSET is specified, we add a default ORDER BY clause.
+ """
+ # Check if this query has OFFSET but no ORDER BY by inspecting query
structure
+ try:
+ # Try to access the internal query structure safely
+ has_offset = getattr(qry, "_offset", None) is not None
+ has_order_by = getattr(qry, "_order_by", None) is not None
+
+ if has_offset and not has_order_by:
+ # Add a default ORDER BY clause using a string literal
+ # that works with window functions
+ # This preserves data order without permutation
+ qry = qry.order_by(cast(literal("1"), String))
Review Comment:
### Non-Deterministic ORDER BY <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using a constant value '1' in ORDER BY might not provide consistent ordering
across executions as SQL Server doesn't guarantee stable sorting when values
are equal.
###### Why this matters
Inconsistent row ordering can lead to unpredictable pagination results when
using OFFSET, potentially causing rows to be skipped or duplicated across pages.
###### Suggested change ∙ *Feature Preview*
Use a unique column or combination of columns that ensures consistent
ordering:
```python
# Add a comment explaining the limitation
# Use a more stable ordering, preferably with a unique column if available
qry = qry.order_by(cast(literal("(SELECT NULL)"), String))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a5a2ec8-3ef1-4526-9b8e-abd4b8829aeb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a5a2ec8-3ef1-4526-9b8e-abd4b8829aeb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a5a2ec8-3ef1-4526-9b8e-abd4b8829aeb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a5a2ec8-3ef1-4526-9b8e-abd4b8829aeb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a5a2ec8-3ef1-4526-9b8e-abd4b8829aeb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4d68249d-9d1e-4370-9f30-5b722eb82888 -->
[](4d68249d-9d1e-4370-9f30-5b722eb82888)
--
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]
