codeant-ai-for-open-source[bot] commented on code in PR #45309:
URL: https://github.com/apache/superset/pull/45309#discussion_r4277288304
##########
superset/sql/parse.py:
##########
@@ -1000,6 +1003,79 @@ def _comment_re_for(dialect: Dialects | None) ->
re.Pattern[str]:
)
+# Oracle 11g and older cap identifiers at 30 bytes.
+ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH = 30
+
+
+def _oracle_output_identifier(projection: exp.Expression) -> exp.Identifier |
None:
+ """
+ Return the identifier naming a projected column, if it has one.
+ """
+ if isinstance(projection, exp.Alias):
+ identifier = projection.args.get("alias")
+ elif isinstance(projection, exp.Column):
+ identifier = projection.this
+ else:
+ return None
+ return identifier if isinstance(identifier, exp.Identifier) else None
+
+
+def _oracle_name_key(identifier: exp.Identifier) -> str:
+ """
+ Return the name Oracle compares, since unquoted names are case-insensitive.
+ """
+ return identifier.name if identifier.quoted else identifier.name.upper()
+
+
+def _unique_oracle_alias(identifier: exp.Identifier, taken: set[str]) ->
exp.Identifier:
+ """
+ Return a `_2`, `_3`, ... variant of the identifier that isn't taken yet.
+ """
+ suffix = 2
+ while True:
+ tail = f"_{suffix}"
+ stem = identifier.name[: ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH -
len(tail)]
Review Comment:
**Suggestion:** This slices characters rather than bytes; a duplicate
multibyte alias can exceed Oracle 11g's 30-byte limit and fail with ORA-00972.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=380854974e914846b2141f90522af282&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=380854974e914846b2141f90522af282&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/parse.py
**Line:** 1037:1037
**Comment:**
*Logic Error: This slices characters rather than bytes; a duplicate
multibyte alias can exceed Oracle 11g's 30-byte limit and fail with ORA-00972.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45309&comment_hash=4e9dc5a9588d8dd0008955022b5e327378aabae3ef9286b6eebe32a39d0a6b0e&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45309&comment_hash=4e9dc5a9588d8dd0008955022b5e327378aabae3ef9286b6eebe32a39d0a6b0e&reaction=dislike'>๐</a>
##########
superset/sql/parse.py:
##########
@@ -2178,6 +2300,35 @@ def set_limit_value(
if not isinstance(self._parsed, exp.Query):
return
+ # Oracle only gained `FETCH FIRST` in 12c, and that is how sqlglot
+ # renders every Oracle limit, so on 10g and 11g a limited query fails
+ # with "ORA-00933: SQL command not properly ended". There, cap the rows
+ # by filtering the original query on `ROWNUM` from outside. `ROWNUM` is
+ # assigned after the inner `ORDER BY` runs, so the first `n` rows are
+ # still the top `n`. SQLAlchemy uses the same rewrite for pre-12c
+ # servers. Later releases keep `FETCH FIRST`, because the wrapper
rejects
+ # duplicate output column names (ORA-00918) that `FETCH FIRST` accepts.
+ if method == LimitMethod.WRAP_ROWNUM:
+ inner = self._parsed.copy()
+ # Keep CTEs at statement level rather than inside the derived
table.
+ cte = inner.args.pop("with_", None)
+ _deduplicate_oracle_output_names(inner)
Review Comment:
**Suggestion:** When a star expands to a name also produced by another
projection, deduplication leaves it unchanged and Oracle 11g rejects the
wrapped query with ORA-00918.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3aee3a3118234057ad31a48eb4313bc4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3aee3a3118234057ad31a48eb4313bc4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/parse.py
**Line:** 2315:2315
**Comment:**
*Logic Error: When a star expands to a name also produced by another
projection, deduplication leaves it unchanged and Oracle 11g rejects the
wrapped query with ORA-00918.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45309&comment_hash=96a43366104735d2f5cd5f2c370cb0b9710b22a4cb1cd83c2e7e11351fb7fd0b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45309&comment_hash=96a43366104735d2f5cd5f2c370cb0b9710b22a4cb1cd83c2e7e11351fb7fd0b&reaction=dislike'>๐</a>
##########
superset/sql/parse.py:
##########
@@ -2116,7 +2192,53 @@ def get_limit_value(self) -> int | None:
if isinstance(literal, exp.Literal) and literal.is_int:
return int(literal.name)
- return None
+ return self._get_rownum_limit()
+
+ def _get_rownum_limit(self) -> int | None:
+ """
+ Return the row cap of a top-level Oracle `ROWNUM` filter, if any.
+
+ Oracle releases before 12c have no `FETCH FIRST`, so a `ROWNUM <= n`
+ condition in the outer `WHERE` is how a query caps its rows there. It
is
+ also the shape `set_limit_value()` produces with `WRAP_ROWNUM`. Only
+ conditions that are `AND`-ed into the filter count: under an `OR` the
+ comparison no longer bounds the result.
+ """
+ if self._dialect != Dialects.ORACLE or not isinstance(self._parsed,
exp.Select):
+ return None
+
+ where = self._parsed.args.get("where")
+ if where is None:
+ return None
+
+ condition = where.this.unnest()
+ conditions = (
+ condition.flatten() if isinstance(condition, exp.And) else
[condition]
+ )
+ caps = []
+ for predicate in conditions:
+ predicate = predicate.unnest()
+ if isinstance(predicate, (exp.LTE, exp.LT)):
+ rownum, bound = predicate.this, predicate.expression
+ elif isinstance(predicate, (exp.GTE, exp.GT)):
+ bound, rownum = predicate.this, predicate.expression
+ else:
+ continue
+
+ rownum = rownum.unnest()
+ bound = bound.unnest()
+ if (
+ isinstance(rownum, exp.Column)
+ and not rownum.table
+ and rownum.name.upper() == "ROWNUM"
Review Comment:
**Suggestion:** A quoted, case-sensitive column named "rownum" is not the
ROWNUM pseudocolumn; misidentifying it makes cap_limit_value skip the row cap.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Incorrect condition
logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=482f5dba655142778cbf2dfea2de7332&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=482f5dba655142778cbf2dfea2de7332&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/parse.py
**Line:** 2233:2233
**Comment:**
*Incorrect Condition Logic: A quoted, case-sensitive column named
"rownum" is not the ROWNUM pseudocolumn; misidentifying it makes
cap_limit_value skip the row cap.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45309&comment_hash=ba8544850d9a236c95f2d4243511587e86803cd1fba93ad9319a670cf71b6143&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45309&comment_hash=ba8544850d9a236c95f2d4243511587e86803cd1fba93ad9319a670cf71b6143&reaction=dislike'>๐</a>
--
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]
