Antonio-RiveroMartnez commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075548118
##########
superset/utils/core.py:
##########
@@ -1764,6 +1765,31 @@ def apply_max_row_limit(
return max_limit
+def apply_max_row_limit_table(
+ limit: int,
+ form_data: dict[str, Any] | None = None,
+) -> int:
+ """
+ Override row limit for table viz type based on server pagination setting
+
+ :param limit: requested row limit
+ :param max_limit: Maximum allowed row limit for table viz
Review Comment:
This is not an param of the method
##########
superset/utils/core.py:
##########
@@ -1764,6 +1765,31 @@ def apply_max_row_limit(
return max_limit
+def apply_max_row_limit_table(
Review Comment:
Same as with `_process_row_limit`, why not extending the current one instead
of creating a table specific to handle optional param?
##########
superset/common/query_object_factory.py:
##########
@@ -105,6 +113,22 @@ def _process_row_limit(
)
return apply_max_row_limit(row_limit or default_row_limit)
+ def _process_row_limit_table(
Review Comment:
Why not just extending the existing _process_row_limit so it can receive the
optional form_data and pass it along instead of creating this specific method
for tables?
##########
superset/config.py:
##########
@@ -969,6 +969,13 @@ class D3TimeFormat(TypedDict, total=False):
# Maximum number of rows returned for any analytical database query
SQL_MAX_ROW = 100000
+# Maximum number of rows that can be returned for table without Server
pagination
+TABLE_VIZ_MAX_ROW_CLIENT = 100000
Review Comment:
I know the use case rn is for tables, but should the config be more
abstract? i.e, `SQL_MAX_ROW_CLIENT` and `SQL_MAX_ROW_SERVER` or even just use
the same `SQL_MAX_ROW ` as above (which might be confusing to keep 3 MAX
configs) and include just one `SQL_MAX_ROW_SERVER`?
##########
superset/models/helpers.py:
##########
@@ -1888,12 +1888,26 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
elif op in {
utils.FilterOperator.ILIKE.value,
utils.FilterOperator.LIKE.value,
+ utils.FilterOperator.TEXT_SEARCH.value,
}:
if target_generic_type != GenericDataType.STRING:
sqla_col = sa.cast(sqla_col, sa.String)
if op == utils.FilterOperator.LIKE.value:
where_clause_and.append(sqla_col.like(eq))
+ elif op == utils.FilterOperator.TEXT_SEARCH.value:
+ # Convert to string and handle None case
+ search_value = str(eq) if eq is not None else ""
+ if (
+ search_value
+ ): # Only add clause if search value is not empty
+ pattern = search_value.lower()
+ where_clause_and.append(
+ sa.or_(
+ sqla_col.ilike(f"{pattern}%"),
+ sqla_col.ilike(f"% {pattern}%"),
Review Comment:
Why not just using the ILIKE? and why adding the extra leading space?
##########
superset/utils/core.py:
##########
@@ -1764,6 +1765,31 @@ def apply_max_row_limit(
return max_limit
+def apply_max_row_limit_table(
+ limit: int,
+ form_data: dict[str, Any] | None = None,
+) -> int:
+ """
+ Override row limit for table viz type based on server pagination setting
+
+ :param limit: requested row limit
+ :param max_limit: Maximum allowed row limit for table viz
+ :param form_data: Form data containing server_pagination setting
+ :return: Capped row limit
+ """
+ max_limit = None
Review Comment:
nit: If you initialize `max_limit =
current_app.config["TABLE_VIZ_MAX_ROW_CLIENT"]` then you won't need the else
below
--
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]
