rusackas opened a new issue, #45758:
URL: https://github.com/apache/superset/issues/45758
### Context
Follow-ups from #45309 (merged in 9cb86d8), which fixed Oracle pre-12c row
limiting (`ROWNUM` instead of `FETCH FIRST`) and the first-query-after-restart
dialect-detection timing bug. Two things were flagged as "not for this PR"
during review and are worth a dedicated pass.
### 1. `max_column_name_length` needs the same version check as
`limit_method`
Raised by @gabotorresruiz on #45309: even with the row-limit fix, an Oracle
11g chart still breaks on any label longer than 30 characters, because
`make_label_compatible` truncates to `max_column_name_length`, and
`OracleEngineSpec.max_column_name_length = 128` is the 12.2+ limit. On
`gvenzl/oracle-xe:11` (11.2.0.2):
```sql
SELECT n AS "Sum of total revenue by region and quarter" FROM nums
-- ORA-00972: identifier is too long
```
#45309 already added `ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH = 30` in
`superset/sql/parse.py` for the ROWNUM-wrap dedup suffixes, so the 30-byte
bound is already in the tree. This mostly needs `max_column_name_length` to use
the same `get_server_major_version()`-driven version split that
`OracleEngineSpec.get_limit_method()` now has (likely a new
`get_max_column_name_length(database)` classmethod mirroring
`get_limit_method`, with `BaseEngineSpec` gaining the same
default-delegates-to-the-attribute shape).
### 2. `_get_rownum_limit()` misses a `ROWNUM` filter nested inside a
parenthesized `AND` group
Found during review of #45309. `SQLStatement._get_rownum_limit()`
(`superset/sql/parse.py`) only flattens a top-level chain of ANDs:
```python
condition = where.this.unnest()
conditions = condition.flatten() if isinstance(condition, exp.And) else
[condition]
```
For `WHERE x = 1 AND (y = 2 AND ROWNUM <= 5)`, sqlglot's `flatten()` yields
the parenthesized group as a single nested `And` leaf rather than recursing
into it, so the loop's `else: continue` silently drops the `ROWNUM <= 5`
condition. Verified directly against the sqlglot AST:
```python
>>> get_rownum_limit("SELECT * FROM t WHERE x = 1 AND ROWNUM <= 5")
5
>>> get_rownum_limit("SELECT * FROM t WHERE x = 1 AND (y = 2 AND ROWNUM <=
5)")
None # should be 5
>>> get_rownum_limit("SELECT * FROM t WHERE (x = 1 AND y = 2) AND ROWNUM <=
5")
5 # left-nested parens are fine; only right-nested ones are missed
```
Practical effect: `cap_limit_value()` treats the query as having no existing
cap and stacks an extra `SELECT * FROM (...) WHERE ROWNUM <= n` wrapper around
a query that already had one, instead of tightening the existing filter in
place. This doesn't corrupt results (the final row count is still `min(5, n)`,
and the added wrapper's own projection is just `*`, so it doesn't reintroduce
the duplicate-column-name bug #45309 fixed) and doesn't fail - it's a redundant
extra subquery layer in a narrow edge case (a hand-written or
externally-sourced query with `ROWNUM` nested inside a parenthesized `AND`
group, that Superset then separately caps). Worth fixing by recursing into
nested `And` leaves rather than treating them as opaque, but not urgent.
cc @kaya-abdullah @gabotorresruiz
--
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]
