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: [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]
