kaya-abdullah opened a new pull request, #45675: URL: https://github.com/apache/superset/pull/45675
Depends on #45309, which needs to merge first: this branch is built on it, so the first three commits here are #45309's and only the last one is new. I'll rebase onto master once it merges. @gabotorresruiz this is the follow-up for the 30-byte label limit you pointed out in https://github.com/apache/superset/pull/45309#discussion_r4288091524. ### SUMMARY Oracle releases before 12.2 limit identifiers to 30 bytes, but `OracleEngineSpec.max_column_name_length` is 128, the 12.2+ limit. So on 11g any chart whose metric or column label is longer than 30 bytes fails: SELECT region AS "region", sum(total_revenue) AS "Sum of total revenue by region and quarter" ... ORA-00972: identifier is too long The limit is in bytes, so a shorter label with non-ASCII characters fails too: `Bölge başına toplam satışlar` is 28 characters and 33 bytes in UTF-8, and 11.2.0.2 rejects it the same way. This PR gives `max_column_name_length` the same version split that `limit_method` got in #45309: - `BaseEngineSpec.get_max_column_name_length(database)` returns `max_column_name_length` by default, like `get_limit_method(database)`. - `make_label_compatible` takes an optional `database` and reads the limit from it. The five places that build labels for a query pass it, so the inner and outer names of the series limit join still match. - `OracleEngineSpec.get_max_column_name_length` returns 30 below 12.2, using the configured or detected version. A version given only as `12c` is treated as 12.1, since 30 bytes works on every release. - The version cache now keeps the full version instead of the major number, so 12.1 and 12.2 can be told apart. - Oracle counts label length in UTF-8 bytes (`_label_length`), so multibyte labels are hashed before they hit the byte limit. Other engines keep counting characters. Labels over the limit get the existing hash, cut to 30 characters, and the result columns keep the original label, as on other engines with a short limit. Nothing changes on 12.2 and later unless a label is over 128 bytes. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Superset master (`apache/superset:367b6f5-dev`) with #45309 applied, against Oracle XE 11.2.0.2, no `version` extra. Before is #45309 alone, after adds this branch. Table chart, `SUM(TOTAL_REVENUE)` by `REGION` with a custom label. `Sum of total revenue by region and quarter` Before: SCREENSHOT_1_BEFORE After: SCREENSHOT_1_AFTER `Bölge başına toplam satışlar` Before: SCREENSHOT_2_BEFORE After: SCREENSHOT_2_AFTER ### TESTING INSTRUCTIONS Unit tests: pytest tests/unit_tests/db_engine_specs/test_oracle.py tests/unit_tests/db_engine_specs/test_base.py Against a real database, both through `SqlaTable.query` and in Explore: - Oracle XE 11.2.0.2 (oracledb thick mode): the two labels above failed with ORA-00972 before and return their rows after. - Oracle 23ai: same results and same SQL before and after. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
