Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: row limits & row count labels are confusing #27700

Merged
merged 7 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/no-op.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ jobs:
python-lint:
strategy:
matrix:
python-version: ["3.9"]
python-version: ["3.9", "3.10"]
runs-on: ubuntu-latest
steps:
- name: No-op for python-lint
Expand All @@ -55,7 +55,7 @@ jobs:
test-postgres-hive:
strategy:
matrix:
python-version: ["3.9"]
python-version: ["3.9", "3.10"]
runs-on: ubuntu-latest
steps:
- name: No-op for frontend-build
Expand All @@ -65,7 +65,7 @@ jobs:
test-postgres-presto:
strategy:
matrix:
python-version: ["3.9"]
python-version: ["3.9", "3.10"]
runs-on: ubuntu-latest
steps:
- name: No-op for frontend-build
Expand All @@ -75,7 +75,7 @@ jobs:
unit-tests:
strategy:
matrix:
python-version: ["3.9"]
python-version: ["3.9", "3.10"]
runs-on: ubuntu-latest
steps:
- name: No-op for frontend-build
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export interface ChartDataResponseResult {
is_cached: boolean;
query: string;
rowcount: number;
sql_rowcount: number;
stacktrace: string | null;
status:
| 'stopped'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ const basicQueryResult: ChartDataResponseResult = {
is_cached: false,
query: 'SELECT ...',
rowcount: 100,
sql_rowcount: 100,
stacktrace: null,
status: 'success',
from_dttm: null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const useResultsTableView = (
<SingleQueryResultPane
colnames={chartDataResult[0].colnames}
coltypes={chartDataResult[0].coltypes}
rowcount={chartDataResult[0].sql_rowcount}
data={chartDataResult[0].data}
dataSize={DATA_SIZE}
datasourceId={datasourceId}
Expand All @@ -61,6 +62,7 @@ export const useResultsTableView = (
colnames={res.colnames}
coltypes={res.coltypes}
data={res.data}
rowcount={res.sql_rowcount}
dataSize={DATA_SIZE}
datasourceId={datasourceId}
isVisible
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/components/ChartPills.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export const ChartPills = forwardRef(
>
{!isLoading && firstQueryResponse && (
<RowCountLabel
rowcount={Number(firstQueryResponse.rowcount) || 0}
rowcount={Number(firstQueryResponse.sql_rowcount) || 0}
limit={Number(rowLimit) || 0}
/>
)}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ import Button from 'src/components/Button';
import Popover from 'src/components/Popover';
import { prepareCopyToClipboardTabularData } from 'src/utils/common';
import CopyToClipboard from 'src/components/CopyToClipboard';
import RowCountLabel from 'src/explore/components/RowCountLabel';
import { getTimeColumns, setTimeColumns } from './utils';

export const CellNull = styled('span')`
Expand Down Expand Up @@ -118,14 +117,6 @@ export const FilterInput = ({
);
};

export const RowCount = ({
data,
loading,
}: {
data?: Record<string, any>[];
loading: boolean;
}) => <RowCountLabel rowcount={data?.length ?? 0} loading={loading} />;

enum FormatPickerValue {
Formatted = 'formatted',
Original = 'original',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ import { css, GenericDataType, styled } from '@superset-ui/core';
import {
CopyToClipboardButton,
FilterInput,
RowCount,
} from 'src/explore/components/DataTableControl';
import { applyFormattingToTabularData } from 'src/utils/common';
import { getTimeColumns } from 'src/explore/components/DataTableControl/utils';
import RowCountLabel from 'src/explore/components/RowCountLabel';
import { TableControlsProps } from '../types';

export const TableControlsWrapper = styled.div`
Expand All @@ -47,6 +47,7 @@ export const TableControls = ({
onInputChange,
columnNames,
columnTypes,
rowcount,
isLoading,
}: TableControlsProps) => {
const originalTimeColumns = getTimeColumns(datasourceId);
Expand Down Expand Up @@ -74,7 +75,7 @@ export const TableControls = ({
align-items: center;
`}
>
<RowCount data={data} loading={isLoading} />
<RowCountLabel rowcount={rowcount} loading={isLoading} />
<CopyToClipboardButton data={formattedData} columns={columnNames} />
</div>
</TableControlsWrapper>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const SamplesPane = ({
const [colnames, setColnames] = useState<string[]>([]);
const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
const [isLoading, setIsLoading] = useState<boolean>(false);
const [rowcount, setRowCount] = useState<number>(0);
const [responseError, setResponseError] = useState<string>('');
const datasourceId = useMemo(
() => `${datasource.id}__${datasource.type}`,
Expand All @@ -66,6 +67,7 @@ export const SamplesPane = ({
setData(ensureIsArray(response.data));
setColnames(ensureIsArray(response.colnames));
setColtypes(ensureIsArray(response.coltypes));
setRowCount(response.rowcount);
setResponseError('');
cache.add(datasource);
if (queryForce && actions) {
Expand Down Expand Up @@ -108,6 +110,7 @@ export const SamplesPane = ({
data={filteredData}
columnNames={colnames}
columnTypes={coltypes}
rowcount={rowcount}
datasourceId={datasourceId}
onInputChange={input => setFilterText(input)}
isLoading={isLoading}
Expand All @@ -128,6 +131,7 @@ export const SamplesPane = ({
data={filteredData}
columnNames={colnames}
columnTypes={coltypes}
rowcount={rowcount}
datasourceId={datasourceId}
onInputChange={input => setFilterText(input)}
isLoading={isLoading}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export const SingleQueryResultPane = ({
data,
colnames,
coltypes,
rowcount,
datasourceId,
dataSize = 50,
isVisible,
Expand All @@ -55,6 +56,7 @@ export const SingleQueryResultPane = ({
data={filteredData}
columnNames={colnames}
columnTypes={coltypes}
rowcount={rowcount}
datasourceId={datasourceId}
onInputChange={input => setFilterText(input)}
isLoading={false}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export const useResultsPane = ({
data={[]}
columnNames={[]}
columnTypes={[]}
rowcount={0}
datasourceId={queryFormData.datasource}
onInputChange={() => {}}
isLoading={false}
Expand All @@ -135,14 +136,14 @@ export const useResultsPane = ({
<EmptyStateMedium image="document.svg" title={title} />,
);
}

return resultResp
.slice(0, queryCount)
.map((result, idx) => (
<SingleQueryResultPane
data={result.data}
colnames={result.colnames}
coltypes={result.coltypes}
rowcount={result.rowcount}
dataSize={dataSize}
datasourceId={queryFormData.datasource}
key={idx}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ describe('DataTablesPane', () => {
data: [{ __timestamp: 1230768000000, genre: 'Action' }],
colnames: ['__timestamp', 'genre'],
coltypes: [2, 1],
rowcount: 1,
sql_rowcount: 1,
},
],
},
Expand Down Expand Up @@ -125,6 +127,8 @@ describe('DataTablesPane', () => {
],
colnames: ['__timestamp', 'genre'],
coltypes: [2, 1],
rowcount: 2,
sql_rowcount: 2,
},
],
},
Expand All @@ -135,6 +139,7 @@ describe('DataTablesPane', () => {
});
userEvent.click(screen.getByText('Results'));
expect(await screen.findByText('2 rows')).toBeVisible();

expect(screen.getByText('Action')).toBeVisible();
expect(screen.getByText('Horror')).toBeVisible();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ describe('ResultsPaneOnDashboard', () => {
],
colnames: ['__timestamp', 'genre'],
coltypes: [2, 1],
rowcount: 2,
sql_rowcount: 2,
},
],
},
Expand Down Expand Up @@ -78,6 +80,8 @@ describe('ResultsPaneOnDashboard', () => {
data: [{ genre: 'Action' }, { genre: 'Horror' }],
colnames: ['genre'],
coltypes: [1],
rowcount: 2,
sql_rowcount: 2,
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ describe('SamplesPane', () => {
],
colnames: ['__timestamp', 'genre'],
coltypes: [2, 1],
rowcount: 2,
sql_rowcount: 2,
},
},
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,13 @@ export interface TableControlsProps {
columnNames: string[];
columnTypes: GenericDataType[];
isLoading: boolean;
rowcount: number;
}

export interface QueryResultInterface {
colnames: string[];
coltypes: GenericDataType[];
rowcount: number;
data: Record<string, any>[][];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ test('RowCountLabel renders limit with danger and tooltip', async () => {
expect(screen.getByText(expectedText)).toBeInTheDocument();
userEvent.hover(screen.getByText(expectedText));
const tooltip = await screen.findByRole('tooltip');
expect(tooltip).toHaveTextContent('Limit reached');
expect(tooltip).toHaveTextContent('The row limit');
expect(tooltip).toHaveStyle('background: rgba(0, 0, 0, 0.902);');
});

Expand Down
10 changes: 7 additions & 3 deletions superset-frontend/src/explore/components/RowCountLabel/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,13 @@ type RowCountLabelProps = {
loading?: boolean;
};

const limitReachedMsg = t(
'The row limit set for the chart was reached. The chart may show partial data.',
);

export default function RowCountLabel(props: RowCountLabelProps) {
const { rowcount = 0, limit, loading } = props;
const limitReached = rowcount === limit;
const { rowcount = 0, limit = null, loading } = props;
const limitReached = limit && rowcount >= limit;
const type =
limitReached || (rowcount === 0 && !loading) ? 'danger' : 'default';
const formattedRowCount = getNumberFormatter()(rowcount);
Expand All @@ -46,7 +50,7 @@ export default function RowCountLabel(props: RowCountLabelProps) {
</Label>
);
return limitReached ? (
<Tooltip id="tt-rowcount-tooltip" title={<span>{t('Limit reached')}</span>}>
<Tooltip id="tt-rowcount-tooltip" title={<span>{limitReachedMsg}</span>}>
{label}
</Tooltip>
) : (
Expand Down
2 changes: 2 additions & 0 deletions superset/common/query_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ def _get_full(
"data": payload.get("data"),
"colnames": payload.get("colnames"),
"coltypes": payload.get("coltypes"),
"rowcount": payload.get("rowcount"),
"sql_rowcount": payload.get("sql_rowcount"),
}
return payload

Expand Down
1 change: 1 addition & 0 deletions superset/common/query_context_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ def get_df_payload(
"status": cache.status,
"stacktrace": cache.stacktrace,
"rowcount": len(cache.df.index),
"sql_rowcount": cache.sql_rowcount,
"from_dttm": query_obj.from_dttm,
"to_dttm": query_obj.to_dttm,
"label_map": label_map,
Expand Down
5 changes: 5 additions & 0 deletions superset/common/utils/query_cache_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def __init__(
is_cached: bool | None = None,
cache_dttm: str | None = None,
cache_value: dict[str, Any] | None = None,
sql_rowcount: int | None = None,
) -> None:
self.df = df
self.query = query
Expand All @@ -79,6 +80,7 @@ def __init__(
self.is_cached = is_cached
self.cache_dttm = cache_dttm
self.cache_value = cache_value
self.sql_rowcount = sql_rowcount

# pylint: disable=too-many-arguments
def set_query_result(
Expand All @@ -102,6 +104,7 @@ def set_query_result(
self.rejected_filter_columns = query_result.rejected_filter_columns
self.error_message = query_result.error_message
self.df = query_result.df
self.sql_rowcount = query_result.sql_rowcount
self.annotation_data = {} if annotation_data is None else annotation_data

if self.status != QueryStatus.FAILED:
Expand All @@ -117,6 +120,7 @@ def set_query_result(
"applied_filter_columns": self.applied_filter_columns,
"rejected_filter_columns": self.rejected_filter_columns,
"annotation_data": self.annotation_data,
"sql_rowcount": self.sql_rowcount,
}
if self.is_loaded and key and self.status != QueryStatus.FAILED:
self.set(
Expand Down Expand Up @@ -167,6 +171,7 @@ def get(
query_cache.status = QueryStatus.SUCCESS
query_cache.is_loaded = True
query_cache.is_cached = cache_value is not None
query_cache.sql_rowcount = cache_value.get("sql_rowcount", None)
query_cache.cache_dttm = (
cache_value["dttm"] if cache_value is not None else None
)
Expand Down
Loading
Loading