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