Copilot commented on code in PR #12339:
URL: https://github.com/apache/gravitino/pull/12339#discussion_r3719712238
##########
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:
##########
@@ -686,7 +781,7 @@ export default function CreateTableDialog({ ...props }) {
}
})
}
- if (sortOredsInfo) {
+ if (sortOredsInfo && isSortOrdersRequired) {
submitData['sortOrders'] = values.sortOrders?.map(s => {
Review Comment:
The Sort Orders tab and submit payload are currently gated by
`isSortOrdersRequired` (MergeTree only). This makes sort orders impossible to
create/submit for non-MergeTree engines even if the user provides them, and
contradicts the stated behavior that sort orders are optional (not unavailable)
for other engines. Consider always rendering the Sort Orders tab when
`sortOredsInfo` is supported, always submitting `sortOrders` when present, and
only applying the *required* validation when the engine is MergeTree-family.
##########
web-v2/web/src/config/catalog.js:
##########
@@ -223,6 +225,41 @@ export const tableDefaultProps = {
select: ['lance', 'delta'],
description: 'The format of the table'
}
+ ],
+ 'jdbc-clickhouse': [
+ {
+ key: 'engine',
+ defaultValue: 'MergeTree',
+ selectGroups: clickHouseEngineGroups
+ },
+ {
+ key: 'cluster-name',
+ defaultValue: '',
+ parentField: 'engine',
+ show: ['Distributed'],
+ description: 'The cluster name for DDL operations'
+ },
+ {
+ key: 'cluster-remote-database',
+ defaultValue: '',
+ parentField: 'engine',
+ show: ['Distributed'],
+ description: 'The remote database name for ClickHouse distributed tables'
+ },
+ {
+ key: 'cluster-remote-table',
+ defaultValue: '',
+ parentField: 'engine',
+ show: ['Distributed'],
+ description: 'The remote table name for ClickHouse distributed tables'
+ },
+ {
+ key: 'cluster-sharding-key',
+ defaultValue: '',
+ parentField: 'engine',
+ show: ['Distributed'],
+ description: 'The sharding key for ClickHouse distributed tables'
+ }
Review Comment:
The PR description states these four Distributed engine fields are 'required
validation', but the diff only adds conditional display metadata; no required
validation is introduced for these keys. Add engine-aware required rules (or
submit-time checks) when `engine === 'Distributed'`, otherwise users can submit
invalid Distributed definitions.
##########
web-v2/web/src/app/catalogs/rightContent/CreateTableDialog.js:
##########
@@ -197,10 +237,17 @@ export default function CreateTableDialog({ ...props }) {
key: 'partitions'
})
}
- if (sortOredsInfo) {
- ;``
+ if (sortOredsInfo && isSortOrdersRequired) {
tabs.push({
- label: <span className='font-normal text-[rgb(0,0,0,0.88)]'>Sort
Orders</span>,
+ label: (
+ <span
+ className={cn('font-normal text-[rgb(0,0,0,0.88)]', {
+ 'before:mr-0.5 before:font-["SimSun"] before:text-[#ff4d4f]
before:content-["*"]': isSortOrdersRequired
+ })}
+ >
+ Sort Orders
+ </span>
+ ),
key: 'sortOrders'
})
}
Review Comment:
The Sort Orders tab and submit payload are currently gated by
`isSortOrdersRequired` (MergeTree only). This makes sort orders impossible to
create/submit for non-MergeTree engines even if the user provides them, and
contradicts the stated behavior that sort orders are optional (not unavailable)
for other engines. Consider always rendering the Sort Orders tab when
`sortOredsInfo` is supported, always submitting `sortOrders` when present, and
only applying the *required* validation when the engine is MergeTree-family.
--
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]
