jerryshao commented on code in PR #12900: URL: https://github.com/apache/gravitino/pull/12900#discussion_r4291552040
########## web-v2/web/src/app/catalogs/rightContent/SemanticMetadataEditor.js: ########## @@ -0,0 +1,390 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useEffect, useRef, useState } from 'react' +import { Alert, Button, Form, Input, Modal, Select, Space, Tabs, theme } from 'antd' +import { isEqual } from 'lodash-es' +import { useAppDispatch } from '@/lib/hooks/useStore' +import { updateSemanticModel } from '@/lib/store/metalakes' +import { semanticDataTypes, validateSemanticModelDefinition } from '@/lib/utils/semanticModel' +import { + getSemanticMetadataEntity, + semanticMetadataFormValues, + updateSemanticMetadata +} from '@/lib/utils/semanticMetadata' + +export default function SemanticMetadataEditor(props) { + try { + getSemanticMetadataEntity(props.definition, props.target) + } catch (error) { + return ( + <Modal + open + title='Metadata unavailable' + onCancel={props.onClose} + footer={<Button onClick={props.onClose}>Close</Button>} + > + <Alert type='error' showIcon message={error.message} /> + </Modal> + ) + } + + return <SemanticMetadataForm {...props} /> +} + +function SemanticMetadataForm({ definition, target, metalake, catalog, schema, semanticModel, onClose, onSaved }) { + const { token } = theme.useToken() + const dispatch = useAppDispatch() + const [form] = Form.useForm() + const [saving, setSaving] = useState(false) + const [errors, setErrors] = useState([]) + const [section, setSection] = useState(target.kind === 'model' ? 'context' : target.section || 'basic') + const [focusField, setFocusField] = useState(null) + const [confirmDiscard, setConfirmDiscard] = useState(false) + const errorRef = useRef(null) + const expressionRef = useRef(null) + + useEffect(() => { + if (!focusField) return + + // Tabs reveal their panel after the active key changes; focus once it is visible. + const frame = requestAnimationFrame(() => { + form.scrollToField(focusField, { block: 'nearest' }) + const input = form.getFieldInstance(focusField) + if (input?.focus) input.focus() + else if (focusField[0] === 'dialects') expressionRef.current?.focus() + setFocusField(null) + }) + + return () => cancelAnimationFrame(frame) + }, [section, focusField, form]) + + useEffect(() => { + if (errors.length) errorRef.current?.focus() + }, [errors]) + + // Keep the editing snapshot stable through list/detail refreshes while the dialog is open. + const [original] = useState(definition) + const entity = getSemanticMetadataEntity(original, target) + const [initial] = useState(() => semanticMetadataFormValues(entity)) + const structuredContextInitialized = useRef(initial.contextFormat === 'structured') + const contextFormat = Form.useWatch('contextFormat', form) || initial.contextFormat + const hasExpression = ['field', 'metric'].includes(target.kind) + const title = target.kind === 'model' ? 'Edit Model AI Context' : `Edit ${target.kind} ${target.name}` + + const close = () => { + if (saving) return + if (isEqual(initial, form.getFieldsValue(true))) onClose() + else setConfirmDiscard(true) + } + + const save = async () => { + try { + await form.validateFields() + } catch (failure) { + const name = failure.errorFields?.[0]?.name + if (name) { + setSection( + name[0] === 'dialects' + ? 'expressions' + : ['contextFormat', 'contextText', 'instructions', 'synonyms', 'examples'].includes(name[0]) + ? 'context' + : 'basic' + ) + setFocusField(name) + } + + return + } + const updated = updateSemanticMetadata(original, target, form.getFieldsValue(true)) + const problems = validateSemanticModelDefinition(updated) + setErrors(problems) + if (problems.length) return + if (isEqual(original, updated)) { + onClose() + + return + } + setSaving(true) + try { + const result = await dispatch( + updateSemanticModel({ + metalake, + catalog, + schema, + semanticModel, + data: { updates: [{ '@type': 'replaceDefinition', definition: updated }] } Review Comment: [Question] This save sends a whole-document `replaceDefinition` built from `original`, which is frozen when the dialog opens (`const [original] = useState(definition)`, line 83, deliberately so that list/detail refreshes do not disturb the draft). The consequence is that any change landing between open and save - another browser tab, another user, or the OSSIE import - is silently discarded, and the user gets no hint that they overwrote it. The REST API has no version token, so a true compare-and-set is not available, but the editor could re-load the model just before saving and refuse (or warn) when the server definition no longer equals `original`. Is the silent last-writer-wins behaviour the intended trade-off here, or worth a guard? Same question applies to the JSON dialog's `replaceDefinition` path. Verified by: read SemanticMetadataEditor.js lines 60-127 and CreateSemanticModelDialog.js lines 124-171 on this branch; confirmed `SemanticModelUpdateRequest.ReplaceSemanticModelDefinitionRequest` carries only the definition, with no expected-version field. ########## web-v2/web/src/lib/utils/semanticMetadata.js: ########## @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { cloneDeep, isEqual } from 'lodash-es' + +const editableProperties = { + model: [], + dataset: ['description'], + field: ['label', 'description', 'datatype'], + metric: ['description', 'datatype'], + relationship: ['from', 'to', 'fromColumns', 'toColumns'] +} + +const missingMetadataMessage = 'The selected metadata no longer exists. Reload the model and try again.' + +/** Finds the selected entity without flattening dataset-scoped field names. */ +export const getSemanticMetadataEntity = (definition, target) => { + const entity = + target.kind === 'model' + ? definition + : target.kind === 'field' + ? definition?.datasets + ?.find(dataset => dataset.name === target.datasetName) + ?.fields?.find(field => field.name === target.name) + : definition?.[`${target.kind}s`]?.find(item => item.name === target.name) + if (!entity) throw new Error(missingMetadataMessage) + + return entity +} + +/** Supplies form values while keeping the original wire representation separate. */ +export const semanticMetadataFormValues = entity => { + if (!entity) throw new Error(missingMetadataMessage) + + return { + ...cloneDeep(entity), + dialects: cloneDeep(entity.expression?.dialects || []), + contextFormat: typeof entity.aiContext === 'string' ? 'text' : 'structured', + contextText: typeof entity.aiContext === 'string' ? entity.aiContext : '', + instructions: typeof entity.aiContext === 'string' ? entity.aiContext : entity.aiContext?.instructions || '', + synonyms: cloneDeep(entity.aiContext?.synonyms || []), + examples: cloneDeep(entity.aiContext?.examples || []) + } +} + +/** Applies only edited form values, preserving every other entity and extension in the definition. */ +export const updateSemanticMetadata = (definition, target, values) => { + const updated = cloneDeep(definition) + const entity = getSemanticMetadataEntity(updated, target) + const initial = semanticMetadataFormValues(entity) + for (const key of editableProperties[target.kind]) { + const value = key === 'datatype' && values[key] === '' ? null : values[key] + if (!isEqual(initial[key], value)) entity[key] = value ?? null + } + if (['field', 'metric'].includes(target.kind) && !isEqual(initial.dialects, values.dialects)) { + entity.expression = { ...entity.expression, dialects: cloneDeep(values.dialects) } + } + + if (values.contextFormat === 'text') { + if (initial.contextFormat !== 'text' || initial.contextText !== values.contextText) { + // Existing structured guidance stays structured so its extra keys cannot be discarded. + if (entity.aiContext && typeof entity.aiContext === 'object') { + entity.aiContext.instructions = values.contextText + } else { + entity.aiContext = values.contextText Review Comment: [Nit] Clearing the text guidance writes `aiContext: ""` rather than removing the key, so there is no way to delete AI context once it exists. Afterwards `hasMetadata` (SemanticModelDetailsPage.js:64) treats `""` as falsy, so the AI Context section disappears from the UI while the Definition tab still shows the empty `aiContext` on the entity - the same mismatch appears for a cleared `label` or `description`, which become `""` via line 51. The server accepts it (`AIContextDTO.Deserializer` maps an empty string to `text`, and `validateAIContext` only requires exactly one of text/object), so this is cosmetic rather than a failed save. Mapping a blank string to `null`/`delete` for these optional keys would keep the stored definition equal to what the UI shows. Verified by: traced `updateSemanticMetadata` with `contextFormat: 'text'` and an empty `contextText` against `hasMetadata`, and read `AIContextDTO.java:102-120` plus `SemanticModelDefinitionValidator.validateAIContext` to confirm the empty value is accepted. ########## web-v2/web/src/app/catalogs/page.js: ########## @@ -332,33 +357,50 @@ const CatalogsListPage = () => { } } if (func) { - store.functions.length === 0 && (await dispatch(fetchFunctions({ init: true, metalake, catalog, schema }))) - await dispatch(setActivatedDetailsLoading(true)) - await dispatch(getFunctionDetails({ init: true, metalake, catalog, schema, functionName: func })) - await dispatch(setActivatedDetailsLoading(false)) + store.functions.length === 0 && (await load(fetchFunctions({ init: true, metalake, catalog, schema }))) + await load(setActivatedDetailsLoading(true)) + await load(getFunctionDetails({ init: true, metalake, catalog, schema, functionName: func })) + await load(setActivatedDetailsLoading(false)) } if (view) { - store.views.length === 0 && (await dispatch(fetchViews({ init: true, metalake, catalog, schema }))) - await dispatch(setActivatedDetailsLoading(true)) - await dispatch(getViewDetails({ init: true, metalake, catalog, schema, view })) - await dispatch(setActivatedDetailsLoading(false)) + store.views.length === 0 && (await load(fetchViews({ init: true, metalake, catalog, schema }))) + await load(setActivatedDetailsLoading(true)) + await load(getViewDetails({ init: true, metalake, catalog, schema, view })) + await load(setActivatedDetailsLoading(false)) } - } - if (paramsSize === 6 && version) { - if (!store.catalogs.length) { - await dispatch(fetchCatalogs({ metalake })) - await dispatch(fetchSchemas({ metalake, catalog, catalogType })) - await dispatch(fetchModels({ init: true, page: 'schemas', metalake, catalog, schema })) + if (semanticModel) { + if (canceled) return + store.semanticModels.length === 0 && + (await load(fetchSemanticModels({ init: true, metalake, catalog, schema }))) + if (canceled) return + load(resetActivatedDetails()) + load(setActivatedDetailsLoading(true)) + + const detailRequest = load( + getSemanticModelDetails({ init: true, metalake, catalog, schema, semanticModel }) + ) + await detailRequest + if (canceled) return + load(setActivatedDetailsLoading(false)) + load( + getCurrentEntityTags({ + init: true, + metalake, + metadataObjectType: 'semantic_model', Review Comment: [Question] The Semantic Model route fetches tags but, unlike every other entity branch in this effect (catalog, schema, table, fileset, topic, model), it does not dispatch `getCurrentEntityPolicies`, and the detail page has no Policies element. The server side looks ready for it: `MetadataObjectPolicyOperations` resolves the type with `Type.valueOf(type.toUpperCase(Locale.ROOT))` and `SEMANTIC_MODEL` is a valid `MetadataObject.Type` with an entry in `MetadataObjectUtil` (lines 66, 130, 287). Is omitting policies intentional for this PR (e.g. deferred to a follow-up), or an oversight? No stale data results either way - `setActiveCatalogEntity` clears `currentEntityPolicies` - so this is only about the missing capability. Verified by: read the whole effect in page.js on this branch and grepped the Semantic Model detail page for policy usage (none); checked the server operations class and MetadataObjectUtil for SEMANTIC_MODEL support. ########## web-v2/web/src/lib/utils/semanticMetadata.js: ########## @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { cloneDeep, isEqual } from 'lodash-es' + +const editableProperties = { + model: [], + dataset: ['description'], + field: ['label', 'description', 'datatype'], Review Comment: [Nit] The focused field editor can change `label`, `description` and `datatype`, but not `dimension`, even though the detail page renders `Time dimension: {field.dimension?.isTime ? 'Yes' : 'No'}` for every field (SemanticModelDetailsPage.js:467) and `FieldDTO` exposes `dimension` as an editable property (`common/.../semantic/FieldDTO.java:51`). Flipping a field from a time dimension to a plain one therefore still forces the user into the raw JSON definition, which is exactly what this editor exists to avoid. Adding a single boolean (or tri-state, to keep `dimension` absent when unset) control to the Basic information tab would close the gap. `updateSemanticMetadata` already clones and preserves the untouched `dimension`, so nothing else has to change. Verified by: compared `editableProperties` here with the JsonProperty list of DatasetDTO/FieldDTO/MetricDTO/RelationshipDTO; `dimension` is the only displayed-but-uneditable one. ########## web-v2/web/src/app/catalogs/rightContent/entitiesContent/SchemaDetailsPage.js: ########## @@ -804,6 +913,62 @@ export default function SchemaDetailsPage() { components={viewComponents} /> </> + ) : tabKey === 'Semantic Models' ? ( + <> + <Flex justify='flex-end' className='mb-4' gap='middle' wrap> + <Search + className='w-full sm:w-64' + name='searchSemanticModelInput' + placeholder='Search...' + value={search} + onChange={onSearchTable} + /> + <Space wrap> + <Button onClick={() => setOpenImportSemanticModel(true)}>Import OSSIE</Button> + <Button + data-refer='create-semantic-model-btn' + type='primary' + icon={<PlusOutlined />} + onClick={() => { + setEditSemanticModel('') + setOpenSemanticModel(true) + }} + > + Create Semantic Model + </Button> + </Space> + </Flex> + <Spin spinning={store.tableLoading}> Review Comment: [Nit] This spinner is bound to `store.tableLoading`, which `fetchSemanticModels` never sets - only the create/update/delete thunks do (`setTableLoading(true)` around the write). On a first visit to a relational schema the tab therefore renders antd's empty "No data" state while the list request is still in flight, instead of a loading state, and a slow `GET .../semantic-models` looks like an empty schema. The list already has a per-schema request record (`semanticModelRequests`), so a dedicated pending flag - or simply reusing it - would let the tab show a spinner until the first response lands. Verified by: read `fetchSemanticModels` and its three reducers in lib/store/metalakes/index.js on this branch; `tableLoading` appears only in the create/update/delete semantic-model thunks and in the unrelated entity-list thunks. -- 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]
