Copilot commented on code in PR #12339:
URL: https://github.com/apache/gravitino/pull/12339#discussion_r3703062336
##########
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:
##########
@@ -594,9 +654,54 @@ export default function CreateTableDialog({ ...props }) {
const handleSubmit = e => {
e.preventDefault()
+
+ const currentEngine = form.getFieldValue('engine')
+ const isCurrentMergeTree = provider === 'jdbc-clickhouse' &&
clickHouseMergeTreeEngines.includes(currentEngine)
Review Comment:
`currentEngine` is read from the `engine` top-level field, but elsewhere in
this component ClickHouse engine is also derived from `values?.properties` (key
`engine`). If the form stores engine under `properties` (or if `engine` is
unset), `isCurrentMergeTree` can be computed incorrectly and MergeTree-required
Sort Orders validation may be skipped. Align submit-time engine detection with
the same source of truth used for `clickHouseEngine` (or derive it from the
actual form structure used for engine).
##########
web-v2/web/src/config/catalog.js:
##########
@@ -223,6 +223,32 @@ export const tableDefaultProps = {
select: ['lance', 'delta'],
description: 'The format of the table'
}
+ ],
+ 'jdbc-clickhouse': [
+ {
+ key: 'engine',
+ defaultValue: 'MergeTree',
+ select: [
+ 'MergeTree',
+ 'ReplacingMergeTree',
+ 'SummingMergeTree',
+ 'AggregatingMergeTree',
+ 'CollapsingMergeTree',
+ 'VersionedCollapsingMergeTree',
+ 'GraphiteMergeTree',
+ 'ReplicatedMergeTree',
+ 'ReplicatedReplacingMergeTree',
+ 'ReplicatedSummingMergeTree',
+ 'ReplicatedAggregatingMergeTree',
+ 'ReplicatedCollapsingMergeTree',
+ 'ReplicatedVersionedCollapsingMergeTree',
+ 'ReplicatedGraphiteMergeTree',
+ 'Distributed',
+ 'TinyLog',
+ 'Log',
+ 'StripeLog'
+ ]
Review Comment:
The MergeTree engine list is duplicated here and in
`clickHouseMergeTreeEngines` (web/src/config/index.js). This increases drift
risk (e.g., adding/removing an engine in one place but not the other). Consider
reusing a single exported constant (e.g., build this `select` as
`[...]clickHouseMergeTreeEngines, 'Distributed', 'TinyLog', 'Log',
'StripeLog'`) or moving engine definitions into one shared config module.
##########
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:
##########
@@ -471,6 +521,16 @@ export default function CreateTableDialog({ ...props }) {
form.setFieldValue(['indexes', idxIndex, 'name'], item.name)
form.setFieldValue(['indexes', idxIndex, 'indexType'],
capitalizeFirstLetter(item.indexType))
Review Comment:
Index type comparisons and payload building rely on
`startsWith('data_skipping_')`, but when editing an existing table you set
`indexType` using `capitalizeFirstLetter(item.indexType)`. If that transforms
`data_skipping_*` into a different casing, the UI will fail to show
Granularity/Set Max Values and the submit payload will omit `properties` (and
may send an unexpected `indexType` value). Store/compare `indexType` in 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()`.
##########
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:
##########
@@ -118,6 +119,14 @@ export default function CreateTableDialog({ ...props }) {
const isClickHouseDistributedEngine =
provider === 'jdbc-clickhouse' &&
values?.properties?.find(item => item?.key ===
'engine')?.value?.toLowerCase?.() === 'distributed'
+
+ const clickHouseEngine =
+ provider === 'jdbc-clickhouse'
+ ? values?.engine || values?.properties?.find(item => item?.key ===
'engine')?.value
+ : undefined
+
+ const isClickHouseMergeTreeEngine =
+ provider === 'jdbc-clickhouse' &&
clickHouseMergeTreeEngines.includes(clickHouseEngine)
Review Comment:
`clickHouseMergeTreeEngines.includes(clickHouseEngine)` is case-sensitive,
but the engine may come from different sources (form field vs. properties) and
other checks in this file already normalize with `toLowerCase()`. To avoid
inconsistent behavior, normalize the engine value before comparison (e.g.,
compare in a single consistent case) so the MergeTree detection is robust
regardless of value source.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
