[#12337] feat(web): Support ClickHouse data-skipping indexes and engine-aware sort orders validation - #12339
[#12337] feat(web): Support ClickHouse data-skipping indexes and engine-aware sort orders validation#12339LauraXia123 wants to merge 7 commits into
Conversation
…rt orders validation in CreateTableDialog
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Enhances the ClickHouse table creation/editing UX by adding support for data-skipping index properties and making Sort Orders validation depend on the selected ClickHouse engine.
Changes:
- Add ClickHouse data-skipping index types and surface
granularity/set_max_valuesin UI and submit payloads. - Make Sort Orders required only for MergeTree-family engines, with dynamic required indicator and submit-time validation.
- Extend table details views/store mapping to include index
properties.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web-v2/web/src/lib/store/metalakes/index.js | Includes index properties in table details mapping for edit/view. |
| web-v2/web/src/config/index.js | Adds ClickHouse defaults/immutables and defines MergeTree engine list + index types. |
| web-v2/web/src/config/catalog.js | Adds ClickHouse engine default property options for table creation. |
| web-v2/web/src/app/catalogs/rightContent/entitiesContent/TableDetailsPage.js | Renders index granularity and set_max_values columns in table details. |
| web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js | Adds index-property fields, payload building, and engine-aware Sort Orders validation/UX. |
Suppressed comments (1)
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:19
- Index type comparisons and payload building rely on
startsWith('data_skipping_'), but when editing an existing table you setindexTypeusingcapitalizeFirstLetter(item.indexType). If that transformsdata_skipping_*into a different casing, the UI will fail to show Granularity/Set Max Values and the submit payload will omitproperties(and may send an unexpectedindexTypevalue). Store/compareindexTypein a canonical format (e.g., keep the backend value as-is / lowercase) and only transform for display/labels if needed; alternatively, normalize comparisons with.toLowerCase().
…with component-level source of truth
Use a unified getClickHouseEngine() function that reads from both
form.getFieldValue('engine') and properties array, matching the same
fallback logic used by clickHouseEngine at component level. Also fix
isClickHouseDistributedEngine to use the same dual-source pattern.
🤖 Generated with CodeArts Agent
…ckHouse tables
Remove capitalizeFirstLetter() on indexType when populating the edit
form. The Select component already handles display transformation
via split/join, and the form value must stay in the backend's
lowercase format (e.g. data_skipping_minmax) so that:
- isDataSkippingIndex() startsWith('data_skipping_') matches correctly
- 'data_skipping_set' equality check works
- submit payload sends the correct indexType value to the backend
🤖 Generated with CodeArts Agent
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
web-v2/web/src/config/catalog.js:231
- The ClickHouse engine list is duplicated here and in
web-v2/web/src/config/index.js(clickHouseMergeTreeEngines). Keeping two separate lists in sync is error-prone and can cause UI/validation drift (e.g., an engine selectable here but not recognized as MergeTree elsewhere). Consider reusing the shared constant (or moving the list to a single module) so both the Select options and validation logic stay aligned.
}
],
'jdbc-clickhouse': [
{
key: 'engine',
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:134
isClickHouseMergeTreeEngineuses a case-sensitiveincludescheck, but the engine value is read from form values/properties and may not have consistent casing (e.g., existing tables or user-provided properties). This can cause the required indicator and engine-aware validation to be skipped for MergeTree engines. Consider normalizing the engine string (e.g., lowercase) before comparison (and apply the same normalization inhandleSubmit'sisCurrentMergeTreecheck).
provider === 'jdbc-clickhouse'
? (values?.engine || values?.properties?.find(item => item?.key === 'engine')?.value)?.toLowerCase()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:1
- For ClickHouse,
indexTypeis being uppercased at submit time. The UI options (indexesInfoMap) and edit path use lowercase values (e.g.,data_skipping_set), so uppercasing risks sending an unexpected value to the API and/or breaking round-tripping when editing. Unless the backend explicitly requires uppercase enum values, keep the same casing as the selected value (or normalize consistently both on load and submit).
/*
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:1652
- The PR description says sort orders validation was moved out of
Form.Item rulesandvalidateTrigger='onSubmit'was removed to avoid Ant Design caching stale closures. However, this diff still addsvalidateTrigger='onSubmit'and an inline validator that closes overisSortOrdersRequired/isMergeTreeEngine. To align with the described fix (and prevent stale engine-dependent validation), move this engine-aware validation into submit-time logic (or a validator that reads current values inside the function) and removevalidateTrigger/ruleshere.
<Form.Item
className={tabKey !== 'sortOrders' ? 'hidden' : ''}
label=''
name='sortOrders'
validateTrigger='onSubmit'
rules={[
{
validator: (_, val) => {
if (isSortOrdersRequired) {
if (Array.isArray(val) && val.length > 0) {
if (isMergeTreeEngine) {
const columns = form.getFieldValue('columns') || []
const nullableSortFields = val
?.filter(s => {
const col = columns.find(c => c?.name === s?.fieldName)
return col && !col.required
})
?.map(s => s?.fieldName)
if (nullableSortFields?.length > 0) {
return Promise.reject(
new Error(
`Nullable columns cannot be used in ORDER BY for MergeTree engines: ${nullableSortFields.join(', ')}`
)
)
}
}
return Promise.resolve()
}
return Promise.reject(new Error('Sort orders are required for MergeTree family engines'))
}
return Promise.resolve()
}
}
]}
>
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:195
- This
hidehandling returnsfalse(hides the prop) when theparentFieldvalue is unset. That makes properties disappear by default wheneverhideis configured but the parent value isn't present yet, which is usually the opposite of what's desired (typically: show until the parent matches a hidden value). Consider returningtruewhenparentValueis missing, and only hiding whenparentValueis present and matches a hidden option.
// If the prop has a "hide" list, hide it when the parentField value matches
if (prop.hide?.length) {
const parentValue = values?.[prop.parentField]?.toLowerCase()
if (!parentValue) {
return false
}
return !prop.hide.map(item => item.toLowerCase()).includes(parentValue)
}
What changes were proposed in this pull request?
This PR enhances the
CreateTableDialogfor ClickHouse table creation with two features:1. Support ClickHouse data-skipping index types with properties
data_skipping_minmax,data_skipping_bloom_filter,data_skipping_set), hidden (-) forprimary_keydata_skipping_setindex typegranularityandset_max_valueswhen editing an existing tablepropertiesobject in the submit payload for data-skipping indexes2. Engine-aware sort orders validation
clickHouseEngineandisClickHouseMergeTreeEnginecomputed variables to detect the current engine type in real-time*) on Sort Orders tab only for MergeTree family enginesForm.Item rules(which can cache stale closure values) tohandleSubmitwhereform.getFieldValue('engine')reads the current engine valueuseEffectonisClickHouseMergeTreeEngineto:validateTrigger='onSubmit'andrulesfrom sortOrdersForm.Itemto avoid Ant Design Form caching stale validator closuresWhy are the changes needed?
ClickHouse data-skipping indexes require
granularityandset_max_valuesproperties to be configured. Without these fields in the UI, users cannot properly create data-skipping indexes through the Gravitino web interface.Sort orders are only required for MergeTree family engines. For other ClickHouse engines (Distributed, TinyLog, Log, StripeLog), sort orders are optional. The previous implementation always validated sort orders as required regardless of engine type, causing form submission to fail for non-MergeTree engines when sort orders were empty.
Fix: #12337
Does this PR introduce any user-facing change?
Yes:
-for non-data-skipping index types.*) now dynamically appears/disappears based on the selected ClickHouse engine type. Sort orders are only required for MergeTree family engines.How was this patch tested?
*and validation requires at least one sort order*disappears, sort orders data is cleared, and form can be submitted without sort orders*reappears and validation error shows if sort orders are emptydata_skipping_minmaxindex - verify Granularity field is shown, Set Max Values shows-data_skipping_setindex - verify both Granularity and Set Max Values fields are shownprimary_keyindex - verify both Granularity and Set Max Values show-