[GitHub] [superset] villebro commented on a change in pull request #18569: feat(explore): Allow using time formatter on temporal columns in data table

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

villebro commented on a change in pull request #18569:
URL: https://github.com/apache/superset/pull/18569#discussion_r799215879



##########
File path: superset-frontend/src/explore/components/DataTableControl/index.tsx
##########
@@ -97,6 +114,126 @@ export const RowCount = ({
   />
 );
 
+enum FormatPickerValue {
+  formatted,
+  epoch,
+}
+
+const FormatPicker = ({
+  onChange,
+  value,
+}: {
+  onChange: any;
+  value: FormatPickerValue;
+}) => (
+  <Radio.Group value={value} onChange={onChange}>
+    <Space direction="vertical">
+      <Radio value={FormatPickerValue.epoch}>{t('Epoch')}</Radio>
+      <Radio value={FormatPickerValue.formatted}>{t('Formatted date')}</Radio>
+    </Space>
+  </Radio.Group>
+);

Review comment:
       As it's possible for temporal columns to be in non-epoch format (e.g. Sqlite and Druid come to mind), should we call the primary option "Raw format" or "Original format"? Also, "Formatted date" might be interpreted as referring to a `DATE` type as opposed to e.g. `TIMESTAMP` or `DATETIME`, so maybe we should consider calling this "Formatted timestamp" as the formatter also displays the time aspect of the value.

##########
File path: superset-frontend/src/explore/components/DataTablesPane/index.tsx
##########
@@ -172,33 +185,25 @@ export const DataTablesPane = ({
   errorMessage?: JSX.Element;
   queriesResponse: Record<string, any>;
 }) => {
-  const [data, setData] = useState<{
-    [RESULT_TYPES.results]?: Record<string, any>[];
-    [RESULT_TYPES.samples]?: Record<string, any>[];
-  }>(NULLISH_RESULTS_STATE);
-  const [isLoading, setIsLoading] = useState({
-    [RESULT_TYPES.results]: true,
-    [RESULT_TYPES.samples]: true,
-  });
-  const [columnNames, setColumnNames] = useState<{
-    [RESULT_TYPES.results]: string[];
-    [RESULT_TYPES.samples]: string[];
-  }>({
-    [RESULT_TYPES.results]: [],
-    [RESULT_TYPES.samples]: [],
-  });
-  const [error, setError] = useState(NULLISH_RESULTS_STATE);
+  const [data, setData] = useState(getDefaultDataTablesState(undefined));
+  const [isLoading, setIsLoading] = useState(getDefaultDataTablesState(true));
+  const [columnNames, setColumnNames] = useState(getDefaultDataTablesState([]));
+  const [columnTypes, setColumnTypes] = useState(getDefaultDataTablesState([]));
+  const [error, setError] = useState(getDefaultDataTablesState(''));

Review comment:
       nice cleanup 👍 

##########
File path: superset-frontend/src/explore/components/DataTableControl/index.tsx
##########
@@ -97,6 +114,128 @@ export const RowCount = ({
   />
 );
 
+enum FormatPickerValue {
+  formatted,
+  epoch,

Review comment:
       I think these should be Pascal case
   ```suggestion
     Formatted,
     Epoch,
   ```

##########
File path: superset/viz.py
##########
@@ -258,8 +258,11 @@ def get_samples(self) -> List[Dict[str, Any]]:
                 "to_dttm": None,
             }
         )
-        df = self.get_df_payload(query_obj)["df"]  # leverage caching logic
-        return df.to_dict(orient="records")
+        payload = self.get_df_payload(query_obj)  # leverage caching logic
+        return {
+            "data": payload["df"].to_dict(orient="records"),
+            "colnames": payload["colnames"],

Review comment:
       here too (requires small change in `viz.py`)

##########
File path: superset/views/core.py
##########
@@ -449,10 +449,12 @@ def get_raw_results(self, viz_obj: BaseViz) -> FlaskResponse:
         payload = viz_obj.get_df_payload()
         if viz_obj.has_error(payload):
             return json_error_response(payload=payload, status=400)
-        return self.json_response({"data": payload["df"].to_dict("records")})
+        return self.json_response(
+            {"data": payload["df"].to_dict("records"), "colnames": payload["colnames"]}

Review comment:
       let's also add "coltypes" here so we get this feature working for legacy charts 👍 




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