Copilot commented on code in PR #9392:
URL: https://github.com/apache/gravitino/pull/9392#discussion_r2592359536
##########
web/web/src/app/metalakes/metalake/rightContent/CreateTableDialog.js:
##########
@@ -406,6 +411,48 @@ const CreateTableDialog = props => {
console.error('Form validation errors:', errors)
}
+ /**
+ * Effect to initialize default properties when creating a new table
+ * For lakehouse-generic, checks if location is set in catalog or schema
+ */
+ useEffect(() => {
+ const initDefaultProps = async () => {
+ if (open && type === 'create' && propInfo.defaultProps &&
propInfo.defaultProps.length > 0) {
+ let isLocationRequired = false
+
+ // For lakehouse-generic, check if location is set in catalog or schema
+ if (currentCatalog?.provider === 'lakehouse-generic') {
+ try {
+ // Check catalog properties for location
+ const [catalogErr, catalogRes] = await to(getCatalogDetailsApi({
metalake, catalog }))
+ const catalogLocation = catalogRes?.catalog?.properties?.location
+
+ // Check schema properties for location
+ const [schemaErr, schemaRes] = await to(getSchemaDetailsApi({
metalake, catalog, schema: schemaName }))
+ const schemaLocation = schemaRes?.schema?.properties?.location
+
+ // Location is required if not set in both catalog and schema
+ isLocationRequired = !catalogLocation && !schemaLocation
Review Comment:
The error responses from the API calls are captured but not checked. If
either `catalogErr` or `schemaErr` is set, the code should handle these errors
appropriately rather than silently continuing. Currently, errors are caught by
the outer try-catch but the error variables from the `to()` helper are ignored.
Consider checking the error variables:
```javascript
const [catalogErr, catalogRes] = await to(
getCatalogDetailsApi({ metalake, catalog })
)
if (catalogErr) {
console.error('Error fetching catalog details:', catalogErr)
// Handle error appropriately
}
const catalogLocation = catalogRes?.catalog?.properties?.location
```
```suggestion
if (catalogErr) {
console.error('Error fetching catalog details:', catalogErr)
}
const catalogLocation = catalogRes?.catalog?.properties?.location
// Check schema properties for location
const [schemaErr, schemaRes] = await to(getSchemaDetailsApi({
metalake, catalog, schema: schemaName }))
if (schemaErr) {
console.error('Error fetching schema details:', schemaErr)
}
const schemaLocation = schemaRes?.schema?.properties?.location
// Location is required if not set in both catalog and schema,
or if there was an error fetching either
isLocationRequired = (!catalogLocation && !schemaLocation) ||
catalogErr || schemaErr
```
##########
web/web/src/app/metalakes/metalake/rightContent/CreateTableDialog.js:
##########
@@ -406,6 +411,48 @@ const CreateTableDialog = props => {
console.error('Form validation errors:', errors)
}
+ /**
+ * Effect to initialize default properties when creating a new table
+ * For lakehouse-generic, checks if location is set in catalog or schema
+ */
+ useEffect(() => {
+ const initDefaultProps = async () => {
+ if (open && type === 'create' && propInfo.defaultProps &&
propInfo.defaultProps.length > 0) {
+ let isLocationRequired = false
+
+ // For lakehouse-generic, check if location is set in catalog or schema
+ if (currentCatalog?.provider === 'lakehouse-generic') {
+ try {
+ // Check catalog properties for location
+ const [catalogErr, catalogRes] = await to(getCatalogDetailsApi({
metalake, catalog }))
+ const catalogLocation = catalogRes?.catalog?.properties?.location
+
+ // Check schema properties for location
+ const [schemaErr, schemaRes] = await to(getSchemaDetailsApi({
metalake, catalog, schema: schemaName }))
+ const schemaLocation = schemaRes?.schema?.properties?.location
+
+ // Location is required if not set in both catalog and schema
+ isLocationRequired = !catalogLocation && !schemaLocation
+ } catch (error) {
+ console.error('Error checking location in catalog/schema:', error)
+ }
+ }
+
+ const defaultPropertyItems = propInfo.defaultProps.map(prop => ({
+ key: prop.key,
+ value: prop.value || '',
+ required: prop.key === 'location' ? isLocationRequired :
prop.required || false,
+ description: prop.description || '',
+ disabled: prop.disabled || false
+ }))
+ setInnerProps(defaultPropertyItems)
+ setValue('propItems', defaultPropertyItems)
+ }
+ }
+
+ initDefaultProps()
+ }, [open, type, propInfo, setValue, currentCatalog, metalake, catalog,
schemaName])
+
/**
* Effect to populate form when editing existing table
*/
Review Comment:
The `initDefaultProps` async function is called on every render when
dependencies change, including when `propInfo` changes. For lakehouse-generic
catalogs, this makes two API calls each time. Consider adding a check to skip
the API calls if they've already been made or if the dialog is being reopened
with the same parameters. Also, consider memoizing `propInfo` if it's being
recreated on each render, which could cause unnecessary API calls.
##########
web/web/src/lib/utils/initial.js:
##########
@@ -607,6 +612,26 @@ const relationalColumnTypeMap = {
'timestamp_tz',
'varchar'
],
+ 'lakehouse-generic': [
+ 'binary',
+ 'boolean',
+ 'byte',
+ 'date',
+ 'decimal',
+ 'double',
+ 'external',
+ 'fixed',
+ 'float',
+ 'integer',
+ 'interval_day',
+ 'interval_year',
+ 'long',
+ 'null',
+ 'short',
+ 'string',
+ 'time',
+ 'timestamp'
+ ],
Review Comment:
The column types for 'lakehouse-generic' include several types that may need
clarification or documentation. Specifically:
- 'external' - This is unusual for a column type and may need explanation
- 'null' - This is typically not used as a column type in most systems
- 'interval_day' and 'interval_year' - These appear specific to certain
systems
Consider adding inline comments or documentation to explain these type
choices and their usage, or verify that these are the correct types for the
lakehouse-generic provider.
##########
web/web/src/app/metalakes/metalake/rightContent/CreateTableDialog.js:
##########
@@ -327,6 +330,8 @@ const CreateTableDialog = props => {
const submitForm = formData => {
const hasErrorProperties = innerProps.some(prop => prop.hasDuplicateKey ||
prop.isReserved || prop.invalid)
+ const hasRequiredEmptyProperties = innerProps.some(prop => prop.required
&& !prop.value?.trim())
Review Comment:
The validation logic uses `!prop.value?.trim()` to check for required empty
properties, but the yup schema at line 123-126 only checks if the value is
required without trimming. This inconsistency means that a value with only
whitespace could pass yup validation but fail the manual check. Consider using
`.trim()` in the yup schema as well:
```javascript
value: yup.string().when('required', {
is: true,
then: schema => schema.trim().required('Value is required')
})
```
--
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]
