rebenitez1802 commented on code in PR #45801:
URL: https://github.com/apache/superset/pull/45801#discussion_r4414398514
##########
superset/models/helpers.py:
##########
@@ -5917,7 +5919,7 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
applied_adhoc_filters_columns: list[Union[str, ColumnTyping]] = []
db_engine_spec = self.db_engine_spec
series_column_labels = [
- db_engine_spec.make_label_compatible(column)
+ db_engine_spec.make_label_compatible(column,
database=self.database)
Review Comment:
🔴 High — Unit tests fail once merged with master
#45681 landed after this PR's CI run. It added
`test_cache_key_collects_series_metric_for_normalized_labels`, which mocks
`make_label_compatible` with `side_effect=lambda name: ...`. Merged with
current master, both of its parametrizations fail with `TypeError: <lambda>()
got an unexpected keyword argument 'database'`; they pass on master. The branch
merges without conflicts and the CI here is green, so this would only show up
on master. Fix: merge master and change the mock in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py` to
`lambda name, database=None: ...`.
##########
superset/db_engine_specs/oracle.py:
##########
@@ -87,6 +87,21 @@ def get_limit_method(cls, database: Database) -> LimitMethod:
return cls.limit_method
+ @classmethod
+ def get_max_column_name_length(cls, database: Database) -> int | None:
+ """Use Oracle's 30-byte identifier limit before release 12.2."""
+ server_version = cls.get_server_version(database)
Review Comment:
🔴 High — A failed version lookup is retried once per label
`make_label_compatible` now asks for the server version for every label
(`get_max_column_name_length` → `get_server_version`), and a failed lookup
isn't remembered. With no Version set and the database unreachable, building
one chart query tries to connect 17–39 times, logging a traceback each time;
master tries none while building the query. python-oracledb's default connect
timeout is 20 s, so one chart can hang for about 340 s, well past the web
server timeout. Fix: remember the failure for the current request (for example
a set on `flask.g`, as #45675 does), so a chart makes at most one attempt and
the next request still retries.
##########
superset/models/helpers.py:
##########
@@ -7201,7 +7205,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 + "__", database=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. With a
series column of 29 bytes or more on Oracle 11.2, this lookup therefore raises
`KeyError: '7e0a3e56f6524e25280e79b853efa5'` while building the query. Users
get a 500, and "View query" breaks too. The same happens when a database's
Version is set to `12c`; on a 12.2+ server set that way, this query works on
master and crashes on this branch. Looking the column up by its key fixes it,
and the 30-byte aliases stay intact:
```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 + "__"]
```
--
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]
