You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kaya-abdullah (via GitHub)" <gi...@apache.org> on 2026/10/14 23:03:36 UTC
[PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
kaya-abdullah opened a new pull request, #45309:
URL: https://github.com/apache/superset/pull/45309
### SUMMARY
Oracle added `FETCH FIRST n ROWS ONLY` in 12c, and sqlglot renders every Oracle limit that way. So on 10g and 11g every SQL Lab query fails with "ORA-00933: SQL command not properly ended", because Superset adds a row limit to it (#35197, #38520).
SQLAlchemy switches its own Oracle limits to `ROWNUM` below 12c, but only after the engine's first connection, since that is when it learns the server version. Superset compiles a chart's query before running it, so the first chart on a new engine, for example right after a restart, fails on 11g with the same error, and later ones work.
This PR fixes both.
**SQL that Superset limits (SQL Lab, `apply_limit_to_sql`).** On servers older than 12c the query is wrapped:
SELECT * FROM (<original query>) WHERE ROWNUM <= n
`ROWNUM` is assigned after the inner `ORDER BY`, so top-n queries still return the top rows. CTEs are moved to the outer statement, like the existing wrap in `cap_limit_value`. The server version is detected, so 11g works without any setup:
- If the database `version` extra (Advanced > Other > Version) is set, it is used and no connection is made. Values like `11g`, `11.2.0.4` and `19c` work, since only the major number is read.
- Otherwise the version is taken from SQLAlchemy's `server_version_info`, connecting once if the engine hasn't connected yet, and cached per Oracle database. If it can't be read, nothing changes and the query keeps `FETCH FIRST`.
12c and later keep `FETCH FIRST` on purpose. I first tried `ROWNUM` for every version, as suggested in #35197, but on Oracle 23ai that breaks queries with duplicate output names, for example `SELECT a.id, b.id FROM a JOIN b ...`: `FETCH FIRST` accepts them, the `ROWNUM` wrapper fails with ORA-00918. On 11g the wrapper is the only option, so repeated output names get a `_2`, `_3`, ... suffix before wrapping, kept within Oracle's 30-byte limit for those releases. Metabase's Oracle driver handles its wrapped queries the same way.
**SQL that SQLAlchemy compiles (charts).** `OracleEngineSpec.register_engine_events` starts each new engine's dialect on `ROWNUM`. That works on every release, and SQLAlchemy's first connection sets it back to `FETCH FIRST` for 12c and later, so nothing changes there after the first query. This needs no extra connection.
Changes:
- `BaseEngineSpec.get_limit_method(database)`, mirroring `get_allows_alias_in_select`, returns `limit_method` by default. The code that applies a limit (SQL Lab, `SQLExecutor`, `Database.apply_limit_to_sql`) calls it instead of reading the attribute.
- `OracleEngineSpec.get_limit_method` returns the new `LimitMethod.WRAP_ROWNUM` below 12c, using the configured or detected version.
- `OracleEngineSpec.register_engine_events` compiles limits with `ROWNUM` until SQLAlchemy knows the server version.
- `SQLStatement.get_limit_value` also reads a top-level `ROWNUM <= n` (or `< n`) filter on Oracle, so a user's own `ROWNUM` limit is kept by `cap_limit_value` and the wrapper isn't stacked on itself.
- Note on Oracle in the engine spec metadata.
The version detection only runs for Oracle databases without a `version` extra, and opens at most one connection per Oracle database and process, only if the engine hasn't connected yet. Other engines keep using the `limit_method` attribute and never connect for this.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Superset master (`apache/superset:367b6f5-dev`) against Oracle XE 11.2.0.2, no `version` extra. Before is the image as is, after is the same image with this branch's files.
SQL Lab, `SELECT * FROM nums`
Before:
SCREENSHOT_1_BEFORE
After:
SCREENSHOT_1_AFTER
First chart after a restart
Before:
SCREENSHOT_2_BEFORE
After:
SCREENSHOT_2_AFTER
SQL Lab, join returning two `id` columns
Before:
SCREENSHOT_3_BEFORE
After:
SCREENSHOT_3_AFTER
### TESTING INSTRUCTIONS
Unit tests:
pytest tests/unit_tests/db_engine_specs/test_oracle.py tests/unit_tests/sql/parse_tests.py tests/unit_tests/models/core_test.py -k "rownum or limit_method or limit_value or apply_limit or deduplicate or register_engine_events"
In the UI, `apache/superset:367b6f5-dev` with and without this branch's files, against `gvenzl/oracle-xe:11` (11.2.0.2), no `version` extra, python-oracledb in thick mode (Instant Client 19, needed for 11g). Both containers were restarted before the chart step, so it is the first query on a new engine:
| Step | Before | After |
|---|---|---|
| SQL Lab, `SELECT * FROM nums` (the query from #38520) | ORA-00933 | 20 rows |
| SQL Lab, join returning two `id` columns | ORA-00933 | 50 rows, columns `ID`, `ID_2` |
| SQL Lab, running the table's generated `SELECT` (`/select_star/`) | ORA-00933 | 50 rows |
| Table chart, first query after a restart | ORA-00933 | 10 rows |
| Same chart, second query | 10 rows | 10 rows |
The same code paths were also run in-process against 11.2.0.2 and 23ai with both python-oracledb and cx_Oracle 8.3, the driver from #38520. On 23ai nothing changes except that a chart compiled before the engine's first connection uses `ROWNUM` once. SQL Lab shapes checked on both servers: `SELECT 1 FROM dual`, top-n with `ORDER BY ... DESC`, a CTE, `UNION ALL`, `GROUP BY`, a window function, `DISTINCT`, `SELECT *`, a query that already has `WHERE ROWNUM <= 3`, a join returning two `n` columns, a parenthesized query, and a query formatted with Format SQL first. All return the same rows on both servers.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #35197, fixes #38520
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "kaya-abdullah (via GitHub)" <gi...@apache.org>.
kaya-abdullah commented on code in PR #45309:
URL: https://github.com/apache/superset/pull/45309#discussion_r4288787170
##########
superset/sql/parse.py:
##########
@@ -1000,6 +1003,82 @@ 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.
+
+ The limit is in bytes, so a multibyte name is cut on a character boundary.
+ """
+ suffix = 2
+ while True:
+ tail = f"_{suffix}"
+ budget = ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH - len(tail)
+ stem = identifier.name.encode()[:budget].decode(errors="ignore")
+ alias = exp.to_identifier(stem + tail, quoted=identifier.quoted)
+ if _oracle_name_key(alias) not in taken:
+ return alias
+ suffix += 1
+
+
+def _deduplicate_oracle_output_names(query: exp.Expression) -> None:
+ """
+ Rename repeated output columns so the query can be wrapped on Oracle.
+
+ Oracle rejects `SELECT * FROM (<query>)` when the query returns two columns
+ with the same name, as in `SELECT a.id, b.id FROM a JOIN b ...`, with
+ "ORA-00918: column ambiguously defined". The second and later copies get a
+ `_2`, `_3`, ... suffix instead. Star projections are left alone, since their
Review Comment:
You're right, that framing came from how I ran into it and it hides why the function is there. Reworded the docstring and the comment above the `WRAP_ROWNUM` branch in b6051bdba8: the wrapper fails with ORA-00918 on 11g too, and both shapes it leaves alone are written down now, qualified stars and repeated expressions without an alias.
I ran your four queries on 11.2.0.2 and got the same results. `SELECT 1 + 1 AS x, 1 + 1 AS x` is the variant the rename does fix, it comes back as X and X_2. Updated the PR description too.
##########
superset/db_engine_specs/oracle.py:
##########
@@ -51,14 +62,83 @@ class OracleEngineSpec(BaseEngineSpec):
"notes": (
"Previously used cx_Oracle, now uses oracledb. Legacy LONG columns are "
"treated as text, not numeric data, so Explore does not select SUM as "
- "their default aggregate."
+ "their default aggregate. On Oracle 11g and older, which reject FETCH "
+ "FIRST, row limits use ROWNUM. The server version is detected on "
+ "first use, or can be set under Advanced > Other > Version."
),
"docs_url": "https://python-oracledb.readthedocs.io/en/latest/user_guide/installation.html",
}
force_column_alias_quotes = True
max_column_name_length = 128
Review Comment:
Agreed, that one needs the same version check as `limit_method`, so it fits better in its own PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45309:
URL: https://github.com/apache/superset/pull/45309#issuecomment-6180790954
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45309?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.37%. Comparing base ([`367b6f5`](https://app.codecov.io/gh/apache/superset/commit/367b6f5cbf5bdd3326ff1a6fc2f8a9eb679b02e9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`02eafa3`](https://app.codecov.io/gh/apache/superset/commit/02eafa3328ffee344994fe6eeb4becd00ea4b05a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45309 +/- ##
===========================================
+ Coverage 56.16% 80.37% +24.21%
===========================================
Files 3040 3040
Lines 198655 198763 +108
Branches 46360 46383 +23
===========================================
+ Hits 111565 159761 +48196
+ Misses 86024 35573 -50451
- Partials 1066 3429 +2363
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45309/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45309/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.96% <19.81%> (-0.02%)` | :arrow_down: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45309/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.72% <19.81%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45309/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.48% <100.00%> (+44.69%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45309/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `80.93% <100.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45309?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45309:
URL: https://github.com/apache/superset/pull/45309#issuecomment-6180602698
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because legacy Oracle limit rewriting still has credible edge-case failures.
- Duplicate multibyte aliases are truncated by character count, so generated names can exceed Oracle 11g's 30-byte identifier limit.
- Star projections can retain duplicate output names in wrapped queries, and quoted "rownum" columns can be mistaken for Oracle's pseudocolumn.
**Assessed commit:** `02eafa3328ff`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45309:
URL: https://github.com/apache/superset/pull/45309#discussion_r4277458327
##########
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:
โ
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `e846a23`.
The alias stem is now truncated by encoded byte length, then decoded on a character boundary; the ASCII suffix is included in the byte budget.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
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:
โ
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `e846a23`.
The ROWNUM check now excludes quoted identifiers, so a quoted column named `rownum` is not treated as the pseudocolumn.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #45309:
URL: https://github.com/apache/superset/pull/45309#discussion_r4288091510
##########
superset/sql/parse.py:
##########
@@ -1000,6 +1003,82 @@ 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.
+
+ The limit is in bytes, so a multibyte name is cut on a character boundary.
+ """
+ suffix = 2
+ while True:
+ tail = f"_{suffix}"
+ budget = ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH - len(tail)
+ stem = identifier.name.encode()[:budget].decode(errors="ignore")
+ alias = exp.to_identifier(stem + tail, quoted=identifier.quoted)
+ if _oracle_name_key(alias) not in taken:
+ return alias
+ suffix += 1
+
+
+def _deduplicate_oracle_output_names(query: exp.Expression) -> None:
+ """
+ Rename repeated output columns so the query can be wrapped on Oracle.
+
+ Oracle rejects `SELECT * FROM (<query>)` when the query returns two columns
+ with the same name, as in `SELECT a.id, b.id FROM a JOIN b ...`, with
+ "ORA-00918: column ambiguously defined". The second and later copies get a
+ `_2`, `_3`, ... suffix instead. Star projections are left alone, since their
Review Comment:
Good catch finding this yourself, and I think it deserves more credit than the PR body gives it. I ran the un-deduplicated wrap against a real `gvenzl/oracle-xe:11` (11.2.0.2) and it fails there too, not only on 23ai:
```
SELECT * FROM (SELECT a.id, b.id FROM a JOIN b ON a.id = b.id) WHERE ROWNUM <= 3
ORA-00918: column ambiguously defined
```
Since `WRAP_ROWNUM` only ever runs below 12c, the 23ai framing points at a path this code cannot reach, so a later reader could take the whole function for dead weight. Worth naming 11g here too?
On the star sentence, your reply to the bot holds up, those queries do fail on master as well. But on 11.2.0.2 the shapes split in a way I did not expect, with a bare star fine and a qualified one not:
| wrapped query | result |
|---|---|
| `SELECT * FROM a JOIN b ON a.id = b.id` | 5 rows |
| `SELECT a.*, b.* FROM a JOIN b ON a.id = b.id` | ORA-00918 |
| `SELECT a.*, b.id FROM a JOIN b ON a.id = b.id` | ORA-00918 |
| `SELECT 1 + 1, 1 + 1 FROM dual` | ORA-00918 |
None of those is a regression so I do not think there is anything to fix, but the last row is a repeated unnamed expression rather than a star, and I would want both cases written down. Or am I missing a reason to leave it implicit?
##########
superset/db_engine_specs/oracle.py:
##########
@@ -51,14 +62,83 @@ class OracleEngineSpec(BaseEngineSpec):
"notes": (
"Previously used cx_Oracle, now uses oracledb. Legacy LONG columns are "
"treated as text, not numeric data, so Explore does not select SUM as "
- "their default aggregate."
+ "their default aggregate. On Oracle 11g and older, which reject FETCH "
+ "FIRST, row limits use ROWNUM. The server version is detected on "
+ "first use, or can be set under Advanced > Other > Version."
),
"docs_url": "https://python-oracledb.readthedocs.io/en/latest/user_guide/installation.html",
}
force_column_alias_quotes = True
max_column_name_length = 128
Review Comment:
Future-direction note, not a blocker and not for this PR. Even with this fix an 11g chart still breaks on any label longer than 30 characters, because `make_label_compatible` truncates to `max_column_name_length` and 128 is the 12.2+ limit. On 11.2.0.2:
```
SELECT n AS "Sum of total revenue by region and quarter" FROM nums
ORA-00972: identifier is too long
```
`make_label_compatible` passes that 42 character label straight through. Same version-dependent shape as `limit_method`, so probably its own change if anyone wants 11g charts working end to end.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #45309:
URL: https://github.com/apache/superset/pull/45309
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "kaya-abdullah (via GitHub)" <gi...@apache.org>.
kaya-abdullah commented on code in PR #45309:
URL: https://github.com/apache/superset/pull/45309#discussion_r4279554943
##########
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:
Star projections are left as they are because their column names aren't known without the table schema. Such a query fails the same way on 11g today, since `FETCH FIRST` isn't available there; aliasing the columns avoids it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #45309:
URL: https://github.com/apache/superset/pull/45309#discussion_r4362408237
##########
superset/sql/parse.py:
##########
@@ -1000,6 +1003,82 @@ 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.
+
+ The limit is in bytes, so a multibyte name is cut on a character boundary.
+ """
+ suffix = 2
+ while True:
+ tail = f"_{suffix}"
+ budget = ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH - len(tail)
+ stem = identifier.name.encode()[:budget].decode(errors="ignore")
+ alias = exp.to_identifier(stem + tail, quoted=identifier.quoted)
+ if _oracle_name_key(alias) not in taken:
+ return alias
+ suffix += 1
+
+
+def _deduplicate_oracle_output_names(query: exp.Expression) -> None:
+ """
+ Rename repeated output columns so the query can be wrapped on Oracle.
+
+ Oracle rejects `SELECT * FROM (<query>)` when the query returns two columns
+ with the same name, as in `SELECT a.id, b.id FROM a JOIN b ...`, with
+ "ORA-00918: column ambiguously defined". The second and later copies get a
+ `_2`, `_3`, ... suffix instead. Star projections are left alone, since their
Review Comment:
That reads much better, thanks. The docstring now says the wrapper is what fails rather than the release, and having both of the shapes it leaves alone written down is the part a future reader needs: `a.*, b.id` and the unaliased repeated expression are not obvious from the code. Good that you checked `SELECT 1 + 1 AS x, 1 + 1 AS x` separately, since that one is the rename working rather than a gap.
##########
superset/db_engine_specs/oracle.py:
##########
@@ -51,14 +62,83 @@ class OracleEngineSpec(BaseEngineSpec):
"notes": (
"Previously used cx_Oracle, now uses oracledb. Legacy LONG columns are "
"treated as text, not numeric data, so Explore does not select SUM as "
- "their default aggregate."
+ "their default aggregate. On Oracle 11g and older, which reject FETCH "
+ "FIRST, row limits use ROWNUM. The server version is detected on "
+ "first use, or can be set under Advanced > Other > Version."
),
"docs_url": "https://python-oracledb.readthedocs.io/en/latest/user_guide/installation.html",
}
force_column_alias_quotes = True
max_column_name_length = 128
Review Comment:
Agreed, its own PR is the right home. One thing that might save you a step when you get to it: this PR already adds `ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH` at `superset/sql/parse.py:1006` for the dedup suffixes, so the 30 byte bound is in the tree and the follow-up is mostly about teaching `max_column_name_length` the same version split that `limit_method` now has. Happy to review it when you open it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #45309:
URL: https://github.com/apache/superset/pull/45309#issuecomment-6434630375
Follow-up opened: #45758 (the `max_column_name_length` version check + the nested-AND `ROWNUM` detection gap).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45309:
URL: https://github.com/apache/superset/pull/45309#issuecomment-6203343664
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | b6051bdba8829d73149a4b0b4dbf57536625e667 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ad14f443eb3f6b585069ca9 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-45309--superset-docs-preview.netlify.app](https://deploy-preview-45309--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AstMyK_TzixKTc1J1i_LL80pzdXMyczNLgEpSUgty8is9U4CKzBJTDE3STEyMU5OM08ySTC1MDcwskxMtlWoB) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45309--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oracle): limit rows with ROWNUM on Oracle before 12c [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45309:
URL: https://github.com/apache/superset/pull/45309#discussion_r4279558453
##########
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:
โ
**Customized review instruction saved!**
**Instruction:**
> Leave star projections unchanged in the Oracle limit rewrite when their output names cannot be determined without schema; callers can alias columns to avoid duplicate names.
**Applied to:**
- `superset/sql/parse.py`
---
๐ก *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org