rebenitez1802 commented on code in PR #45675:
URL: https://github.com/apache/superset/pull/45675#discussion_r4402211565
##########
superset/models/helpers.py:
##########
@@ -6098,7 +6100,7 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
def _create_join_condition(col_name: str, expr: Any) ->
Any:
# Get the corresponding column from the subquery
subq_col_name = db_engine_spec.make_label_compatible(
- col_name + "__"
+ col_name + "__", self.database
)
# Reference the column from the already-created
aliased subquery
subq_col = subq_alias.c[subq_col_name]
Review Comment:
🔴 High — "Group remaining as Others" crashes when Oracle hashes a series
label
`make_sqla_column_compatible` sets `sqla_col.key = label_expected`, so
`subq_alias.c` is keyed by `col_name + "__"`, not by the hashed alias. On
Oracle before 12.2, a series limit with "Others" on a series column of 29 or
more bytes now raises `KeyError: '0f979e1f19fd1f30ca919a1be3168a'` (a 500)
instead of running. A 12.2+ database whose Version is set to `12c` builds this
query on master and fails on this branch. Looking the column up by its expected
label fixes it, and it also fixes the same crash that master already has for
long Postgres columns:
```suggestion
# The subquery keys its columns by the expected
label, which
# differs from the SQL alias when the engine
shortened it
subq_col = subq_alias.c[col_name + "__"]
```
A test that builds the query with `group_others_when_limit_reached=True`,
`series_limit`, and a 29-byte series column on version `11.2.0.4` fails with
the KeyError on this branch and passes with the change.
##########
superset/db_engine_specs/oracle.py:
##########
@@ -86,24 +90,57 @@ def get_limit_method(cls, database: Database) ->
LimitMethod:
return cls.limit_method
+ @classmethod
+ def get_max_column_name_length(cls, database: Database) -> int | None:
+ """
+ Cap labels at 30 bytes on Oracle releases before 12.2.
+
+ Those releases reject longer labels with "ORA-00972: identifier is too
+ long". A version given only as "12c" is treated as 12.1, since 30 bytes
+ is accepted by every release.
+ """
+ version = cls.get_server_version(database)
+ if version is not None and version < LONG_IDENTIFIERS_VERSION:
+ return ORACLE_LEGACY_MAX_IDENTIFIER_LENGTH
+
+ return cls.max_column_name_length
+
+ @staticmethod
+ def _label_length(label: str) -> int:
+ """
+ Oracle limits identifiers in bytes, so count the label's UTF-8 bytes.
+
+ A label of non-ASCII characters can be too long with fewer characters
+ than the limit.
+ """
+ return len(label.encode())
+
@classmethod
def get_server_major_version(cls, database: Database) -> int | None:
"""
Return the major version of the Oracle server, or `None` if unknown.
+ """
+ version = cls.get_server_version(database)
+ return version[0] if version is not None else None
+
+ @classmethod
+ def get_server_version(cls, database: Database) -> tuple[int, ...] | None:
+ """
+ Return the version of the Oracle server, or `None` if unknown.
A version specified in the DB extra wins. Oracle names its releases
"11g", "12c" or "23ai", or by a dotted number like "11.2.0.4", so only
- the leading major number is read. Otherwise the version is read from
the
- server once and cached per database. SQLAlchemy already knows it if the
- engine has connected before; if not, this costs one connection per
+ the leading dotted numbers are read. Otherwise the version is read from
+ the server once and cached per database. SQLAlchemy already knows it if
+ the engine has connected before; if not, this costs one connection per
process.
"""
version = database.get_extra().get("version")
- if match := re.match(r"\s*(\d+)", str(version or "")):
- return int(match.group(1))
+ if match := re.match(r"\s*(\d+(?:\.\d+)*)", str(version or "")):
+ return tuple(int(part) for part in match.group(1).split("."))
key = f"{database.id}:{database.sqlalchemy_uri}"
- if key not in _server_major_versions:
+ if key not in _server_versions:
Review Comment:
🔴 High — A failed version lookup is retried once per label
`make_label_compatible` now reaches this lookup for every label, and a
failed lookup isn't remembered. With no Version set and the database down or
the password wrong, building one chart query tries to connect about 20 times;
master tries 0 times for "View query" and once to run the chart. With
python-oracledb's default 20-second connect timeout, one chart can hang for
about 400 seconds and hold a web worker, and a rotated password produces about
20 failed logins per chart. This is different from the earlier thread: the
retries happen many times inside one query, not once on the next query.
Remembering the failure for the current request or task keeps "try again
next time" and limits it to one attempt per request:
```python
from flask import g, has_app_context
...
key = f"{database.id}:{database.sqlalchemy_uri}"
if key not in _server_versions:
# Every label of a chart query asks for the version, so a failed
# lookup is retried on the next request rather than once per
label.
failed: set[str] = (
g.setdefault("oracle_version_lookup_failures", set())
if has_app_context()
else set()
)
if key in failed:
return None
try:
... # unchanged
except Exception: # pylint: disable=broad-except
logger.warning(
"Unable to read the Oracle server version", exc_info=True
)
failed.add(key)
return None
if not version_info:
failed.add(key)
return None
```
With this change, `test_get_limit_method_unknown_version` needs its second
call inside a new `app.app_context()`, and a test that builds 20 labels against
an unreachable database can assert a single `get_sqla_engine` call.
--
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]
