rusackas opened a new pull request, #45771:
URL: https://github.com/apache/superset/pull/45771
### SUMMARY
Follow-up from #45309. `SQLStatement._get_rownum_limit()` detects an
existing top-level Oracle `ROWNUM <= n` filter so `cap_limit_value()` tightens
it in place rather than stacking a second wrapper on top, but it only flattened
a *top-level* chain of `AND`s:
```python
condition = where.this.unnest()
conditions = (
condition.flatten() if isinstance(condition, exp.And) else [condition]
)
```
`exp.And.flatten()` unwraps a left- or right-leaning chain of plain `AND`s,
but a nested `AND` grouped in parentheses, as in `x = 1 AND (y = 2 AND ROWNUM
<= 5)`, comes back as a single `And` leaf instead of its own `y = 2` and
`ROWNUM <= 5` leaves. The loop's `else: continue` then drops it silently, so
`get_limit_value()` returns `None` even though the query already has a `ROWNUM`
cap.
Verified directly against the sqlglot AST before fixing:
```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 were already fine; only right-nested ones were missed
```
Practical effect: `cap_limit_value()` treats the query as having no existing
cap and wraps the whole thing in another `SELECT * FROM (...) WHERE ROWNUM <=
n`, instead of tightening the filter already there. This didn't corrupt results
(the final row count was still `min(existing, new)`, and the extra wrapper's
own projection is just `*`, so it didn't reintroduce the duplicate-column-name
bug #45309 fixed) and didn't error - just a redundant extra subquery layer in a
narrow edge case.
### THE FIX
Replaced the one-level `flatten()` call with `_flatten_and()`, a small
recursive helper that looks through parentheses around a nested `AND` so every
`AND`-ed leaf is found regardless of how the condition is grouped, while still
treating an `OR` as an opaque leaf (since a comparison under an `OR` doesn't
unconditionally bound the result, which is unchanged from before).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A, no UI surface.
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/sql/parse_tests.py -k "rownum or flatten_and or
get_limit_value"
```
Added:
- `test_flatten_and`: direct coverage of the new helper against several
`AND`/`OR` groupings, parenthesized on either side, nested two deep.
- Two new `test_get_limit_value` cases covering a `ROWNUM` cap nested in
parentheses on either side of the chain, and a three-way split across two
parenthesized groups.
- One new case confirming an `OR` nested inside parentheses is still
correctly excluded (`x = 1 AND (y = 2 OR ROWNUM <= 5)` stays `None`).
Also ran the full `tests/unit_tests/sql/parse_tests.py`,
`tests/unit_tests/db_engine_specs/test_oracle.py`, and
`tests/unit_tests/models/core_test.py` (1734 tests) to confirm no regression in
the surrounding Oracle/limit-handling logic from #45309.
### ADDITIONAL INFORMATION
- [x] Has associated issue: part of #45758
- [ ] 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
