[GitHub] [superset] michael-s-molina commented on a diff in pull request #20683: feat: the samples endpoint supports filters and pagination

[GitBox <gi...@apache.org>]

michael-s-molina commented on code in PR #20683:
URL: https://github.com/apache/superset/pull/20683#discussion_r922232391


##########
superset/datasets/api.py:
##########
@@ -810,7 +822,15 @@ def samples(self, pk: int) -> Response:
         """
         try:
             force = parse_boolean_string(request.args.get("force"))
-            rv = SamplesDatasetCommand(pk, force).run()
+            page = request.args.get("page")
+            per_page = request.args.get("per_page")
+            rv = SamplesDatasetCommand(
+                pk,
+                force,

Review Comment:
   ```suggestion
                   model_id=pk,
                   force=force,
   ```



##########
superset/datasets/commands/samples.py:
##########
@@ -30,40 +29,79 @@
     DatasetSamplesFailedError,
 )
 from superset.datasets.dao import DatasetDAO
+from superset.datasets.schemas import DatasetSamplesQuerySchema
 from superset.exceptions import SupersetSecurityException
-from superset.utils.core import QueryStatus
-
-logger = logging.getLogger(__name__)
+from superset.utils.core import DatasourceDict, QueryStatus
 
 
 class SamplesDatasetCommand(BaseCommand):
-    def __init__(self, model_id: int, force: bool):
+    def __init__(
+        self,
+        model_id: int,
+        force: bool,
+        *,

Review Comment:
   ```suggestion
   ```



##########
superset/datasets/commands/samples.py:
##########
@@ -78,3 +116,24 @@ def validate(self) -> None:
             security_manager.raise_for_ownership(self._model)
         except SupersetSecurityException as ex:
             raise DatasetForbiddenError() from ex
+
+    @staticmethod
+    def get_limit_clause(
+        page: Optional[int], per_page: Optional[int]
+    ) -> Dict[str, int]:
+        samples_row_limit = app.config.get("SAMPLES_ROW_LIMIT", 1000)
+        limit = samples_row_limit
+        offset = 0
+
+        if isinstance(page, int) and isinstance(per_page, int):

Review Comment:
   `isinstance` fails because `page` and `per_page` are strings in the URL, so they are being ignored.



##########
superset/datasets/commands/samples.py:
##########
@@ -30,40 +29,79 @@
     DatasetSamplesFailedError,
 )
 from superset.datasets.dao import DatasetDAO
+from superset.datasets.schemas import DatasetSamplesQuerySchema
 from superset.exceptions import SupersetSecurityException
-from superset.utils.core import QueryStatus
-
-logger = logging.getLogger(__name__)
+from superset.utils.core import DatasourceDict, QueryStatus
 
 
 class SamplesDatasetCommand(BaseCommand):
-    def __init__(self, model_id: int, force: bool):
+    def __init__(
+        self,
+        model_id: int,
+        force: bool,
+        *,
+        payload: Optional[DatasetSamplesQuerySchema] = None,
+        page: Optional[int] = None,
+        per_page: Optional[int] = None,
+    ):
         self._model_id = model_id
         self._force = force
         self._model: Optional[SqlaTable] = None
+        self._payload = payload
+        self._page = page
+        self._per_page = per_page
 
     def run(self) -> Dict[str, Any]:
         self.validate()
-        if not self._model:
-            raise DatasetNotFoundError()
+        limit_clause = self.get_limit_clause(self._page, self._per_page)
+        self._model = cast(SqlaTable, self._model)
+        datasource: DatasourceDict = {
+            "type": self._model.type,
+            "id": self._model.id,
+        }
 
-        qc_instance = QueryContextFactory().create(
-            datasource={
-                "type": self._model.type,
-                "id": self._model.id,
-            },
-            queries=[{}],
+        # constructing samples query
+        samples_instance = QueryContextFactory().create(
+            datasource=datasource,
+            queries=[
+                {**self._payload, **limit_clause} if self._payload else limit_clause
+            ],
             result_type=ChartDataResultType.SAMPLES,
             force=self._force,
         )
-        results = qc_instance.get_payload()
+
+        # constructing count(*) query
+        count_star_payload = {

Review Comment:
   We should consider the filters for the count as well because the pagination is calculated considering them.
   
   Right now, if I do a query with:
   ```
   {"filters": [{"col": "gender", "op": "==", "val": "boy"}]}
   ```
   I get the total count in `dataset_count_star` without the filters.



-- 
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