Skip to content

bar chart support #2005

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.

Sign up for GitHub

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

Open
gingerwizard wants to merge 1 commit into
base: main
Choose a base branch
from
Open

bar chart support #2005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions packages/app/src/ChartUtils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,6 +1080,19 @@ export function convertToPieChartConfig(
return omit(config, ['granularity']);
}

export function convertToBarChartConfig(
config: BuilderChartConfigWithOptTimestamp,
): BuilderChartConfigWithOptTimestamp {
const convertedConfig = structuredClone(omit(config, ['granularity']));

// Apply a default limit if not already configured
if (!convertedConfig.limit) {
convertedConfig.limit = { limit: 10 };
}

return convertedConfig;
}

export function convertToTableChartConfig(
config: BuilderChartConfigWithOptTimestamp,
): BuilderChartConfigWithOptTimestamp {
Expand Down
9 changes: 9 additions & 0 deletions packages/app/src/DBDashboardPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ import {
} from '@/dashboard';

import ChartContainer from './components/charts/ChartContainer';
import { DBBarChart } from './components/DBBarChart';
import { DBPieChart } from './components/DBPieChart';
import DBSqlRowTableWithSideBar from './components/DBSqlRowTableWithSidebar';
import OnboardingModal from './components/OnboardingModal';
Expand Down Expand Up @@ -546,6 +547,14 @@ const Tile = forwardRef(
config={queriedConfig}
/>
)}
{queriedConfig?.displayType === DisplayType.Bar && (
<DBBarChart
key={`${keyPrefix}-${chart.id}`}
title={title}
toolbarPrefix={toolbar}
config={queriedConfig}
/>
)}
{effectiveMarkdownConfig?.displayType === DisplayType.Markdown &&
'markdown' in effectiveMarkdownConfig && (
<HDXMarkdownChart
Expand Down
33 changes: 31 additions & 2 deletions packages/app/src/components/ChartDisplaySettingsDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useCallback } from 'react';
import { useForm, useWatch } from 'react-hook-form';
import { Controller, useForm, useWatch } from 'react-hook-form';
import {
ChartConfigWithDateRange,
DisplayType,
Expand All @@ -11,6 +11,7 @@ import {
Divider,
Drawer,
Group,
NumberInput,
Stack,
} from '@mantine/core';

Expand All @@ -25,7 +26,9 @@ export type ChartConfigDisplaySettings = Pick<
| 'alignDateRangeToGranularity'
| 'fillNulls'
| 'compareToPreviousPeriod'
>;
> & {
maxNumberOfGroups?: number;
};
Comment on lines +29 to +31
Copy link
Copy Markdown
Contributor

@pulpdrew pulpdrew Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this display setting intentionally not persisted? All other display settings are persisted as part of the config in the DB / URL Params and available via the API - it is odd if this one is not.

Perhaps instead of adding this new property, we can just use the existing limit property. That way it is persisted, and you don't have to copy back and forth between limit and maxNumberOfGroups in the places where that is happening right now.

normalizeChartConfig could also then be updated to strip limit from non-Bar type chart configs before saving, since the customizable limit is only available for bar charts (though it would be good to add to pie as well)


interface ChartDisplaySettingsDrawerProps {
opened: boolean;
Expand All @@ -41,13 +44,15 @@ function applyDefaultSettings({
alignDateRangeToGranularity,
compareToPreviousPeriod,
fillNulls,
maxNumberOfGroups,
}: ChartConfigDisplaySettings): ChartConfigDisplaySettings {
return {
numberFormat: numberFormat ?? DEFAULT_NUMBER_FORMAT,
alignDateRangeToGranularity:
alignDateRangeToGranularity == null ? true : alignDateRangeToGranularity,
fillNulls: fillNulls ?? 0,
compareToPreviousPeriod: compareToPreviousPeriod ?? false,
maxNumberOfGroups: maxNumberOfGroups ?? 10,
};
}

Expand Down Expand Up @@ -83,6 +88,7 @@ export default function ChartDisplaySettingsDrawer({

const isTimeChart =
displayType === DisplayType.Line || displayType === DisplayType.StackedBar;
const isBarChart = displayType === DisplayType.Bar;

return (
<Drawer
Expand Down Expand Up @@ -128,6 +134,29 @@ export default function ChartDisplaySettingsDrawer({
</>
)}

{isBarChart && (
Copy link
Copy Markdown
Contributor

@pulpdrew pulpdrew Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is neat, and would be nice to add to Pie charts too.

However, we'd ideally give the user an Order By input as well, so that they can specify which 10 groups to show. There is an order by on the table chart type that might be re-usable

Copy link
Copy Markdown
Contributor

@pulpdrew pulpdrew Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also should not show this for raw sql bar charts, since it's ignored.

<>
<Box>
<Controller
control={control}
name="maxNumberOfGroups"
render={({ field }) => (
<NumberInput
size="xs"
label="Max Number of Groups"
min={1}
max={1000}
value={field.value ?? 10}
onChange={v =>
field.onChange(typeof v === 'number' ? v : 10)
}
/>
)}
/>
</Box>
<Divider />
</>
)}
<NumberFormatForm control={control} />
<Divider />
<Group gap="xs" mt="xs" justify="space-between">
Expand Down
38 changes: 38 additions & 0 deletions packages/app/src/components/ChartEditor/constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ ORDER BY ts ASC;`;
export const SQL_PLACEHOLDERS: Record<DisplayType, string> = {
[DisplayType.Line]: TIMESERIES_PLACEHOLDER_SQL,
[DisplayType.StackedBar]: TIMESERIES_PLACEHOLDER_SQL,
[DisplayType.Bar]: `SELECT
SeverityText,
count()
FROM
default.otel_logs
WHERE TimestampTime >= fromUnixTimestamp64Milli({startDateMilliseconds:Int64})
AND TimestampTime < fromUnixTimestamp64Milli({endDateMilliseconds:Int64})
GROUP BY SeverityText
ORDER BY count() DESC
LIMIT 10;`,
[DisplayType.Table]: `SELECT
count()
FROM
Expand Down Expand Up @@ -90,6 +100,34 @@ export const DISPLAY_TYPE_INSTRUCTIONS: Partial<
> = {
[DisplayType.Line]: TIMESERIES_INSTRUCTIONS,
[DisplayType.StackedBar]: TIMESERIES_INSTRUCTIONS,
[DisplayType.Bar]: (
<>
<Text size="xs" fw="bold">
Result columns are plotted as follows:
</Text>
<List size="xs" withPadding spacing={3} mb="xs">
<List.Item>
<Text span size="xs" fw={600}>
Bar Value
</Text>
<Text span size="xs">
{' '}
— The first numeric column determines each bar&apos;s height.
</Text>
</List.Item>
<List.Item>
<Text span size="xs" fw={600}>
X Axis Label
</Text>
<Text span size="xs">
{' '}
— Each unique value of each string, map, and array type column will
be used as an X axis label. Group By is required.
</Text>
</List.Item>
</List>
</>
),
[DisplayType.Pie]: (
<>
<Text size="xs" fw="bold">
Expand Down
19 changes: 17 additions & 2 deletions packages/app/src/components/ChartEditor/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,13 @@ export const isRawSqlDisplayType = (
| DisplayType.Table
| DisplayType.Line
| DisplayType.StackedBar
| DisplayType.Bar
| DisplayType.Pie
| DisplayType.Number =>
displayType === DisplayType.Table ||
displayType === DisplayType.Line ||
displayType === DisplayType.StackedBar ||
displayType === DisplayType.Bar ||
displayType === DisplayType.Pie ||
displayType === DisplayType.Number;

Expand Down Expand Up @@ -236,12 +238,13 @@ export const validateChartForm = (
});
}

// Validate number and pie charts only have one series
// Validate number, pie, and bar charts only have one series
if (
!isRawSqlChart &&
Array.isArray(form.series) &&
(form.displayType === DisplayType.Number ||
form.displayType === DisplayType.Pie) &&
form.displayType === DisplayType.Pie ||
form.displayType === DisplayType.Bar) &&
form.series.length > 1
) {
errors.push({
Expand All @@ -250,6 +253,18 @@ export const validateChartForm = (
});
}

// Validate bar charts require a group by
if (
!isRawSqlChart &&
form.displayType === DisplayType.Bar &&
!form.groupBy
) {
errors.push({
path: `groupBy`,
message: 'Group By is required for bar charts',
});
}

Comment on lines +256 to +267
Copy link
Copy Markdown
Contributor

@pulpdrew pulpdrew Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a unit test covering this check? There are existing unit tests covering this function which can be extended.

for (const error of errors) {
console.warn(`Validation error in field ${error.path}: ${error.message}`);
setError(error.path, {
Expand Down
Loading
Loading