rebenitez1802 commented on code in PR #45892:
URL: https://github.com/apache/superset/pull/45892#discussion_r4428217194
##########
superset/models/helpers.py:
##########
@@ -249,7 +249,13 @@ def build_like_predicate(
``lower(expr) LIKE lower('%term%')`` is used rather than ``ILIKE`` because
the latter is not portable across engines.
+
+ ``lower()`` is only defined for string types on strict engines (e.g.
+ PostgreSQL), so non-string expressions (numeric, boolean, date, or
+ untyped) are cast to a string first. String columns are left untouched.
"""
+ if not isinstance(expr.type, sa.String):
+ expr = sa.cast(expr, sa.String)
Review Comment:
🟡 **Medium — MSSQL: untyped calculated columns lose non-codepage characters**
Calculated columns usually have no type, so they now get `CAST(... AS
VARCHAR(max))`, which is non-Unicode. On SQL Server 2022 with the default
`SQL_Latin1_General_CP1_CI_AS` collation, `CAST(N'Ñandú 東京' AS VARCHAR(max))`
returns `Ñandú ??`. A search for `東京` on a calculated `UPPER(nvc)` column
returns 1 row on master and 0 on this branch. The `sa.Unicode()` mssql variant
in the suggestion above covers it.
##########
superset/models/helpers.py:
##########
@@ -249,7 +249,13 @@ def build_like_predicate(
``lower(expr) LIKE lower('%term%')`` is used rather than ``ILIKE`` because
the latter is not portable across engines.
+
+ ``lower()`` is only defined for string types on strict engines (e.g.
+ PostgreSQL), so non-string expressions (numeric, boolean, date, or
+ untyped) are cast to a string first. String columns are left untouched.
"""
+ if not isinstance(expr.type, sa.String):
+ expr = sa.cast(expr, sa.String)
Review Comment:
🔴 **High — Cast without a length is invalid SQL on Oracle and Db2**
`sa.cast(expr, sa.String)` has no length. Oracle renders it as `CAST(n AS
VARCHAR2)`, which fails with `ORA-00906: missing left parenthesis`. On a real
Oracle Free 23, base `lower(n) LIKE '%4%'` returns the row, and this branch
errors for NUMBER, DATE and untyped calculated columns, all of which Oracle
converts implicitly on master. Db2 (`ibm_db_sa`) renders `VARCHAR(None)`. Typed
text columns are not affected.
Giving the cast type a length where the dialect needs one fixes it:
`VARCHAR2(4000 CHAR)` on Oracle and `NVARCHAR(max)` on MSSQL (see the next
comment). Every other dialect is unchanged (`VARCHAR`/`CHAR`/`STRING`):
```suggestion
if not isinstance(expr.type, sa.String):
# A bare VARCHAR is invalid where a length is required (Oracle
renders
# VARCHAR2, Db2 VARCHAR(None)); MSSQL needs NVARCHAR to keep
characters
# outside the database code page.
cast_type = (
sa.String()
.with_variant(sa.String(4000), "oracle")
.with_variant(sa.String(32672), "ibm_db_sa")
.with_variant(sa.Unicode(), "mssql")
)
expr = sa.cast(expr, cast_type)
```
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -145,6 +145,39 @@ def
test_build_like_predicate_is_case_insensitive_and_escaped() -> None:
assert "ESCAPE" not in bigquery_sql
+def test_build_like_predicate_casts_non_text_columns() -> None:
+ import sqlalchemy as sa
+ from sqlalchemy_bigquery import BigQueryDialect
+
+ from superset.models.helpers import build_like_predicate
+
+ postgres = sa.dialects.registry.load("postgresql")()
+
+ def compile_sql(predicate: sa.ColumnElement, dialect: sa.engine.Dialect)
-> str:
+ return str(
+ predicate.compile(dialect=dialect,
compile_kwargs={"literal_binds": True})
+ )
+
+ for column_type in (sa.Numeric(), sa.Integer()):
+ sql = compile_sql(
+ build_like_predicate(sa.column("n", column_type), "0"), postgres
+ )
+ assert "lower(CAST(n AS VARCHAR))" in sql
+
+ # String columns are left untouched: no CAST is introduced.
+ text_sql = compile_sql(
+ build_like_predicate(sa.column("s", sa.String()), "a"), postgres
+ )
+ assert "lower(s)" in text_sql
+ assert "CAST" not in text_sql
+
+ # BigQuery still has no ESCAPE clause for a non-text column.
+ bigquery_sql = compile_sql(
+ build_like_predicate(sa.column("n", sa.Numeric()), "0"),
BigQueryDialect()
+ )
+ assert "ESCAPE" not in bigquery_sql
Review Comment:
🟡 **Medium — Tests miss the untyped and dialect-specific cases**
This test only compiles for Postgres and BigQuery. Changing the existing
test to `sa.column("c", sa.String())` hides the change in behavior for untyped
columns rather than asserting it, and a "skip untyped columns" mutation still
passes every test. With the fix above applied, this version passes and fails
again if the fix is reverted:
```suggestion
# Untyped expressions (e.g. calculated columns) are cast as well.
untyped_sql = compile_sql(
build_like_predicate(sa.literal_column("upper(s)"), "a"), postgres
)
assert "lower(CAST(upper(s) AS VARCHAR))" in untyped_sql
# Dialects that need a length or a unicode type get one.
oracle_sql = compile_sql(
build_like_predicate(sa.column("n", sa.Numeric()), "0"),
sa.dialects.registry.load("oracle")(),
)
assert "CAST(n AS VARCHAR2(4000 CHAR))" in oracle_sql
mssql_sql = compile_sql(
build_like_predicate(sa.column("n", sa.Numeric()), "0"),
sa.dialects.registry.load("mssql")(),
)
assert "CAST(n AS NVARCHAR(max))" in mssql_sql
# BigQuery still has no ESCAPE clause for a non-text column.
bigquery_sql = compile_sql(
build_like_predicate(sa.column("n", sa.Numeric()), "0"),
BigQueryDialect()
)
assert "ESCAPE" not in bigquery_sql
```
--
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]
