You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "amaannawab923 (via GitHub)" <gi...@apache.org> on 2025/05/05 05:32:15 UTC
[PR] Feat/increase row limit [superset]
amaannawab923 opened a new pull request, #33357:
URL: https://github.com/apache/superset/pull/33357
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075850969
##########
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:
@betodealmeida i agree , that we should maintain consistency from a ux point of view but filtering results on every possible substring of the search value absolutely breaks the table in case of analytics tables & gives random results..
Here i am searching for all real_name that includes ad
<img width="1138" alt="Screenshot 2025-05-06 at 9 53 25 PM" src="https://github.com/user-attachments/assets/8d2f1f79-a7ba-41af-92d5-da3f8c17aaed" />
& the search operation begins to feel random since Searches are supposed to be based on prefixes
An example would be linkedin search below since it has the search by name case
<img width="559" alt="Screenshot 2025-05-06 at 9 56 17 PM" src="https://github.com/user-attachments/assets/edafad33-ac10-4caa-a7a7-018c27c189ad" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075750808
##########
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:
@Antonio-RiveroMartnez keep in mind we're fitering the analytics database here, so most of them won't have indexes that can be easily leveraged.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075590173
##########
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:
I did try using ilike but ilike fails in scenarios with strings have more than one substring
For example:
In case of fullname which is usually used by most orgs
If someone wants to search by first name & last name they expect the same result
Let's take the name
"John Anderson"
You want to type both john & anderson & expect the same result
then ilike(" And"%) or ilike("Joh%")
will return the same full name and infact it goes to an unlimited number of substrings
For example:
Searching for James Carter Wilson
ilike(" Car"%)
ilike(" Wil%")
will still return James Cater wilson since the rest of the substrings also have a leading space
These test cases will fail when just using ilike
Below is a screenshot of it working in action
Where it returns both "Ahmad Andra" & "Andra Alsup" where andra is last name of the first while last name of the second respectively
<img width="1718" alt="Screenshot 2025-05-06 at 12 17 30 AM" src="https://github.com/user-attachments/assets/4cacad90-6a76-496d-bf25-1095efe2368f" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2081810359
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -127,7 +168,8 @@ export default typedMemo(function DataTable<D extends object>({
...initialState_,
// zero length means all pages, the `usePagination` plugin does not
// understand pageSize = 0
- sortBy: sortByRef.current,
+ // sortBy: sortByRef.current,
+ sortBy: serverPagination ? sortByFromParent : sortByRef.current,
Review Comment:
sortByFromParent is the sortBy state from the persistent filterState of the table chart which we need on initial Render as the table loses all of its variables on complete re render... So getting the sortBy state from redux helps maintain consistency
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075756559
##########
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:
Also, instead of passing the whole `form_data` we can just pass `server_pagination`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2860355792
@geido Ephemeral environment spinning up at http://34.221.88.197:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2079178509
##########
superset/common/query_object_factory.py:
##########
@@ -96,14 +102,27 @@ def _process_extras(
return extras
def _process_row_limit(
- self, row_limit: int | None, result_type: ChartDataResultType
+ self,
+ row_limit: int | None,
+ result_type: ChartDataResultType,
+ server_pagination: bool = False,
Review Comment:
Sure
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2081813072
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -381,16 +466,31 @@ export default typedMemo(function DataTable<D extends object>({
) : null}
</div>
{searchInput ? (
- <div className="col-sm-6">
+ <StyledSpace className="col-sm-6">
+ {serverPagination && (
+ <div className="search-select-container">
+ <span className="search-by-label">Search by: </span>
+ <SearchSelectDropdown
+ searchOptions={searchOptions}
+ value={serverPaginationData?.searchColumn || ''}
+ onChange={onSearchColChange}
+ />
+ </div>
+ )}
Review Comment:
The search select dropdown component to have dynamic search column
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075595782
##########
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:
Right
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075634832
##########
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:
Checked out pgtrgm ... but it wont suffice our case because we want a prefix match for strings & substrings & pgtrgm uses fuzzy search that means
Wilson carter will be returned for Wil & car
but it will also be returned for strings like "il" , "art" , "er"
Which isnt expected & will return wrong results
Will try to look for better approach though ... This approach i have implemented for a pvt fork which worked out greatly hence migrated it here
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2081816081
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -66,17 +92,37 @@ export default (memo as <T>(fn: T) => T)(function GlobalFilter<
200,
);
+ // Preserve focus during server-side filtering to maintain a better user experience
+ useEffect(() => {
+ if (
+ serverPagination &&
+ isSearchFocused.get(id) &&
+ document.activeElement !== inputRef.current
+ ) {
+ inputRef.current?.focus();
+ }
+ }, [value, serverPagination]);
+
+ const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
+ const target = e.target as HTMLInputElement;
+ e.preventDefault();
+ isSearchFocused.set(id, true);
+ setValue(target.value);
+ };
+
+ const handleBlur = () => {
+ isSearchFocused.set(id, false);
+ };
Review Comment:
Updating the map to inform that the field has lost focus ... and there is no need to focus it on initial render
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2079177918
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
+ server_pagination: bool = False,
) -> int:
"""
- Override row limit if max global limit is defined
+ Override row limit based on server pagination setting
:param limit: requested row limit
- :param max_limit: Maximum allowed row limit
+ :param server_pagination: whether server-side pagination is enabled
:return: Capped row limit
- >>> apply_max_row_limit(100000, 10)
- 10
- >>> apply_max_row_limit(10, 100000)
- 10
- >>> apply_max_row_limit(0, 10000)
- 10000
- """
- if max_limit is None:
Review Comment:
Because the function is no where using the max_limit from anywhere... I can add it again ... i only removed it because the argument wasn't getting used
Usages
superset/viz.py
row_limit = apply_max_row_limit(row_limit). # No second arg for max_limit
superset/datasource/api.py
row_limit = apply_max_row_limit(app.config["FILTER_SELECT_ROW_LIMIT"]) # No second arg for max_limit
superset/sqllab/sqllab_execution_context.py
limit = apply_max_row_limit(query_params.get("queryLimit") or 0) # No second arg for max_limit
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
+ server_pagination: bool = False,
Review Comment:
Sure
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075606198
##########
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:
I see, in that case, extensions like https://www.postgresql.org/docs/current/pgtrgm.html should be preferred which would have to leverage some new indexes for those fields you want to search. That's probably out of the scope of this PR itself, but using hardcoded leading spaces IMO is not something we should do
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075742917
##########
superset-frontend/packages/superset-ui-core/src/query/types/Operator.ts:
##########
@@ -33,6 +33,7 @@ const BINARY_OPERATORS = [
'NOT LIKE',
'REGEX',
'TEMPORAL_RANGE',
+ 'TEXT_SEARCH',
Review Comment:
Let's just use `ILIKE` for consistency with how search works across the app, see my first comment.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2079833512
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
+ server_pagination: bool = False,
) -> int:
"""
- Override row limit if max global limit is defined
+ Override row limit based on server pagination setting
:param limit: requested row limit
- :param max_limit: Maximum allowed row limit
+ :param server_pagination: whether server-side pagination is enabled
:return: Capped row limit
- >>> apply_max_row_limit(100000, 10)
- 10
- >>> apply_max_row_limit(10, 100000)
- 10
- >>> apply_max_row_limit(0, 10000)
- 10000
- """
- if max_limit is None:
Review Comment:
Nice, if it's not being used let's get rid of it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2079928086
##########
superset/common/query_object_factory.py:
##########
@@ -96,14 +102,27 @@ def _process_extras(
return extras
def _process_row_limit(
- self, row_limit: int | None, result_type: ChartDataResultType
+ self,
+ row_limit: int | None,
+ result_type: ChartDataResultType,
+ server_pagination: bool | None = None,
Review Comment:
❤️
##########
superset/config.py:
##########
@@ -969,6 +969,10 @@ class D3TimeFormat(TypedDict, total=False):
# Maximum number of rows returned for any analytical database query
SQL_MAX_ROW = 100000
+# Maximum number of rows for any query without Server Pagination in Table Viz type
Review Comment:
```suggestion
# Maximum number of rows for any query with Server Pagination in Table Viz type
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075758192
##########
superset/common/query_context_factory.py:
##########
@@ -70,7 +70,7 @@ def create( # pylint: disable=too-many-arguments
datasource_model_instance,
form_data,
self._query_object_factory.create(
- result_type, datasource=datasource, **query_obj
+ result_type, datasource=datasource, **query_obj, form_data=form_data
Review Comment:
```suggestion
result_type, datasource=datasource, form_data=form_data, **query_obj
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2860316935
@geido Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14892995603). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2854678409
@Antonio-RiveroMartnez Ephemeral environment spinning up at http://54.190.200.46:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075573020
##########
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:
Makes sense , since both of them have same values... We can eliminate one
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2081812301
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -198,10 +246,46 @@ export default typedMemo(function DataTable<D extends object>({
globalFilter: defaultGlobalFilter,
sortTypes,
autoResetSortBy: !isEqual(columnNames, previousColumnNames),
+ manualSortBy: !!serverPagination,
...moreUseTableOptions,
},
...tableHooks,
);
+
+ const handleSearchChange = useCallback(
+ (query: string) => {
+ if (manualSearch && onSearchChange) {
+ onSearchChange(query);
+ } else {
+ setGlobalFilter(query);
+ }
+ },
+ [manualSearch, onSearchChange, setGlobalFilter],
+ );
+
+ // updating the sort by to the own State of table viz
+ useEffect(() => {
+ const serverSortBy = serverPaginationData?.sortBy || [];
+
+ if (serverPagination && !isEqual(sortBy, serverSortBy)) {
+ if (Array.isArray(sortBy) && sortBy.length > 0) {
+ const [sortByItem] = sortBy;
+ const matchingColumn = columns.find(col => col?.id === sortByItem?.id);
+
+ if (matchingColumn && 'columnKey' in matchingColumn) {
+ const sortByWithColumnKey: SortByItem = {
+ ...sortByItem,
+ key: (matchingColumn as { columnKey: string }).columnKey,
+ };
+
+ handleSortByChange([sortByWithColumnKey]);
+ }
+ } else {
+ handleSortByChange([]);
+ }
+ }
+ }, [sortBy]);
+
Review Comment:
useEffect for updating the sortBy state in redux of the table viz & triggering query to get the latest data based on sortBy
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/increase row limit [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2849967698
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/33357?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `38.46154%` with `8 lines` in your changes missing coverage. Please review.
> Project coverage is 75.81%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`49aa28e`)](https://app.codecov.io/gh/apache/superset/commit/49aa28ef4d99846f3b01e49beff43a1dc8d74cc6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1827 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/33357?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/core.py](https://app.codecov.io/gh/apache/superset/pull/33357?src=pr&el=tree&filepath=superset%2Futils%2Fcore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | 16.66% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/33357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/common/query\_object\_factory.py](https://app.codecov.io/gh/apache/superset/pull/33357?src=pr&el=tree&filepath=superset%2Fcommon%2Fquery_object_factory.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdF9mYWN0b3J5LnB5) | 40.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/33357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #33357 +/- ##
===========================================
+ Coverage 60.48% 75.81% +15.32%
===========================================
Files 1931 553 -1378
Lines 76236 39976 -36260
Branches 8568 0 -8568
===========================================
- Hits 46114 30307 -15807
+ Misses 28017 9669 -18348
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.24% <30.76%> (-0.91%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.42% <38.46%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.70% <38.46%> (-1.10%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.81% <38.46%> (+12.30%)` | :arrow_up: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.92% <38.46%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/33357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/33357?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2074007003
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable import/no-extraneous-dependencies */
+import { styled } from '@superset-ui/core';
+import { Select } from 'antd';
+import { SearchOption } from '../../types';
+
+const StyledSelect = styled(Select)`
+ width: 120px;
+ margin-right: 8px;
+`;
+
+interface SearchSelectDropdownProps {
+ value?: string;
+ onChange: (searchCol: string) => void;
+ searchOptions: SearchOption[];
+}
+
+function SearchSelectDropdown({
+ value,
+ onChange,
+ searchOptions,
+}: SearchSelectDropdownProps) {
+ return (
+ <StyledSelect
+ value={value || searchOptions?.[0]?.value}
Review Comment:
### Unsafe Fallback Value Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The fallback value logic may fail if searchOptions is an empty array, leading to a potential undefined value being assigned.
###### Why this matters
If searchOptions is empty and no value is provided, the component will attempt to use undefined as a value for the Select component, which could cause unexpected behavior or errors.
###### Suggested change ∙ *Feature Preview*
Add a safer fallback value check:
```typescript
value={value || (searchOptions?.[0]?.value ?? '')}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5eb1b1bc-eafd-4b5c-9f51-a24e1748da37/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5eb1b1bc-eafd-4b5c-9f51-a24e1748da37?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5eb1b1bc-eafd-4b5c-9f51-a24e1748da37?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5eb1b1bc-eafd-4b5c-9f51-a24e1748da37?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5eb1b1bc-eafd-4b5c-9f51-a24e1748da37)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8ed47c1e-12ff-4272-a4fc-b7bda99f6207 -->
[](8ed47c1e-12ff-4272-a4fc-b7bda99f6207)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable import/no-extraneous-dependencies */
+import { styled } from '@superset-ui/core';
+import { Select } from 'antd';
+import { SearchOption } from '../../types';
+
+const StyledSelect = styled(Select)`
+ width: 120px;
+ margin-right: 8px;
Review Comment:
### Hard-coded style values <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Hard-coded numeric values in the styled component without clear meaning or explanation.
###### Why this matters
Magic numbers make the code less maintainable and harder to understand the purpose of specific values.
###### Suggested change ∙ *Feature Preview*
```typescript
const SELECT_WIDTH = 120;
const SELECT_MARGIN = 8;
const StyledSelect = styled(Select)`
width: ${SELECT_WIDTH}px;
margin-right: ${SELECT_MARGIN}px;
`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/404c79b1-9ee4-4e82-b9be-567ca1decf07/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/404c79b1-9ee4-4e82-b9be-567ca1decf07?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/404c79b1-9ee4-4e82-b9be-567ca1decf07?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/404c79b1-9ee4-4e82-b9be-567ca1decf07?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/404c79b1-9ee4-4e82-b9be-567ca1decf07)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0ad272ca-db93-437b-aa20-e52b2eb59f15 -->
[](0ad272ca-db93-437b-aa20-e52b2eb59f15)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable import/no-extraneous-dependencies */
+import { styled } from '@superset-ui/core';
+import { Select } from 'antd';
+import { SearchOption } from '../../types';
+
+const StyledSelect = styled(Select)`
+ width: 120px;
+ margin-right: 8px;
+`;
+
+interface SearchSelectDropdownProps {
+ value?: string;
+ onChange: (searchCol: string) => void;
+ searchOptions: SearchOption[];
+}
+
+function SearchSelectDropdown({
+ value,
+ onChange,
+ searchOptions,
+}: SearchSelectDropdownProps) {
+ return (
+ <StyledSelect
+ value={value || searchOptions?.[0]?.value}
+ options={searchOptions}
+ onChange={(value: string) => {
+ onChange(value);
+ }}
Review Comment:
### Unnecessarily verbose event handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The onChange handler is unnecessarily verbose with a wrapper arrow function that only passes the value through.
###### Why this matters
Adding unnecessary layers of function wrapping makes the code more difficult to read at a glance and adds cognitive overhead.
###### Suggested change ∙ *Feature Preview*
```typescript
onChange={onChange}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f095dcc3-7020-4d7d-b97e-f9719d924ac5/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f095dcc3-7020-4d7d-b97e-f9719d924ac5?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f095dcc3-7020-4d7d-b97e-f9719d924ac5?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f095dcc3-7020-4d7d-b97e-f9719d924ac5?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f095dcc3-7020-4d7d-b97e-f9719d924ac5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ba5796d2-7a55-4918-89ab-b3d106703933 -->
[](ba5796d2-7a55-4918-89ab-b3d106703933)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -16,34 +16,55 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { memo, ComponentType, ChangeEventHandler } from 'react';
+import {
+ memo,
+ ComponentType,
+ ChangeEventHandler,
+ useRef,
+ useEffect,
+} from 'react';
import { Row, FilterValue } from 'react-table';
import useAsyncState from '../utils/useAsyncState';
export interface SearchInputProps {
count: number;
value: string;
onChange: ChangeEventHandler<HTMLInputElement>;
+ onBlur?: () => void;
+ inputRef?: React.RefObject<HTMLInputElement>;
}
+const isSearchFocused = new Map();
Review Comment:
### Global State Map Memory Leak <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using a global Map to track focus state across multiple instances can lead to memory leaks and state persistence issues
###### Why this matters
When multiple table instances are created and destroyed, the focus states in the Map will persist, potentially causing incorrect focus behavior and memory leaks as the Map retains references to destroyed components
###### Suggested change ∙ *Feature Preview*
Move the focus state into component state using useState or into a React context if sharing between components is needed:
```typescript
function GlobalFilter<D extends object>({ ... }) {
const [isSearchFocused, setIsSearchFocused] = useState(false);
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const target = e.target as HTMLInputElement;
e.preventDefault();
setIsSearchFocused(true);
setValue(target.value);
};
const handleBlur = () => {
setIsSearchFocused(false);
};
useEffect(() => {
if (
serverPagination &&
isSearchFocused &&
document.activeElement !== inputRef.current
) {
inputRef.current?.focus();
}
}, [value]);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf8c8fe-e2d1-4163-b221-851ca94458cc/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf8c8fe-e2d1-4163-b221-851ca94458cc?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf8c8fe-e2d1-4163-b221-851ca94458cc?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf8c8fe-e2d1-4163-b221-851ca94458cc?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf8c8fe-e2d1-4163-b221-851ca94458cc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:62d6fd32-9f9f-42db-8322-01722025e799 -->
[](62d6fd32-9f9f-42db-8322-01722025e799)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/utils/externalAPIs.ts:
##########
@@ -30,3 +30,11 @@ export const updateExternalFormData = (
pageSize,
},
});
+
+export const updateTableOwnState = (
+ setDataMask: SetDataMaskHook = () => {},
Review Comment:
### Unclear default function implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The default empty function implementation obscures the purpose of the setDataMask parameter and its expected behavior.
###### Why this matters
While providing defaults can be helpful, in this case it makes it less clear that this is a required callback for updating the data mask state.
###### Suggested change ∙ *Feature Preview*
Remove the default implementation and make the parameter required:
```typescript
export const updateTableOwnState = (
setDataMask: SetDataMaskHook,
modifiedOwnState: TableOwnState,
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9685ffa4-8a01-4f9c-92e7-15a58a9b434d/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9685ffa4-8a01-4f9c-92e7-15a58a9b434d?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9685ffa4-8a01-4f9c-92e7-15a58a9b434d?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9685ffa4-8a01-4f9c-92e7-15a58a9b434d?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9685ffa4-8a01-4f9c-92e7-15a58a9b434d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2f0e5a6f-85b7-4ae9-89e4-e15cc15b8277 -->
[](2f0e5a6f-85b7-4ae9-89e4-e15cc15b8277)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/utils/externalAPIs.ts:
##########
@@ -30,3 +30,11 @@
pageSize,
},
});
+
+export const updateTableOwnState = (
+ setDataMask: SetDataMaskHook = () => {},
+ modifiedOwnState: object,
Review Comment:
### Overly permissive object type for table state <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using the generic 'object' type is too permissive and could allow invalid state updates to pass through type checking.
###### Why this matters
Without proper type constraints, the function might accept invalid state properties that could cause runtime errors or unexpected behavior in the table component.
###### Suggested change ∙ *Feature Preview*
Define a specific interface for the table state and use it to type the parameter:
```typescript
interface TableOwnState {
currentPage?: number;
pageSize?: number;
sortColumn?: string;
sortOrder?: 'asc' | 'desc';
searchText?: string;
}
export const updateTableOwnState = (
setDataMask: SetDataMaskHook = () => {},
modifiedOwnState: Partial<TableOwnState>,
) =>
setDataMask({
ownState: modifiedOwnState,
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55601b5f-a270-49de-93cd-f2dd20bf9742/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55601b5f-a270-49de-93cd-f2dd20bf9742?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55601b5f-a270-49de-93cd-f2dd20bf9742?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55601b5f-a270-49de-93cd-f2dd20bf9742?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55601b5f-a270-49de-93cd-f2dd20bf9742)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:51455ccf-29a9-4d8a-aa9a-4bbddf8dcd2c -->
[](51455ccf-29a9-4d8a-aa9a-4bbddf8dcd2c)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -66,17 +92,37 @@
200,
);
+ // Only preserve focus if search is focused for this instance
+ useEffect(() => {
+ if (
+ serverPagination &&
+ isSearchFocused.get(id) &&
+ document.activeElement !== inputRef.current
+ ) {
+ inputRef.current?.focus();
+ }
+ }, [value, id]);
Review Comment:
### Inefficient Focus Effect Dependencies <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useEffect hook runs on every value change, potentially causing unnecessary focus checks and DOM manipulations.
###### Why this matters
Frequent re-renders and DOM operations can impact performance, especially with rapid value changes during typing.
###### Suggested change ∙ *Feature Preview*
Optimize the effect dependencies to only run when necessary:
```typescript
useEffect(() => {
if (serverPagination && isSearchFocused && document.activeElement !== inputRef.current) {
inputRef.current?.focus();
}
}, [serverPagination, isSearchFocused]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5279421-9afe-447a-bfec-26922e56a880/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5279421-9afe-447a-bfec-26922e56a880?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5279421-9afe-447a-bfec-26922e56a880?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5279421-9afe-447a-bfec-26922e56a880?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5279421-9afe-447a-bfec-26922e56a880)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4d8fbec2-b6d8-4279-99d1-39d8dbbc5576 -->
[](4d8fbec2-b6d8-4279-99d1-39d8dbbc5576)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable import/no-extraneous-dependencies */
+import { styled } from '@superset-ui/core';
+import { Select } from 'antd';
+import { SearchOption } from '../../types';
+
+const StyledSelect = styled(Select)`
+ width: 120px;
+ margin-right: 8px;
+`;
+
+interface SearchSelectDropdownProps {
+ value?: string;
+ onChange: (searchCol: string) => void;
+ searchOptions: SearchOption[];
+}
Review Comment:
### Missing Interface Property Documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The interface lacks documentation explaining the purpose of its properties and their constraints.
###### Why this matters
Without clear property descriptions, other developers may misuse the component or need to investigate the implementation to understand the expected values and behaviors.
###### Suggested change ∙ *Feature Preview*
```typescript
interface SearchSelectDropdownProps {
/** The currently selected search column value */
value?: string;
/** Callback triggered when a new search column is selected */
onChange: (searchCol: string) => void;
/** Available search column options to populate the dropdown */
searchOptions: SearchOption[];
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85b419a7-5e76-4925-8bad-1bbbf50e5cab/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85b419a7-5e76-4925-8bad-1bbbf50e5cab?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85b419a7-5e76-4925-8bad-1bbbf50e5cab?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85b419a7-5e76-4925-8bad-1bbbf50e5cab?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/85b419a7-5e76-4925-8bad-1bbbf50e5cab)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:617f046b-dc7c-43f3-bb9b-d4c7a6e05aaf -->
[](617f046b-dc7c-43f3-bb9b-d4c7a6e05aaf)
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -66,17 +92,37 @@
200,
);
+ // Only preserve focus if search is focused for this instance
+ useEffect(() => {
Review Comment:
### Incomplete useEffect documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useEffect comment doesn't explain why focus preservation is needed.
###### Why this matters
Without understanding the rationale, developers might remove this seemingly unnecessary focus management code.
###### Suggested change ∙ *Feature Preview*
// Preserve focus during server-side filtering to maintain a better user experience
useEffect(() => {
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3f5b9090-40d5-4350-955a-47aa26c88960/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3f5b9090-40d5-4350-955a-47aa26c88960?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3f5b9090-40d5-4350-955a-47aa26c88960?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3f5b9090-40d5-4350-955a-47aa26c88960?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3f5b9090-40d5-4350-955a-47aa26c88960)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:42dd487b-0bff-4195-a7f4-46a5a48c0f1e -->
[](42dd487b-0bff-4195-a7f4-46a5a48c0f1e)
##########
superset/views/base.py:
##########
@@ -109,6 +109,8 @@
"JWT_ACCESS_CSRF_COOKIE_NAME",
"SQLLAB_QUERY_RESULT_TIMEOUT",
"SYNC_DB_PERMISSIONS_IN_ASYNC_MODE",
+ "TABLE_VIZ_MAX_ROW_CLIENT",
+ "TABLE_VIZ_MAX_ROW_SERVER",
Review Comment:
### Ambiguous constant names <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The newly added constants have ambiguous names that don't clearly convey their purpose - whether they represent a limit, threshold, or other value.
###### Why this matters
Developers will need to look elsewhere in the codebase to understand what these constants control, reducing code readability and maintainability.
###### Suggested change ∙ *Feature Preview*
Rename to more descriptive names like:
```python
"TABLE_VIZ_CLIENT_SIDE_ROW_LIMIT",
"TABLE_VIZ_SERVER_SIDE_ROW_LIMIT",
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ee0cc10-0f5f-4b8b-8ad1-20e7e722855b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ee0cc10-0f5f-4b8b-8ad1-20e7e722855b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ee0cc10-0f5f-4b8b-8ad1-20e7e722855b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ee0cc10-0f5f-4b8b-8ad1-20e7e722855b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ee0cc10-0f5f-4b8b-8ad1-20e7e722855b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9164ddf8-9ad5-4269-80b8-cdc27daff3c3 -->
[](9164ddf8-9ad5-4269-80b8-cdc27daff3c3)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2863174201
@Antonio-RiveroMartnez Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14908287774). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2854629511
Hey @Antonio-RiveroMartnez , Can you please review this pr whenever available
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075748447
##########
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:
Or just do a single statement:
```python
max_limit = (
current_app.config["TABLE_VIZ_MAX_ROW_SERVER"]
if form_data and form_data.get("server_pagination")
else current_app.config["TABLE_VIZ_MAX_ROW_CLIENT"]
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2079775965
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
+ server_pagination: bool = False,
) -> int:
"""
- Override row limit if max global limit is defined
+ Override row limit based on server pagination setting
:param limit: requested row limit
- :param max_limit: Maximum allowed row limit
+ :param server_pagination: whether server-side pagination is enabled
:return: Capped row limit
- >>> apply_max_row_limit(100000, 10)
- 10
- >>> apply_max_row_limit(10, 100000)
- 10
- >>> apply_max_row_limit(0, 10000)
- 10000
- """
- if max_limit is None:
Review Comment:
Oh ok ok, yeah, I'm OK with deleting if not being used
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2867358181
Merging!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/increase row limit [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2849953334
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](b50da9b0-6a3d-4800-80bf-c99b77580192)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2854797673
Taking a look now, but a few initial comments:
1. In general, any PR that changes the UI/UX significantly should go [through a discussion](https://github.com/apache/superset/issues/5602). For uncontroversial changes the process is usually quick, since you can request "lazy consensus" and people don't have to vote. I think we're OK in this one, just letting you know for the future.
2. Splitting the PR into 4 would've been better, even if it takes more time/work upfront. It makes reviews quicker and it's simpler to revert features if something goes wrong.
3. I'm a bit concerned about the performance of the `LIKE` search, and wondering if we should have optional ways of optimizing it, but let me look at the PR in details before we start worrying about this.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075741924
##########
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:
There also the question of UX here. I would expect that searching for `son` would match `Anderson`, since that's the behavior we have in other input fields in Superset:
<img width="490" alt="Screenshot 2025-05-06 at 11 32 37 AM" src="https://github.com/user-attachments/assets/38747ff4-682e-4a61-9b61-9c8039746325" />
So instead of adding a new operator here I think it's better to just use `ILIKE`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075850969
##########
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:
@betodealmeida i agree , that we should maintain consistency from a ux point of view but filtering results on every possible substring of the search value absolutely breaks the table in case of analytics tables & gives random results..
Here i am searching for all real_name that includes ad
<img width="1138" alt="Screenshot 2025-05-06 at 9 53 25 PM" src="https://github.com/user-attachments/assets/8d2f1f79-a7ba-41af-92d5-da3f8c17aaed" />
& the search operation begins to feel random since Searches are supposed to be based on prefixes
An example would be linkedin search below since it has the search by name case
<img width="564" alt="Screenshot 2025-05-06 at 10 34 05 PM" src="https://github.com/user-attachments/assets/ed43fc2c-e742-4b88-9c52-fa3e948d4b8c" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2854649301
@Antonio-RiveroMartnez Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14861335391). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2863208225
@Antonio-RiveroMartnez Ephemeral environment spinning up at http://54.187.252.12:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on PR #33357:
URL: https://github.com/apache/superset/pull/33357#issuecomment-2861854726
Resolved all the comments @betodealmeida @Antonio-RiveroMartnez
Removed the Text search operator & refactored the process row limit function as per comments & removed redundant implementation
Please re review whenever available, Thanks !
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2079170104
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
Review Comment:
The have checked the function usage, The function is not using this custom max_limit anywhere in the code ... across all of its uage .... Hence i ended up removing it since it was not being used ....
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2081815332
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -66,17 +92,37 @@ export default (memo as <T>(fn: T) => T)(function GlobalFilter<
200,
);
+ // Preserve focus during server-side filtering to maintain a better user experience
+ useEffect(() => {
+ if (
+ serverPagination &&
+ isSearchFocused.get(id) &&
+ document.activeElement !== inputRef.current
+ ) {
+ inputRef.current?.focus();
+ }
+ }, [value, serverPagination]);
Review Comment:
There was a problem that i faced with losing the focus in the search field ... as the user types in search field it triggers a complete re render & the search field focus is lost... Hence we are preserving the focus with the help of a map
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075606198
##########
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:
I see, in that case, extensions like https://www.postgresql.org/docs/current/pgtrgm.html should be preferred which would have to leverage some new indexes for those fields you want to search. That's probably out of the scope of this PR itself, but using hardcoded leading spaces IMO is not something we should do, not sure what others think tho, so I'll leave that open
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075591810
##########
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:
Thanks for pointing out , forgot to remove it while refactoring the method
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2075594806
##########
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:
Makes sense , i was just too afraid to break some existing unit test cases that might be using _process_row_limit ... Let me do that
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #33357:
URL: https://github.com/apache/superset/pull/33357
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #33357:
URL: https://github.com/apache/superset/pull/33357#discussion_r2079160787
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
+ server_pagination: bool = False,
Review Comment:
could this allow None too? so we're not forced to provide it
##########
superset/common/query_object_factory.py:
##########
@@ -96,14 +102,27 @@ def _process_extras(
return extras
def _process_row_limit(
- self, row_limit: int | None, result_type: ChartDataResultType
+ self,
+ row_limit: int | None,
+ result_type: ChartDataResultType,
+ server_pagination: bool = False,
Review Comment:
Could this be None too?
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
Review Comment:
Does this mean this method is now ignoring a custom `max_limit` and instead using the config max limits? because that's it not what it was doing before, meaning, this could be a braking change
##########
superset/utils/core.py:
##########
@@ -1741,24 +1741,29 @@ def parse_boolean_string(bool_str: str | None) -> bool:
def apply_max_row_limit(
limit: int,
- max_limit: int | None = None,
+ server_pagination: bool = False,
) -> int:
"""
- Override row limit if max global limit is defined
+ Override row limit based on server pagination setting
:param limit: requested row limit
- :param max_limit: Maximum allowed row limit
+ :param server_pagination: whether server-side pagination is enabled
:return: Capped row limit
- >>> apply_max_row_limit(100000, 10)
- 10
- >>> apply_max_row_limit(10, 100000)
- 10
- >>> apply_max_row_limit(0, 10000)
- 10000
- """
- if max_limit is None:
Review Comment:
We used to use the config values only if no max_limit was provided, why the change to always using the configs now?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org