You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/15 23:39:23 UTC
[GitHub] [superset] codyml opened a new pull request, #20728: feature(dashboard): Drill to detail modal
codyml opened a new pull request, #20728:
URL: https://github.com/apache/superset/pull/20728
- [x] Merged in @zhaoyongjie's #20683 and did some quick/dirty fixes so pagination is supported server-side
- [x] Used @zhaoyongjie's #20697 as a starting point, but created a new container component `DatasourceResultsPane` that replaces `SamplesPane` but calls much of the same view logic
- [x] Got server-paginated results to load
- [ ] Implement some loading states and caching for pages
- [ ] Create controls bar with clearable filters on the left and other widgets on the right (see `DatasourceFilterBar` stub)
- [ ] Trigger from the viz plugins
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943465653
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-MAX_CACHED_PAGES + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [datasourceId, datasourceType, filters, pageIndex, resultsPages]);
+
+ // this is to preserve the order of the columns, even if there are integer values,
+ // while also only grabbing the first column's keys
+ const columns = useTableColumns(
+ resultsPage?.colNames,
+ resultsPage?.colTypes,
+ resultsPage?.data,
+ datasource,
+ );
+
+ const sortDisabledColumns = columns.map(column => ({
+ ...column,
+ disableSortBy: true,
+ }));
+
+ // Update page index on pagination click
+ const onServerPagination = useCallback(({ pageIndex }) => {
+ setPageIndex(pageIndex);
+ }, []);
+
+ // Clear cache on reload button click
+ const handleReload = useCallback(() => {
+ setResultsPages(new Map());
Review Comment:
This component has too many `new Map()`, If performance is not considered, at least when I read the code, I don't know exactly which **cache object** is being manipulated. ---- we can be optimized later after the PR merged.
--
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
[GitHub] [superset] codyml commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1211218060
> @codyml Thanks for the new feature, I have tested this feature in my local. there are 2 issues.
>
> 1. The drill detail modal always sends `force query`. the `force query` means that don't use data cache in the backend, so every force query will send actual SQL to the underlying database, usually, it's an "expensive" operation.
>
> 
>
> 2. The drill detail request is 2 times when I click` drill to detail`.
>
> drill.to.detail.mov
@zhaoyongjie Thanks for looking it over again. I fixed (2), so it should only send one request. For (1), do you think we should have `force=false` for all of these requests? Should we be worried about getting stale data? I could also have it only do `force=true` if you click the reload button in the upper right of the modal.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r925756388
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
Review Comment:
Unless it's against style guidelines I like to do an empty line after blocks for readability.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r925754767
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
+ const [colnames, setColnames] = useState<string[]>([]);
+ const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [responseError, setResponseError] = useState<string>('');
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [page, setPage] = useState(0);
+ const [sortBy, setSortBy] = useState([]);
+ const datasourceId = useMemo(
+ () => `${datasource.id}__${datasource.type}`,
+ [datasource],
+ );
+
+ useEffect(() => {
+ pageResponses.current = {};
+ }, [datasource, filters, sortBy]);
+
+ useEffect(() => {
+ const getPageData = async () => {
+ try {
+ setIsLoading(true);
+ let pageResponse = pageResponses.current[page];
+ if (!pageResponse) {
Review Comment:
Sure, if @kasiazjc is okay with it I can re-add the refresh button and have it clear the cache and refresh the current page?
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r939967940
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
Review Comment:
The `TableView` has a property `initialPageIndex`, this property means which page is the first page when pagination. BTW, in the pagination context, the `index` means `page number` instead of `index`, so maybe the first page is 1.
https://github.com/apache/superset/blob/e214e1ace616c3fdd40fcf64c501e08407feb8b3/superset-frontend/src/components/TableView/TableView.tsx#L118-L139
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929928021
##########
superset-frontend/src/dashboard/components/DrillDetailPane/index.ts:
##########
@@ -0,0 +1,22 @@
+/**
+ * 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.
+ */
+
+import DrillDetailPane from './DrillDetailPane';
+
+export default DrillDetailPane;
Review Comment:
Oh nice, didn't know you could do this! 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
[GitHub] [superset] github-actions[bot] commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1222228896
@geido Ephemeral environment spinning up at http://54.245.172.237: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
[GitHub] [superset] JunTech commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
JunTech commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1236600622
how to use in 2.0.0 branch
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943943056
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -122,7 +122,6 @@ export default function EchartsMixedTimeseries({
const filters: QueryObjectFilterClause[] = [];
filters.push({
col: formData.granularitySqla,
- grain: formData.timeGrainSqla,
Review Comment:
Reverted these changes in this PR pending solution
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r925754767
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
+ const [colnames, setColnames] = useState<string[]>([]);
+ const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [responseError, setResponseError] = useState<string>('');
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [page, setPage] = useState(0);
+ const [sortBy, setSortBy] = useState([]);
+ const datasourceId = useMemo(
+ () => `${datasource.id}__${datasource.type}`,
+ [datasource],
+ );
+
+ useEffect(() => {
+ pageResponses.current = {};
+ }, [datasource, filters, sortBy]);
+
+ useEffect(() => {
+ const getPageData = async () => {
+ try {
+ setIsLoading(true);
+ let pageResponse = pageResponses.current[page];
+ if (!pageResponse) {
Review Comment:
Sure, if @kasiazjc is okay with it I can re-add the refresh button in the header and have it clear the cache and refresh the current page?
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929919311
##########
superset-frontend/src/components/Chart/chartAction.js:
##########
@@ -603,8 +603,13 @@ export const getDatasourceSamples = async (
datasourceId,
force,
jsonPayload,
+ pagination,
) => {
- const endpoint = `/datasource/samples?force=${force}&datasource_type=${datasourceType}&datasource_id=${datasourceId}`;
+ let endpoint = `/datasource/samples?force=${force}&datasource_type=${datasourceType}&datasource_id=${datasourceId}`;
+ if (pagination) {
+ endpoint += `&page=${pagination.page}&per_page=${pagination.perPage}`;
+ }
+
try {
const response = await SupersetClient.post({ endpoint, jsonPayload });
Review Comment:
Oh great, this is much cleaner 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
[GitHub] [superset] codyml commented on pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1196849417
Closed to refocus on additional work required given clarified requirements.
--
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
[GitHub] [superset] codyml commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1212282103
> For the ideal, the `force` is sent when the `force refresh` is selected in three-dot menu. IMO, this can be optimized later, just keep `force=false` for all of requests.
@zhaoyongjie Sounds good, 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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943746649
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx:
##########
@@ -77,7 +77,7 @@ export default function EchartsGauge({
const pointerEvent = e.event.event;
const filters: QueryObjectFilterClause[] = [];
if (groupby.length > 0) {
- const values = e.name.split(',');
Review Comment:
Thanks, I'll drop the commit that added these fixes from this PR.
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943388262
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -122,7 +122,6 @@ export default function EchartsMixedTimeseries({
const filters: QueryObjectFilterClause[] = [];
filters.push({
col: formData.granularitySqla,
- grain: formData.timeGrainSqla,
Review Comment:
We definitely need the grain because that's needed to interpret the timestamp. So we need to investigate why it's breaking.
--
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
[GitHub] [superset] geido commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
geido commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1211968566
Some more comments from manual testing:
- When resizing the modal, the pagination should stick to the bottom. Also, there are 2 vertical scrolls there. I believe we should only have 1 vertical scroll
- Let's set a minimum size for the modal as the user should never resize it to the point that is unusable
- When refreshing, should we move the user to page 1? I am thinking of an edge case where we have 2 pages initially, but a bunch of data is deleted or the underlying dataset has changed, now you only have 1 page, but the request will still try to look for page 2 causing a scenario like the one depicted below
- In the example above you can also see some weird layout for the no results which needs improvement
- I think we can consider this problem for a next phase, but I believe the user should be able to select exclusively all the content of the table without selecting any other area of the application
--
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
[GitHub] [superset] kasiazjc commented on a diff in pull request #20728: feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r926532748
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
+ const [colnames, setColnames] = useState<string[]>([]);
+ const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [responseError, setResponseError] = useState<string>('');
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [page, setPage] = useState(0);
+ const [sortBy, setSortBy] = useState([]);
+ const datasourceId = useMemo(
+ () => `${datasource.id}__${datasource.type}`,
+ [datasource],
+ );
+
+ useEffect(() => {
+ pageResponses.current = {};
+ }, [datasource, filters, sortBy]);
+
+ useEffect(() => {
+ const getPageData = async () => {
+ try {
+ setIsLoading(true);
+ let pageResponse = pageResponses.current[page];
+ if (!pageResponse) {
Review Comment:
Let's do 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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r928470301
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
Let's find a way to dynamically calculate the length of LRU(or just a LIFO and fixed-size cache). I open a [PR](https://github.com/apache/superset/pull/20841/files) for this. The samples_row_limit could be accessed by `common.conf.SAMPLES_ROW_LIMIT` in the Redux store. the length of the cache might be set to `ceil(SAMPLES_ROW_LIMIT / PAGE_SIZE)` --- This means that the memory consumption of `DrillDetailPane` and `SamplesPane` is approximately the same, in the other words, if the `SamplesPane` is work then the `DrillDetial` is work as well.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929922773
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
Review Comment:
I'm using 0-indexed page indices in front-end code because the `TableView` component does and I felt like consistency with that was more important – that okay?
--
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
[GitHub] [superset] codyml commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1222554228
All issues should now be fixed, with the exception of D2D on values containing commas, which is being worked on in separate PRs (#21151, #21124).
--
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
[GitHub] [superset] jinghua-qa commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1222674016
Dashboard Regression Test LGTM
--
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
[GitHub] [superset] jinghua-qa merged pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
jinghua-qa merged PR #20728:
URL: https://github.com/apache/superset/pull/20728
--
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
[GitHub] [superset] geido commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943432632
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,275 @@
+/**
+ * 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.
+ */
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import { useSelector } from 'react-redux';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+ QueryFormData,
+ JsonObject,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+import { getDrillPayload } from './utils';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+
+export default function DrillDetailPane({
+ formData,
+ initialFilters,
+}: {
+ formData: QueryFormData;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ const SAMPLES_ROW_LIMIT = useSelector(
+ (state: { common: { conf: JsonObject } }) =>
+ state.common.conf.SAMPLES_ROW_LIMIT,
+ );
+
+ // Extract datasource ID/type from string ID
+ const [datasourceId, datasourceType] = useMemo(
+ () => formData.datasource.split('__'),
+ [formData.datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
Review Comment:
Would you mind moving all the useEffect before rendering at the bottom?
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,275 @@
+/**
+ * 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.
+ */
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import { useSelector } from 'react-redux';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+ QueryFormData,
+ JsonObject,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+import { getDrillPayload } from './utils';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+
+export default function DrillDetailPane({
+ formData,
+ initialFilters,
+}: {
+ formData: QueryFormData;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ const SAMPLES_ROW_LIMIT = useSelector(
+ (state: { common: { conf: JsonObject } }) =>
+ state.common.conf.SAMPLES_ROW_LIMIT,
+ );
+
+ // Extract datasource ID/type from string ID
+ const [datasourceId, datasourceType] = useMemo(
+ () => formData.datasource.split('__'),
+ [formData.datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ const cachePageLimit = Math.ceil(SAMPLES_ROW_LIMIT / PAGE_SIZE);
+ useEffect(() => {
+ if (!isLoading && !resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ const jsonPayload = getDrillPayload(formData, filters);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ jsonPayload,
+ PAGE_SIZE,
+ pageIndex + 1,
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-cachePageLimit + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [
+ cachePageLimit,
+ datasourceId,
+ datasourceType,
+ filters,
+ formData,
+ isLoading,
+ pageIndex,
+ resultsPages,
+ ]);
+
+ // this is to preserve the order of the columns, even if there are integer values,
+ // while also only grabbing the first column's keys
+ const columns = useTableColumns(
+ resultsPage?.colNames,
+ resultsPage?.colTypes,
+ resultsPage?.data,
+ formData.datasource,
+ );
+
+ const sortDisabledColumns = columns.map(column => ({
+ ...column,
+ disableSortBy: true,
+ }));
+
+ // Update page index on pagination click
+ const onServerPagination = useCallback(({ pageIndex }) => {
+ setPageIndex(pageIndex);
+ }, []);
+
+ // Clear cache on reload button click
+ const handleReload = useCallback(() => {
+ setResultsPages(new Map());
+ }, []);
+
+ let tableContent = null;
+
+ if (responseError) {
+ // Render error if page download failed
+ tableContent = (
+ <div
Review Comment:
I believe we can shorten this part a little bit as effectively the following bit of code is repeated multiple times below
```
<div
css={css`
height: ${theme.gridUnit * 128}px;
`}
>
```
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r947077029
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,258 @@
+/**
+ * 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.
+ */
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import { useSelector } from 'react-redux';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+ QueryFormData,
+ JsonObject,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+import { getDrillPayload } from './utils';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+
+export default function DrillDetailPane({
+ formData,
+ initialFilters,
+}: {
+ formData: QueryFormData;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ const SAMPLES_ROW_LIMIT = useSelector(
+ (state: { common: { conf: JsonObject } }) =>
+ state.common.conf.SAMPLES_ROW_LIMIT,
+ );
+
+ // Extract datasource ID/type from string ID
+ const [datasourceId, datasourceType] = useMemo(
+ () => formData.datasource.split('__'),
+ [formData.datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // this is to preserve the order of the columns, even if there are integer values,
+ // while also only grabbing the first column's keys
+ const columns = useTableColumns(
+ resultsPage?.colNames,
+ resultsPage?.colTypes,
+ resultsPage?.data,
+ formData.datasource,
+ );
+
+ // Disable sorting on columns
+ const sortDisabledColumns = useMemo(
+ () =>
+ columns.map(column => ({
+ ...column,
+ disableSortBy: true,
+ })),
+ [columns],
+ );
+
+ // Update page index on pagination click
+ const onServerPagination = useCallback(({ pageIndex }) => {
+ setPageIndex(pageIndex);
+ }, []);
+
+ // Clear cache on reload button click
+ const handleReload = useCallback(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, []);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!isLoading && !resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ const jsonPayload = getDrillPayload(formData, filters);
+ const cachePageLimit = Math.ceil(SAMPLES_ROW_LIMIT / PAGE_SIZE);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ false,
+ jsonPayload,
+ PAGE_SIZE,
+ pageIndex + 1,
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-cachePageLimit + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
Review Comment:
From @geido: when request fails, it re-attempts forever.
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943464109
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
Review Comment:
This component has too many `new Map()`, If performance is not considered, at least when I read the code, I don't know exactly which **cache object** is being manipulated. ---- can be optimized later.
--
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
[GitHub] [superset] zhaoyongjie commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1211494617
/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DRILL_TO_DETAIL=true
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r927785091
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
I understand your point - want to be able to cache the viewed samples, but this cache can become infinitely(Equivalent to downloading all records in the browser) large, and the browser may crash. Do we consider not caching here?(Not working is better than being slow), or setting a length for the map(like a simple LRU).
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r928470301
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
Let's find a way to dynamically calculate the length of LRU. I open a [PR](https://github.com/apache/superset/pull/20841/files) for this. The samples_row_limit could be accessed by `common.conf.SAMPLES_ROW_LIMIT` in the Redux store. the length of the cache might be set to `floor(SAMPLES_ROW_LIMIT / PAGE_SIZE)` --- This means that the memory consumption of `DrillDetailPane` and `SamplesPane` is approximately the same.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r938262158
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
Review Comment:
I tried to use the new LRU class but I had trouble because it doesn't support the way I was trying to ensure consistent state updates by treating the `Map` as immutable and setting state with duplicate objects. I'm not sure if we'd actually see any inconsistencies as a result but I'd rather use a solution that supports immutable use if possible.
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
Review Comment:
I tried to use the new LRU class but I had trouble because it doesn't support the way I was trying to ensure consistent state updates by treating the `Map` as immutable and setting state with duplicate objects. I'm not sure if we'd actually see any inconsistencies as a result but I'd rather use a solution that supports immutable-style use if possible.
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1210337131
@zhaoyongjie Ephemeral environment spinning up at http://52.24.26.123: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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r942916256
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx:
##########
@@ -77,7 +77,7 @@ export default function EchartsGauge({
const pointerEvent = e.event.event;
const filters: QueryObjectFilterClause[] = [];
if (groupby.length > 0) {
- const values = e.name.split(',');
Review Comment:
@michael-s-molina This was breaking for single values that had commas in them - does my replacement make sense?
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r942916799
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -122,7 +122,6 @@ export default function EchartsMixedTimeseries({
const filters: QueryObjectFilterClause[] = [];
filters.push({
col: formData.granularitySqla,
- grain: formData.timeGrainSqla,
Review Comment:
@michael-s-molina For some reason, having the grain in the filter object made it return no results. Removing it fixed it but let me know if you think this will cause other problems.
--
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
[GitHub] [superset] zhaoyongjie commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1211526034
> > @codyml Thanks for the new feature, I have tested this feature in my local. there are 2 issues.
> >
> > 1. The drill detail modal always sends `force query`. the `force query` means that don't use data cache in the backend, so every force query will send actual SQL to the underlying database, usually, it's an "expensive" operation.
> >
> > 
> >
> > 2. The drill detail request is 2 times when I click` drill to detail`.
> >
> > drill.to.detail.mov
>
> @zhaoyongjie Thanks for looking it over again. I fixed (2), so it should only send one request. For (1), do you think we should have `force=false` for all of these requests? Should we be worried about getting stale data? I could also have it only do `force=true` if you click the reload button in the upper right of the modal.
For the ideal, the `force` is sent when the `force refresh` is selected in three-dot menu. IMO, this can be optimized later, just keep `force=false` for all of requests.
--
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
[GitHub] [superset] codyml commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1238233299
> how to use in 2.0.0 branch
Hi @JunTech! This feature wasn't included in the 2.0.0 release, so using it would require running Superset from your own non-release branch that includes this commit (and previous supporting commits), then enabling the `DRILL_TO_DETAIL` feature flag. This feature is very much still a work-in-progress and you're likely to run into incomplete support for viz plugins and the potential for bugs when enabled. It'll be included in a release when it's more complete!
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r927756823
##########
superset-frontend/packages/superset-ui-core/src/query/types/Query.ts:
##########
@@ -32,23 +32,30 @@ import { PostProcessingRule } from './PostProcessing';
import { JsonObject } from '../../connection';
import { TimeGranularity } from '../../time-format';
-export type QueryObjectFilterClause = {
+export type BaseQueryObjectFilterClause = {
col: QueryFormColumn;
grain?: TimeGranularity;
isExtra?: boolean;
Review Comment:
the grain and isExtra(if the column is a time column and needs grain) are still available to us, please do not change them. .....or we can refactor this part later.
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r927785091
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
I understand your point - want to be able to cache the viewed samples, but this cache can become infinitely large, and the browser may crash. Do we consider not caching here?(Not working is better than being slow), or setting a length for the map(like a simple LRU).
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r927982777
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
Review Comment:
Oops, missed that part in the wiki – will change. For the component name, I'll change it to `DrillDetailPane` like you suggested before if you think that still works.
--
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
[GitHub] [superset] geido commented on a diff in pull request #20728: feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r925655214
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
+ const [colnames, setColnames] = useState<string[]>([]);
+ const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [responseError, setResponseError] = useState<string>('');
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [page, setPage] = useState(0);
Review Comment:
I think page should start from 1
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
Review Comment:
```suggestion
```
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
Review Comment:
Since this is only used once I believe it makes sense to define its styles inline using `css` from Emotion. Have a look at this https://github.com/apache/superset/wiki/Emotion-Styling-Guidelines-and-Best-Practices
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
+ const [colnames, setColnames] = useState<string[]>([]);
+ const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [responseError, setResponseError] = useState<string>('');
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [page, setPage] = useState(0);
+ const [sortBy, setSortBy] = useState([]);
+ const datasourceId = useMemo(
+ () => `${datasource.id}__${datasource.type}`,
+ [datasource],
+ );
+
+ useEffect(() => {
+ pageResponses.current = {};
+ }, [datasource, filters, sortBy]);
+
+ useEffect(() => {
+ const getPageData = async () => {
+ try {
+ setIsLoading(true);
+ let pageResponse = pageResponses.current[page];
+ if (!pageResponse) {
Review Comment:
Should we consider the ability to refresh the cache? Users tend to work on different tabs and might expect to see updated data without having to hard refresh
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r928485842
##########
superset-frontend/src/components/Chart/chartAction.js:
##########
@@ -603,8 +603,13 @@ export const getDatasourceSamples = async (
datasourceId,
force,
jsonPayload,
+ pagination,
) => {
- const endpoint = `/datasource/samples?force=${force}&datasource_type=${datasourceType}&datasource_id=${datasourceId}`;
+ let endpoint = `/datasource/samples?force=${force}&datasource_type=${datasourceType}&datasource_id=${datasourceId}`;
+ if (pagination) {
+ endpoint += `&page=${pagination.page}&per_page=${pagination.perPage}`;
+ }
+
try {
const response = await SupersetClient.post({ endpoint, jsonPayload });
Review Comment:
In order to keep 1) the interface consistent(API call is same as function call), and 2) avoid join strings that are too long, we do a little refactoring.
```suggestion
perPage,
page,
) => {
try {
const searchParams = {
datasource_type: datasourceType,
datasource_id: datasourceId,
force,
};
if (isDefined(perPage) && isDefined(page)) {
searchParams.per_page = perPage;
searchParams.page = page;
}
const response = await SupersetClient.post({
endpoint: '/datasource/samples',
jsonPayload,
searchParams,
});
```
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-MAX_CACHED_PAGES + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [datasourceId, datasourceType, filters, pageIndex, resultsPages]);
+
+ // this is to preserve the order of the columns, even if there are integer values,
+ // while also only grabbing the first column's keys
+ const columns = useTableColumns(
+ resultsPage?.colNames,
+ resultsPage?.colTypes,
+ resultsPage?.data,
+ datasource,
+ );
+
+ const sortDisabledColumns = columns.map(column => ({
+ ...column,
+ disableSortBy: true,
+ }));
+
+ // Update page index on pagination click
+ const onServerPagination = useCallback(({ pageIndex }) => {
+ setPageIndex(pageIndex);
+ }, []);
+
+ // Clear cache on reload button click
+ const handleReload = useCallback(() => {
+ setResultsPages(new Map());
Review Comment:
free memory explicitly, don't rely on GC.
```suggestion
resultsPages.clear();
```
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
Review Comment:
```suggestion
const perPage = 50;
```
##########
superset-frontend/src/dashboard/components/DrillDetailPane/index.ts:
##########
@@ -0,0 +1,22 @@
+/**
+ * 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.
+ */
+
+import DrillDetailPane from './DrillDetailPane';
+
+export default DrillDetailPane;
Review Comment:
```suggestion
export { default as DrillDetailPane } from './DrillDetailPane';
```
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
Review Comment:
let's use [LRU](https://github.com/apache/superset/pull/20842) to refactor this.
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -39,6 +45,8 @@ import ModalTrigger from 'src/components/ModalTrigger';
import Button from 'src/components/Button';
import ViewQueryModal from 'src/explore/components/controls/ViewQueryModal';
import { ResultsPaneOnDashboard } from 'src/explore/components/DataTablesPane';
+import Modal from 'src/components/Modal';
+import DrillDetailPane from 'src/dashboard/components/DrillDetailPane';
Review Comment:
```suggestion
import { DrillDetailPane } from 'src/dashboard/components/DrillDetailPane';
```
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-MAX_CACHED_PAGES + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [datasourceId, datasourceType, filters, pageIndex, resultsPages]);
Review Comment:
the `id` and `type` was derived from datasource
```suggestion
}, [datasource, filters, pageIndex, resultsPages]);
```
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
Review Comment:
```
const [pageIndex, setPageIndex] = useState(1);
```
--
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
[GitHub] [superset] zhaoyongjie commented on pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1194867520
> I believe the samples pane in Explore should also account for these filters. If I set is_software_dev = 1 I don't expect to see samples where this condition is not true.
It's not true. The `SamplesPane` implies **sampling the data source** and has no relation to the chart. V1 API is at [here](https://github.com/apache/superset/blob/6b0c3032b28a606fedefa7552823c6e18a95a563/superset/common/query_actions.py#L144), legacy API is at [here](https://github.com/apache/superset/blob/6b0c3032b28a606fedefa7552823c6e18a95a563/superset/viz.py#L252).
-----------
It is a point worth discussing why the original implementation did not reset the `filter` fields in the `QueryObject`? IMO, It's a bug.
The filters should be applied `where clause` or `having clause` in the query. The `metrics` has been reset on the `QueryObject`, so we did not have a chance to send a query with `having clause`.
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r928512782
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
Review Comment:
```suggestion
const [pageIndex, setPageIndex] = useState(1);
```
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
Review Comment:
```suggestion
const [pageIndex, setPageIndex] = useState(1);
```
--
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
[GitHub] [superset] michael-s-molina commented on pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1199415055
Can we display the modal with a fixed height/width? I find it weird that it starts small and grows after the results are rendered.
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r933330936
##########
superset/common/query_actions.py:
##########
@@ -162,6 +162,27 @@ def _get_samples(
return _get_full(query_context, query_obj, force_cached)
+def _get_deill_detail(
Review Comment:
typo....will fix.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929925748
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-MAX_CACHED_PAGES + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [datasourceId, datasourceType, filters, pageIndex, resultsPages]);
Review Comment:
I'd rather keep it explicit here if that's okay, just to make it easier for linters to catch errors if other parts of the code change.
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943730393
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx:
##########
@@ -77,7 +77,7 @@ export default function EchartsGauge({
const pointerEvent = e.event.event;
const filters: QueryObjectFilterClause[] = [];
if (groupby.length > 0) {
- const values = e.name.split(',');
Review Comment:
Ok. I'll create ticket/PR for 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
[GitHub] [superset] github-actions[bot] commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1211496854
@zhaoyongjie Ephemeral environment spinning up at http://35.92.54.239: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
[GitHub] [superset] zhaoyongjie commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1211554410
I made a new [Dashboard](http://35.92.54.239:8080/superset/dashboard/11/?native_filters_key=-bblsJtUTdJayrXOD2hqXBr8HSYTeFNuCbEMxPaXpJ_qlenMFnhUv4gLseAWOO7-) on the ephemeral environment.
The time filter on the drill payload looks incorrect because the `time filter` is different from other filters in the current filter design. Ideally, these things should be applied under the filters field, but this part still needs a lot of work.
The drill-detail payload has such fields.
```
{
extras: { time_grain_sqla } <---- how to aggregate time column, for example: P1Y('2022-03-01') => '2022-01-01'
granularity <----- used for time column
time_range <----- used for time value, it's a pairs value, like the `between` value in SQL
}
```
Fortunately, the `time_range` field supports lots of `time expression`. The `time value(timestamp represented by integer)` and `time_grain_sqla` should be converted to `time expression`. For example, the time value is `1643673600000` and time_grain_sqla is `P1Y`.
1. convert the time value from the integer to the time string,
2. apply time expression into time string and get a `time_range` value
```
datetrunc(datetime('2022-02-01'), year) : lastday(datetime('2019-02-01'), year)
```
https://user-images.githubusercontent.com/2016594/184063120-78ea29a1-5f5d-40f7-a81c-d6260313fb79.mov
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943723253
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx:
##########
@@ -77,7 +77,7 @@ export default function EchartsGauge({
const pointerEvent = e.event.event;
const filters: QueryObjectFilterClause[] = [];
if (groupby.length > 0) {
- const values = e.name.split(',');
Review Comment:
@michael-s-molina It was for values with commas inside – the one it was breaking on was in the FCC New Coder Survey 2018 dashboard, Minority Status (in their city) chart, `Yes, an ethnic minority` value. If you could fix this in a separate PR that would be great, 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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943387450
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx:
##########
@@ -77,7 +77,7 @@ export default function EchartsGauge({
const pointerEvent = e.event.event;
const filters: QueryObjectFilterClause[] = [];
if (groupby.length > 0) {
- const values = e.name.split(',');
Review Comment:
If you mean that we have single values with a dangling comma at the end, then your code will fix it. However, if we have values with commas then we need to use a different separator. Let me know which one is it and if it's the separator case then I can open a PR to fix it for all 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
[GitHub] [superset] michael-s-molina commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1222633049
> All issues should now be fixed, with the exception of D2D on values containing commas, which is being worked on in separate PRs (#21151, #21124).
@codyml Both PRs are merged in case you want to rebase.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feature(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r940368670
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
Review Comment:
@zhaoyongjie I'm having trouble using `initialPageIndex` to tell `TableView` to use 1-indexed page numbers: it seems like for server pagination, that parameter is used to set the current page number instead, and `TableView` expects the first page index to be `0`. If you can figure out how to do it without rewriting `TableView` to expect 1-indexed page numbers, feel free to push a commit.
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1223291926
Ephemeral environment shutdown and build artifacts deleted.
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r927762353
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
+ Map<
+ number,
+ {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+ }
+ >
+ >(new Map());
+
+ // Get string identifier for dataset
+ const datasourceId = useMemo(
+ () => `${datasource.id}__${datasource.type}`,
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Download page of results if not already in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasource.type,
+ datasource.id,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...resultsPages.entries(),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [
+ datasource.id,
+ datasource.type,
+ filters,
+ pageIndex,
+ resultsPage,
+ resultsPages,
+ ]);
+
+ // this is to preserve the order of the columns, even if there are integer values,
+ // while also only grabbing the first column's keys
+ const columns = useTableColumns(
+ resultsPage?.colNames,
+ resultsPage?.colTypes,
+ resultsPage?.data,
+ datasourceId,
+ );
+
+ const sortDisabledColumns = columns.map(column => ({
+ ...column,
+ disableSortBy: true,
+ }));
+
+ // Update page index on pagination click
+ const onServerPagination = useCallback(({ pageIndex }) => {
+ setPageIndex(pageIndex);
+ }, []);
+
+ // Clear cache on reload button click
+ const handleReload = useCallback(() => {
+ setResultsPages(new Map());
+ }, []);
+
+ // Render error if page download failed
+ if (responseError) {
+ return (
+ <pre
+ css={css`
+ margin-top: ${theme.gridUnit * 4}px;
+ `}
+ >
+ {responseError}
+ </pre>
+ );
+ }
+
+ // Render loading if first page hasn't loaded
+ if (!resultsPages.size) {
+ return (
+ <div
+ css={css`
+ height: ${theme.gridUnit * 25}px;
+ `}
+ >
+ <Loading />
+ </div>
+ );
+ }
+
+ // Render empty state if no results are returned for page
+ if (resultsPage?.total === 0) {
+ const title = t('No rows were returned for this dataset');
+ return <EmptyStateMedium image="document.svg" title={title} />;
+ }
+
+ // Render chart if at least one page has successfully loaded
+ return (
+ <div
+ css={css`
+ display: flex;
+ flex-direction: column;
+ `}
+ >
+ <TableControls
+ filters={filters}
+ setFilters={setFilters}
+ totalCount={resultsPage?.total}
+ onReload={handleReload}
+ />
+ <div>
Review Comment:
`<div>...</div>` can be removed
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
I understand your point - want to be able to cache the viewed samples, but this cache can become infinitely large, and the browser may crash. Do we consider not caching here?(Not working is better than being slow), or setting a length for the map.
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
Review Comment:
For the naming, the ResultPane has used for the `results` tab, so we have to pick a new name for this.
Another nitpicky request is that instead of writing logic in the `index.[t|j]sx?`, we should create a new file where the index file only export modules.
--
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
[GitHub] [superset] michael-s-molina commented on pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1194438712
Hi @codyml. We need to account for charts and dashboard configurations when displaying the drilling details panel. One example is the chart below:
<img width="1779" alt="Screen Shot 2022-07-25 at 2 56 01 PM" src="https://user-images.githubusercontent.com/70410625/180842990-77c4957c-90b6-4b03-9d10-c68657cd99b0.png">
In this chart, we should consider the controls configuration that impacts the query like `is_software_dev = 1`, `last_yr_income <= 200000`, and `time range = 2018-03-01 <= col < 2022-07-25`. We also need consider that each visualization has different controls.
I believe the samples pane in Explore should also account for these filters. If I set `is_software_dev = 1` I don't expect to see samples where this condition is not true.
The drilling details panel should also be influenced by dashboard configurations like `developer_type` in the example below:
<img width="1767" alt="Screen Shot 2022-07-25 at 3 03 58 PM" src="https://user-images.githubusercontent.com/70410625/180844347-fe3095d0-88ce-410f-8c64-3c708205fbd3.png">
To make things more complicated, dashboard filters can come from the filters panel or cross-filters.
I don't know if I covered everything. We may have other sources that impact a chart. Tagging some folks to see if I'm missing anything @rusackas @lauderbaugh @zhaoyongjie @kgabryje @geido
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929930818
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -39,6 +45,8 @@ import ModalTrigger from 'src/components/ModalTrigger';
import Button from 'src/components/Button';
import ViewQueryModal from 'src/explore/components/controls/ViewQueryModal';
import { ResultsPaneOnDashboard } from 'src/explore/components/DataTablesPane';
+import Modal from 'src/components/Modal';
+import DrillDetailPane from 'src/dashboard/components/DrillDetailPane';
Review Comment:
I had it as the default export because it's the only export of the directory as the wiki suggests:
> Default VS named exports: As recommended by [Typescript](https://www.typescriptlang.org/docs/handbook/modules.html), “If a module’s primary purpose is to house one specific export, then you should consider exporting it as a default export. This makes both importing and actually using the import a little easier”. If you’re exporting multiple objects, use named exports instead.
That ok?
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r927767653
##########
superset-frontend/packages/superset-ui-core/src/query/types/Query.ts:
##########
@@ -32,23 +32,30 @@ import { PostProcessingRule } from './PostProcessing';
import { JsonObject } from '../../connection';
import { TimeGranularity } from '../../time-format';
-export type QueryObjectFilterClause = {
+export type BaseQueryObjectFilterClause = {
col: QueryFormColumn;
grain?: TimeGranularity;
isExtra?: boolean;
Review Comment:
With this refactoring I was trying to keep everything the same except allow the specific types of filter clauses (binary, set, unary) to be exported/imported separately, as TypeScript was complaining because `val` was optional on the union `QueryObjectFilterClause` type. If I did it right `grain` and `isExtra` should still be present on all specific filter clause types and the union `QueryObjectFilterClause` type – does that work?
--
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
[GitHub] [superset] codecov[bot] commented on pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1192053635
# [Codecov](https://codecov.io/gh/apache/superset/pull/20728?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20728](https://codecov.io/gh/apache/superset/pull/20728?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3ca3f87) into [master](https://codecov.io/gh/apache/superset/commit/922b4b8d1dd6767d9e675ce95b3ffefe16034a7a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (922b4b8) will **decrease** coverage by `11.67%`.
> The diff coverage is `53.70%`.
> :exclamation: Current head 3ca3f87 differs from pull request most recent head 2e4f5ee. Consider uploading reports for the commit 2e4f5ee to get more accurate results
```diff
@@ Coverage Diff @@
## master #20728 +/- ##
===========================================
- Coverage 66.29% 54.62% -11.68%
===========================================
Files 1758 1755 -3
Lines 66801 66716 -85
Branches 7055 7049 -6
===========================================
- Hits 44288 36441 -7847
- Misses 20713 28479 +7766
+ Partials 1800 1796 -4
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.13% <48.10%> (+0.02%)` | :arrow_up: |
| python | `57.47% <48.10%> (-24.03%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.26% <48.10%> (+0.02%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20728?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/components/Icons/index.tsx](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...end/src/dashboard/components/SliceHeader/index.tsx](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyL2luZGV4LnRzeA==) | `88.00% <ø> (ø)` | |
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `55.04% <ø> (ø)` | |
| [superset/views/datasource/utils.py](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS91dGlscy5weQ==) | `27.90% <27.90%> (ø)` | |
| [superset/views/datasource/views.py](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS92aWV3cy5weQ==) | `51.45% <46.15%> (-45.22%)` | :arrow_down: |
| [...dashboard/components/SliceHeaderControls/index.tsx](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMvaW5kZXgudHN4) | `63.52% <62.50%> (ø)` | |
| [superset/views/datasource/schemas.py](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS9zY2hlbWFzLnB5) | `87.87% <83.33%> (-12.13%)` | :arrow_down: |
| [...erset-frontend/src/components/Chart/chartAction.js](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvY2hhcnRBY3Rpb24uanM=) | `55.92% <100.00%> (ø)` | |
| [superset/datasets/api.py](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvYXBpLnB5) | `50.22% <100.00%> (-38.31%)` | :arrow_down: |
| ... and [330 more](https://codecov.io/gh/apache/superset/pull/20728/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929921612
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
Review Comment:
I've always been in the habit of keeping global-level constants in all caps – is there a reason to not do so?
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r928032017
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
Hmm, good point. I tried implementing a simple LRU with 5 elements but I'm worried that might not be enough to stop huge amounts of data from being cached. Do you think what I have now helps enough or should we get rid of the cache altogether?
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943388262
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -122,7 +122,6 @@ export default function EchartsMixedTimeseries({
const filters: QueryObjectFilterClause[] = [];
filters.push({
col: formData.granularitySqla,
- grain: formData.timeGrainSqla,
Review Comment:
We definitely need the grain because that's how the server-side will interpret the timestamp. So we need to investigate why it's breaking.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943742600
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-MAX_CACHED_PAGES + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [datasourceId, datasourceType, filters, pageIndex, resultsPages]);
+
+ // this is to preserve the order of the columns, even if there are integer values,
+ // while also only grabbing the first column's keys
+ const columns = useTableColumns(
+ resultsPage?.colNames,
+ resultsPage?.colTypes,
+ resultsPage?.data,
+ datasource,
+ );
+
+ const sortDisabledColumns = columns.map(column => ({
+ ...column,
+ disableSortBy: true,
+ }));
+
+ // Update page index on pagination click
+ const onServerPagination = useCallback(({ pageIndex }) => {
+ setPageIndex(pageIndex);
+ }, []);
+
+ // Clear cache on reload button click
+ const handleReload = useCallback(() => {
+ setResultsPages(new Map());
Review Comment:
Ok sounds good, let's work on this in a future PR. My intention with the `new Map()`s was to essentially never mutate the `Map` because it's part of React state, and instead set the state to a new `Map` each time there's a state update.
--
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
[GitHub] [superset] codyml commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1212293001
> Some more comments from manual testing:
>
> * When resizing the modal, the pagination should stick to the bottom. Also, there are 2 vertical scrolls there. I believe we should only have 1 vertical scroll
> * Let's set a minimum size for the modal as the user should never resize it to the point that is unusable
> * When refreshing, should we move the user to page 1? I am thinking of an edge case where we have 2 pages initially, but a bunch of data is deleted or the underlying dataset has changed, now you only have 1 page, but the request will still try to look for page 2 causing a scenario like the one depicted below
> * In the example above you can also see some weird layout for the no results which needs improvement
> * I think we can consider this problem for a next phase, but I believe the user should be able to select exclusively all the content of the table without selecting any other area of the application
@geido Good call on the modal resizing and good catch on the extra scrollbars, will fix! Agreed on the reset to page 1 on refresh, will update. For the selection, maybe we should reintroduce the Copy button if it's clear that it'll only copy the one page? But sure, let's tackle separately.
--
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
[GitHub] [superset] geido commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
geido commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1222223192
/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DRILL_TO_DETAIL=true
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1222569140
@jinghua-qa Ephemeral environment spinning up at http://54.191.88.138: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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feature(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r940355766
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
Review Comment:
Oh great, didn't see that thanks! Will update.
--
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
[GitHub] [superset] zhaoyongjie commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1210333801
/testenv up FEATURE_DRILL_TO_DETAIL=true
--
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
[GitHub] [superset] jinghua-qa commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1222561755
/testenv up
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929927368
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
+ ...[...resultsPages.entries()].slice(-MAX_CACHED_PAGES + 1),
+ [
+ pageIndex,
+ {
+ total: response.total_count,
+ data: response.data,
+ colNames: ensureIsArray(response.colnames),
+ colTypes: ensureIsArray(response.coltypes),
+ },
+ ],
+ ]),
+ );
+ setResponseError('');
+ })
+ .catch(error => {
+ setResponseError(`${error.name}: ${error.message}`);
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [datasourceId, datasourceType, filters, pageIndex, resultsPages]);
+
+ // this is to preserve the order of the columns, even if there are integer values,
+ // while also only grabbing the first column's keys
+ const columns = useTableColumns(
+ resultsPage?.colNames,
+ resultsPage?.colTypes,
+ resultsPage?.data,
+ datasource,
+ );
+
+ const sortDisabledColumns = columns.map(column => ({
+ ...column,
+ disableSortBy: true,
+ }));
+
+ // Update page index on pagination click
+ const onServerPagination = useCallback(({ pageIndex }) => {
+ setPageIndex(pageIndex);
+ }, []);
+
+ // Clear cache on reload button click
+ const handleReload = useCallback(() => {
+ setResultsPages(new Map());
Review Comment:
Is this necessary? The JS GC has been pretty reliable in my experience and this strikes me as an over-optimization, plus we'll need to do a call to `setResultsPages` anyway to trigger a state update.
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r929933132
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
Ok, sounds good!
--
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
[GitHub] [superset] codyml commented on a diff in pull request #20728: feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r925754217
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ QueryObjectFilterClause,
+ styled,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+ margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: QueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const pageResponses = useRef({});
+ const [results, setResults] = useState<{
+ total: number;
+ dataPage: Record<string, any>[];
+ } | null>();
+
+ const [colnames, setColnames] = useState<string[]>([]);
+ const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [responseError, setResponseError] = useState<string>('');
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [page, setPage] = useState(0);
Review Comment:
I think the `Table` expects the page index to be 0-indexed but I'll make the conversion when calling the endpoint.
--
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
[GitHub] [superset] codyml closed pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml closed pull request #20728: [WIP] feature(dashboard): Drill to detail modal
URL: https://github.com/apache/superset/pull/20728
--
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
[GitHub] [superset] zhaoyongjie commented on pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu access
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1208870223
@codyml Thanks for the new feature, I have tested this feature in my local. there are 2 issues.
1. The drill detail modal always sends `force query`. the `force query` means that don't use data cache in the backend, so every force query will send actual SQL to the underlying database, usually, it's an "expensive" operation.
2. The drill detail request is 2 times when I click` drill to detail`.
https://user-images.githubusercontent.com/2016594/183557696-584bea48-e9ce-4f7c-b1aa-79ddeb88713c.mov
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r928470301
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
Let's find a way to dynamically calculate the length of LRU(or just a LIFO and fixed-size cache). I open a [PR](https://github.com/apache/superset/pull/20841/files) for this. The samples_row_limit could be accessed by `common.conf.SAMPLES_ROW_LIMIT` in the Redux store. the length of the cache might be set to `floor(SAMPLES_ROW_LIMIT / PAGE_SIZE)` --- This means that the memory consumption of `DrillDetailPane` and `SamplesPane` is approximately the same.
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r928470301
##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,232 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ Datasource,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: Datasource;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<
Review Comment:
Let's find a way to dynamically calculate the length of LRU(or just a LIFO and fixed-size cache). I open a [PR](https://github.com/apache/superset/pull/20841/files) for this. The samples_row_limit could be accessed by `common.conf.SAMPLES_ROW_LIMIT` in the Redux store. the length of the cache might be set to `ceil(SAMPLES_ROW_LIMIT / PAGE_SIZE)` --- This means that the memory consumption of `DrillDetailPane` and `SamplesPane` is approximately the same.
--
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
[GitHub] [superset] codyml commented on pull request #20728: [WIP] feature(dashboard): Drill to detail modal
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20728:
URL: https://github.com/apache/superset/pull/20728#issuecomment-1199312289
Reopening!
--
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
[GitHub] [superset] geido commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943421030
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
Review Comment:
Agreed
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20728: feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r943464109
##########
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx:
##########
@@ -0,0 +1,239 @@
+/**
+ * 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.
+ */
+
+import React, {
+ useState,
+ useEffect,
+ useMemo,
+ useCallback,
+ useRef,
+} from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ css,
+ ensureIsArray,
+ GenericDataType,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasourceSamples } from 'src/components/Chart/chartAction';
+import TableControls from './TableControls';
+
+type ResultsPage = {
+ total: number;
+ data: Record<string, any>[];
+ colNames: string[];
+ colTypes: GenericDataType[];
+};
+
+const PAGE_SIZE = 50;
+const MAX_CACHED_PAGES = 5;
+
+export default function DrillDetailPane({
+ datasource,
+ initialFilters,
+}: {
+ datasource: string;
+ initialFilters?: BinaryQueryObjectFilterClause[];
+}) {
+ const theme = useTheme();
+ const [pageIndex, setPageIndex] = useState(0);
+ const lastPageIndex = useRef(pageIndex);
+ const [filters, setFilters] = useState(initialFilters || []);
+ const [isLoading, setIsLoading] = useState(false);
+ const [responseError, setResponseError] = useState('');
+ const [resultsPages, setResultsPages] = useState<Map<number, ResultsPage>>(
+ new Map(),
+ );
+
+ // Get string identifier for dataset
+ const [datasourceId, datasourceType] = useMemo(
+ () => datasource.split('__'),
+ [datasource],
+ );
+
+ // Get page of results
+ const resultsPage = useMemo(() => {
+ const nextResultsPage = resultsPages.get(pageIndex);
+ if (nextResultsPage) {
+ lastPageIndex.current = pageIndex;
+ return nextResultsPage;
+ }
+
+ return resultsPages.get(lastPageIndex.current);
+ }, [pageIndex, resultsPages]);
+
+ // Clear cache and reset page index if filters change
+ useEffect(() => {
+ setResultsPages(new Map());
+ setPageIndex(0);
+ }, [filters]);
+
+ // Update cache order if page in cache
+ useEffect(() => {
+ if (
+ resultsPages.has(pageIndex) &&
+ [...resultsPages.keys()].at(-1) !== pageIndex
+ ) {
+ const nextResultsPages = new Map(resultsPages);
+ nextResultsPages.delete(pageIndex);
+ setResultsPages(
+ nextResultsPages.set(
+ pageIndex,
+ resultsPages.get(pageIndex) as ResultsPage,
+ ),
+ );
+ }
+ }, [pageIndex, resultsPages]);
+
+ // Download page of results & trim cache if page not in cache
+ useEffect(() => {
+ if (!resultsPages.has(pageIndex)) {
+ setIsLoading(true);
+ getDatasourceSamples(
+ datasourceType,
+ datasourceId,
+ true,
+ filters.length ? { filters } : null,
+ { page: pageIndex + 1, perPage: PAGE_SIZE },
+ )
+ .then(response => {
+ setResultsPages(
+ new Map([
Review Comment:
This component has too many `new Map()`, If performance is not considered, at least when I read the code, I don't know exactly which **cache object** is being manipulated. ---- we can be optimized later after the PR merged.
--
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