kaya-abdullah commented on code in PR #45675:
URL: https://github.com/apache/superset/pull/45675#discussion_r4406521136
##########
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:
Thanks, you're right. Applied your suggestion in 8d17ca5cc9, and added
test_series_limit_others_with_shortened_series_label, which fails with the
KeyError without it.
Checked on real Oracle with a 29-byte series label: 23ai with Version set to
12c now returns the Others row instead of the KeyError.
On 11.2 the same chart now gets past the KeyError but fails with ORA-00904.
That one comes from the ROWNUM wrapper renaming the subquery's long columns,
and it also happens without "Others". It's a separate bug, and I have a fix
for it that I'll open after this PR.
##########
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:
Good catch. Done in 8d17ca5cc9 the way you described: a failed lookup is
remembered
in g for the rest of the request or task, so the next one still tries again.
test_get_limit_method_unknown_version now makes its second call in a new app
context, and test_failed_version_lookup_is_tried_once_per_context builds 20
labels against an unreachable database and asserts one get_sqla_engine call.
On a real 11.2 with a wrong password and no Version set, "View query" for a
chart with 11 labels went from 32 get_sqla_engine calls and 227 seconds to 3
calls and under a second.
--
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]
