Skip to content

Conversation

@Br0wnHammer
Copy link
Contributor

This PR introduces the nullable checkbox for clickhouse destinations.

Fixes #3486

@Br0wnHammer Br0wnHammer temporarily deployed to external-contributor November 22, 2025 07:27 — with GitHub Actions Inactive
@Br0wnHammer Br0wnHammer temporarily deployed to external-contributor November 22, 2025 07:27 — with GitHub Actions Inactive
@Br0wnHammer Br0wnHammer temporarily deployed to external-contributor November 22, 2025 07:27 — with GitHub Actions Inactive
@Br0wnHammer Br0wnHammer temporarily deployed to external-contributor November 22, 2025 07:27 — with GitHub Actions Inactive
@Br0wnHammer
Copy link
Contributor Author

UI

@ilidemi ilidemi requested review from Copilot and ilidemi November 24, 2025 21:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a nullable checkbox feature for ClickHouse destinations, allowing users to configure whether columns can contain null values. The changes introduce UI components and state management logic to support nullable column configuration.

Key Changes:

  • Added nullable checkbox UI control in the column configuration interface
  • Implemented state preservation logic for nullable settings across column updates
  • Refactored column display layout to support the new nullable control

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ui/app/mirrors/create/cdc/columnbox.tsx Major refactor to add nullable checkbox column with grid layout and new state management handlers
ui/app/mirrors/create/cdc/sortingkey.tsx Preserves nullable settings when creating new column configurations
ui/app/mirrors/create/cdc/schemabox.tsx Ensures existing column settings are preserved during row updates
ui/app/mirrors/create/cdc/customColumnType.tsx Minor refactor changing some() to find() for consistency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 53 to 54
if (!columns.find((setting) => setting.sourceName === colName)) {
// Check if there's an existing ColumnSetting to preserve nullableEnabled
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The existingSetting will always be undefined because line 53 already verifies that no column with sourceName === colName exists. This logic attempts to find a setting that was just confirmed to not exist, making existingSetting always undefined and the preservation attempt ineffective.

Suggested change
if (!columns.find((setting) => setting.sourceName === colName)) {
// Check if there's an existing ColumnSetting to preserve nullableEnabled
// If the column with colName does not exist after update, add it and preserve nullableEnabled if possible
if (!columns.some((setting) => setting.sourceName === colName)) {

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

@Br0wnHammer the existingSetting code can be removed, the result of it is only used when the setting doesn't exist :)

if (row.source !== tableRow.source) return row;

const columnExistsInRow = row.columns.some(
const existingColumn = row.columns.find(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

@Br0wnHammer Br0wnHammer Nov 27, 2025

Choose a reason for hiding this comment

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

Find helps in getting the column object. Instead of a boolean. Which we can use for future proofing. If needed I can revert to as it was before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now it is just putting more work on the runtime while we only need a boolean. Let's leave this change for when the future actually comes.

);

const isIncluded = !tableRow.exclude.has(column.name);
const nullableEnabled = getNullableEnabled(column.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is O(N^2) in the number of columns, could that be improved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can use a map for O(1) lookups.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still O(N^2) :)

<div style={{ fontSize: 12, fontWeight: 500 }}>Name</div>
<div style={{ fontSize: 12, fontWeight: 500 }}>Type</div>
{showOrdering &&
<div style={{ fontSize: 12, fontWeight: 500 }}>Nullable</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div style={{ fontSize: 12, fontWeight: 500 }}>Nullable</div>
<div style={{ fontSize: 12, fontWeight: 500, display: 'flex', alignItems: 'center', gap: '0.25rem' }}>
Nullable
<InfoPopover tips="Enabling nullable columns wraps types in Nullable() which may impact query performance and storage in ClickHouse." />
</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, didn't realize the popover would land right on top of checkboxes and would make things look off. Not seeing a way to make it all work together, let's just remove it.

@Br0wnHammer Br0wnHammer deployed to external-contributor November 27, 2025 18:02 — with GitHub Actions Active
@Br0wnHammer Br0wnHammer temporarily deployed to external-contributor November 27, 2025 18:02 — with GitHub Actions Inactive
@Br0wnHammer Br0wnHammer temporarily deployed to external-contributor November 27, 2025 18:02 — with GitHub Actions Inactive
@Br0wnHammer Br0wnHammer temporarily deployed to external-contributor November 27, 2025 18:02 — with GitHub Actions Inactive
@Br0wnHammer
Copy link
Contributor Author

Screenshot 2025-11-27 at 23 18 12

@ilidemi

@Br0wnHammer Br0wnHammer requested a review from ilidemi November 27, 2025 18:02
Copy link
Contributor

@ilidemi ilidemi left a comment

Choose a reason for hiding this comment

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

Figured out an ok design (not my specialty 😅)

Comment on lines 5 to +9
import { Checkbox } from '@/lib/Checkbox';
import { Label } from '@/lib/Label';
import { RowWithCheckbox } from '@/lib/Layout';
import { Dispatch, SetStateAction } from 'react';
import { Dispatch, Fragment, SetStateAction, useMemo } from 'react';
import InfoPopover from '@/components/InfoPopover';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { Checkbox } from '@/lib/Checkbox';
import { Label } from '@/lib/Label';
import { RowWithCheckbox } from '@/lib/Layout';
import { Dispatch, SetStateAction } from 'react';
import { Dispatch, Fragment, SetStateAction, useMemo } from 'react';
import InfoPopover from '@/components/InfoPopover';
import { appTheme } from '@/lib/AppTheme/appTheme';
import { Checkbox } from '@/lib/Checkbox';
import { Label } from '@/lib/Label';
import { RowWithCheckbox } from '@/lib/Layout';
import { Dispatch, Fragment, SetStateAction, useMemo } from 'react';

Comment on lines +92 to +112
<div
style={{
display: 'grid',
gridTemplateColumns: showNullable
? 'minmax(0, 2fr) minmax(0, 1fr) auto'
: 'minmax(0, 2fr) minmax(0, 1fr)',
alignItems: 'center',
columnGap: '2.5rem',
width: '80%',
}}
>
<div style={{ fontSize: 12, fontWeight: 500, textAlign: 'left' }}>Name</div>
<div style={{ fontSize: 12, fontWeight: 500 }}>Type</div>
{showNullable && (
<div style={{ fontSize: 12, fontWeight: 500, display: 'flex', alignItems: 'center', gap: '0.25rem', justifyContent: 'flex-end' }}>
Nullable
<InfoPopover
tips="Enabling nullable columns wraps types in Nullable() which may impact query performance and storage in Clickhouse."
/>
</div>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div
style={{
display: 'grid',
gridTemplateColumns: showNullable
? 'minmax(0, 2fr) minmax(0, 1fr) auto'
: 'minmax(0, 2fr) minmax(0, 1fr)',
alignItems: 'center',
columnGap: '2.5rem',
width: '80%',
}}
>
<div style={{ fontSize: 12, fontWeight: 500, textAlign: 'left' }}>Name</div>
<div style={{ fontSize: 12, fontWeight: 500 }}>Type</div>
{showNullable && (
<div style={{ fontSize: 12, fontWeight: 500, display: 'flex', alignItems: 'center', gap: '0.25rem', justifyContent: 'flex-end' }}>
Nullable
<InfoPopover
tips="Enabling nullable columns wraps types in Nullable() which may impact query performance and storage in Clickhouse."
/>
</div>
)}
<div
style={{
display: 'grid',
gridTemplateColumns: showNullable
? 'auto auto auto'
: 'auto auto',
alignItems: 'center',
columnGap: '3rem',
width: 'fit-content',
}}
>
<div
style={{
fontSize: 12,
fontWeight: 500,
textAlign: 'left',
paddingLeft: `calc(${appTheme.spacing.xxSmall} + ${appTheme.spacing.medium})`,
}}
>
Name
</div>
<div style={{ fontSize: 12, fontWeight: 500 }}>Type</div>
{showNullable && (
<div style={{ fontSize: 12, fontWeight: 500 }}>Nullable</div>
)}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support allowNullable on a column level

2 participants