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