Copilot commented on code in PR #9923:
URL: https://github.com/apache/gravitino/pull/9923#discussion_r2782048889
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -247,92 +248,128 @@ export const setIntoTreeNodeWithFetch = createAsyncThunk(
const loaded = loadedNodes.filter(key => !reloadedKeys.includes(key))
dispatch(setLoadedNodes(loaded))
}
- } else if (pathArr.length === 4 && type === 'relational') {
- const [err, res] = await to(getTablesApi({ metalake, catalog, schema }))
+ } else if (pathArr.length === 4) {
+ const [funcErr, funcRes] = await to(getFunctionsApi({ metalake, catalog,
schema, details: false }))
- if (err || !res) {
- throw new Error(err)
+ if (funcErr || !funcRes) {
+ throw new Error(funcErr)
}
- const { identifiers = [] } = res || {}
-
- result.data = identifiers.map(tableItem => {
+ const { identifiers: functionIdentifiers = [] } = funcRes || {}
+ const functions = functionIdentifiers.map(functionItem => {
+ const functionName = functionItem?.name ||
functionItem?.identifier?.name || functionItem
return {
- ...tableItem,
- node: 'table',
- id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${tableItem.name}}}`,
- key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${tableItem.name}}}`,
- path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, table: tableItem.name }).toString()}`,
- name: tableItem.name,
- title: tableItem.name,
+ ...functionItem,
+ node: 'function',
+ id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${functionName}}}`,
+ key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${functionName}}}`,
+ path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, function: functionName }).toString()}`,
+ name: functionName,
+ title: functionName,
isLeaf: true,
- columns: [],
children: []
}
})
- } else if (pathArr.length === 4 && type === 'fileset') {
- const [err, res] = await to(getFilesetsApi({ metalake, catalog, schema
}))
- if (err || !res) {
- throw new Error(err)
- }
+ switch (type) {
+ case 'relational': {
+ const [tableErr, tableRes] = await to(getTablesApi({ metalake,
catalog, schema }))
+ if (tableErr || !tableRes) {
+ throw new Error(tableErr)
+ }
- const { identifiers = [] } = res || {}
+ const { identifiers: tableIdentifiers = [] } = tableRes || {}
+ const tables = tableIdentifiers.map(tableItem => {
+ return {
+ ...tableItem,
+ node: 'table',
+ id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${tableItem.name}}}`,
+ key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${tableItem.name}}}`,
+ path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, table: tableItem.name }).toString()}`,
+ name: tableItem.name,
+ title: tableItem.name,
+ isLeaf: true,
+ columns: [],
+ children: []
+ }
+ })
- result.data = identifiers.map(filesetItem => {
- return {
- ...filesetItem,
- node: 'fileset',
- id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${filesetItem.name}}}`,
- key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${filesetItem.name}}}`,
- path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, fileset: filesetItem.name }).toString()}`,
- name: filesetItem.name,
- title: filesetItem.name,
- isLeaf: true
+ result.data = [...tables, ...functions]
+ break
}
- })
- } else if (pathArr.length === 4 && type === 'messaging') {
- const [err, res] = await to(getTopicsApi({ metalake, catalog, schema }))
-
- if (err || !res) {
- throw new Error(err)
- }
+ case 'fileset': {
+ const [filesetErr, filesetRes] = await to(getFilesetsApi({ metalake,
catalog, schema }))
+ if (filesetErr || !filesetRes) {
+ throw new Error(filesetErr)
+ }
- const { identifiers = [] } = res || {}
+ const { identifiers: filesetIdentifiers = [] } = filesetRes || {}
+ const filesets = filesetIdentifiers.map(filesetItem => {
+ return {
+ ...filesetItem,
+ node: 'fileset',
+ id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${filesetItem.name}}}`,
+ key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${filesetItem.name}}}`,
+ path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, fileset: filesetItem.name }).toString()}`,
+ name: filesetItem.name,
+ title: filesetItem.name,
+ isLeaf: true
+ }
+ })
- result.data = identifiers.map(topicItem => {
- return {
- ...topicItem,
- node: 'topic',
- id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${topicItem.name}}}`,
- key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${topicItem.name}}}`,
- path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, topic: topicItem.name }).toString()}`,
- name: topicItem.name,
- title: topicItem.name,
- isLeaf: true
+ result.data = [...filesets, ...functions]
+ break
}
- })
- } else if (pathArr.length === 4 && type === 'model') {
- const [err, res] = await to(getModelsApi({ metalake, catalog, schema }))
+ case 'messaging': {
+ const [topicErr, topicRes] = await to(getTopicsApi({ metalake,
catalog, schema }))
+ if (topicErr || !topicRes) {
+ throw new Error(topicErr)
+ }
- if (err || !res) {
- throw new Error(err)
- }
+ const { identifiers: topicIdentifiers = [] } = topicRes || {}
+ const topics = topicIdentifiers.map(topicItem => {
+ return {
+ ...topicItem,
+ node: 'topic',
+ id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${topicItem.name}}}`,
+ key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${topicItem.name}}}`,
+ path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, topic: topicItem.name }).toString()}`,
+ name: topicItem.name,
+ title: topicItem.name,
+ isLeaf: true
+ }
+ })
- const { identifiers = [] } = res || {}
+ result.data = [...topics, ...functions]
+ break
+ }
+ case 'model': {
+ const [modelErr, modelRes] = await to(getModelsApi({ metalake,
catalog, schema }))
+ if (modelErr || !modelRes) {
+ throw new Error(modelErr)
+ }
- result.data = identifiers.map(modelItem => {
- return {
- ...modelItem,
- node: 'model',
- id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${modelItem.name}}}`,
- key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${modelItem.name}}}`,
- path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, model: modelItem.name }).toString()}`,
- name: modelItem.name,
- title: modelItem.name,
- isLeaf: true
+ const { identifiers: modelIdentifiers = [] } = modelRes || {}
+ const models = modelIdentifiers.map(modelItem => {
+ return {
+ ...modelItem,
+ node: 'model',
+ id:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${modelItem.name}}}`,
+ key:
`{{${metalake}}}{{${catalog}}}{{${type}}}{{${schema}}}{{${modelItem.name}}}`,
+ path: `?${new URLSearchParams({ metalake, catalog, catalogType:
type, schema, model: modelItem.name }).toString()}`,
+ name: modelItem.name,
+ title: modelItem.name,
+ isLeaf: true
+ }
+ })
+
+ result.data = [...models, ...functions]
+ break
}
- })
+ default:
+ result.data = functions
+ break
+ }
Review Comment:
`setIntoTreeNodeWithFetch` appends `functions` under *all* catalog types
(`fileset`, `messaging`, `model`, and `default`). However the UI
routing/details handling appears to only support function details when
`catalogType === 'relational'` (see `RightContent`). This can produce function
nodes in the tree that navigate to unsupported pages/empty details. Either
limit function nodes to `relational` schemas, or extend the routing/details
pages to support `function` for the other catalog types.
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/Functions.js:
##########
@@ -0,0 +1,101 @@
+/*
+ * 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.
+ */
+
+'use client'
+
+import { useEffect, useMemo } from 'react'
+import { Spin, Table } from 'antd'
+import { useAntdColumnResize } from 'react-antd-column-resize'
+import { useAppDispatch, useAppSelector } from '@/lib/hooks/useStore'
+import { fetchFunctions } from '@/lib/store/metalakes'
+import { formatToDateTime, isValidDate } from '@/lib/utils/date'
+import Link from 'next/link'
+
+export default function Functions({ metalake, catalog, schema }) {
+ const dispatch = useAppDispatch()
+ const store = useAppSelector(state => state.metalakes)
+
+ useEffect(() => {
+ if (!metalake || !catalog || !schema) return
+ dispatch(fetchFunctions({ metalake, catalog, schema, init: true }))
+ }, [dispatch, metalake, catalog, schema])
+
+ const columns = useMemo(
+ () => [
+ {
+ title: 'Name',
+ dataIndex: 'name',
+ key: 'name',
+ width: 240,
+ ellipsis: true,
+ sorter: (a, b) =>
a?.name.toLowerCase().localeCompare(b?.name.toLowerCase()),
+ render: name => (
+ <Link
+ data-refer={`function-link-${name}`}
+
href={`/catalogs?metalake=${encodeURIComponent(metalake)}&catalogType=relational&catalog=${encodeURIComponent(catalog)}&schema=${encodeURIComponent(schema)}&function=${encodeURIComponent(name)}`}
+ >
+ {name}
+ </Link>
+ )
+ },
Review Comment:
The `columns` memo is created with an empty dependency array, but the column
`render` builds a Link using `metalake`, `catalog`, and `schema` props. If
those props change without an unmount, the Links will keep pointing at the old
values. Include these props in the `useMemo` dependency list (or avoid
memoizing the columns).
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -247,92 +248,128 @@ export const setIntoTreeNodeWithFetch = createAsyncThunk(
const loaded = loadedNodes.filter(key => !reloadedKeys.includes(key))
dispatch(setLoadedNodes(loaded))
}
- } else if (pathArr.length === 4 && type === 'relational') {
- const [err, res] = await to(getTablesApi({ metalake, catalog, schema }))
+ } else if (pathArr.length === 4) {
+ const [funcErr, funcRes] = await to(getFunctionsApi({ metalake, catalog,
schema, details: false }))
- if (err || !res) {
- throw new Error(err)
+ if (funcErr || !funcRes) {
+ throw new Error(funcErr)
}
Review Comment:
In `setIntoTreeNodeWithFetch` (schema children load), `getFunctionsApi(...)`
is called unconditionally and the thunk throws if it fails. This means a
failure/404 from the functions endpoint prevents tables/filesets/topics/models
from loading in the tree at all. Consider scoping the functions request to
supported catalog types (likely `relational`), or treating function-list errors
as non-fatal (e.g., fall back to the non-function entities and optionally
toast/log).
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/Functions.js:
##########
@@ -0,0 +1,101 @@
+/*
+ * 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.
+ */
+
+'use client'
+
+import { useEffect, useMemo } from 'react'
+import { Spin, Table } from 'antd'
+import { useAntdColumnResize } from 'react-antd-column-resize'
+import { useAppDispatch, useAppSelector } from '@/lib/hooks/useStore'
+import { fetchFunctions } from '@/lib/store/metalakes'
+import { formatToDateTime, isValidDate } from '@/lib/utils/date'
+import Link from 'next/link'
+
+export default function Functions({ metalake, catalog, schema }) {
+ const dispatch = useAppDispatch()
+ const store = useAppSelector(state => state.metalakes)
+
+ useEffect(() => {
+ if (!metalake || !catalog || !schema) return
+ dispatch(fetchFunctions({ metalake, catalog, schema, init: true }))
+ }, [dispatch, metalake, catalog, schema])
+
+ const columns = useMemo(
+ () => [
+ {
+ title: 'Name',
+ dataIndex: 'name',
+ key: 'name',
+ width: 240,
+ ellipsis: true,
+ sorter: (a, b) =>
a?.name.toLowerCase().localeCompare(b?.name.toLowerCase()),
Review Comment:
The Name sorter calls `a?.name.toLowerCase()` / `b?.name.toLowerCase()`
without guarding for `name` being undefined/non-string, which can throw at
runtime. Use a safe fallback (e.g., `(a?.name ?? '').toString().toLowerCase()`).
```suggestion
sorter: (a, b) => {
const aName = (a?.name ?? '').toString().toLowerCase()
const bName = (b?.name ?? '').toString().toLowerCase()
return aName.localeCompare(bName)
},
```
##########
web-v2/web/src/app/catalogs/TreeComponent.js:
##########
@@ -292,6 +292,12 @@ export const TreeComponent = forwardRef(function
TreeComponent(props, ref) {
<Icons.iconify icon='mdi:globe-model' className='my-icon-small' />
</span>
)
+ case 'function':
+ return (
+ <span role='img' aria-label='frown' className='anticon
anticon-frown'>
+ <Icons.iconify icon='material-symbols:function'
className='my-icon-small' />
+ </span>
Review Comment:
The function tree icon uses `aria-label='frown'`, which is not descriptive
of the icon’s meaning and is confusing for screen readers. Use a meaningful
label (e.g., "function") for this new node type (and ideally align the other
node icons similarly over time).
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -1613,6 +1650,32 @@ export const deleteVersion = createAsyncThunk(
}
)
+export const fetchFunctions = createAsyncThunk(
+ 'appMetalakes/fetchFunctions',
+ async ({ init, metalake, catalog, schema }, { dispatch }) => {
+ if (init) {
+ await dispatch(setTableLoading(true))
+ }
+
+ const [err, res] = await to(getFunctionsApi({ metalake, catalog, schema,
details: true }))
+ await dispatch(setTableLoading(false))
Review Comment:
`fetchFunctions` always dispatches `setTableLoading(false)` even when `init`
is false (i.e., when it didn't set loading to true). This can inadvertently
clear loading state that was set by other thunks. Gate the "set false" call the
same way as the "set true" call, or move functions to their own loading flag.
```suggestion
if (init) {
await dispatch(setTableLoading(false))
}
```
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -2103,6 +2168,14 @@ export const appMetalakesSlice = createSlice({
toast.error(action.error.message)
}
})
+ builder.addCase(fetchFunctions.fulfilled, (state, action) => {
+ state.functions = action.payload.functions
Review Comment:
`fetchFunctions.fulfilled` overwrites `state.functions` regardless of which
schema is currently active, and there is no clear-on-pending behavior. If users
switch schemas quickly, an out-of-order response can populate functions for the
previous schema. Consider tracking the active `{metalake,catalog,schema}` for
the functions list and ignoring stale responses (or using `abort`/requestId
checks in the thunk).
```suggestion
const requestArgs = action.meta && action.meta.arg ? action.meta.arg :
{}
const { metalake, catalog, schema } = requestArgs
const { metalakeInUse, catalogInUse, schemaInUse } = state
// If we don't have an active selection recorded, preserve existing
behavior.
if (
metalakeInUse === undefined ||
catalogInUse === undefined ||
schemaInUse === undefined
) {
state.functions = action.payload.functions
return
}
// Only update functions if the response still matches the active
selection.
if (
metalake === metalakeInUse &&
catalog === catalogInUse &&
schema === schemaInUse
) {
state.functions = action.payload.functions
}
```
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -1613,6 +1650,32 @@ export const deleteVersion = createAsyncThunk(
}
)
+export const fetchFunctions = createAsyncThunk(
+ 'appMetalakes/fetchFunctions',
+ async ({ init, metalake, catalog, schema }, { dispatch }) => {
+ if (init) {
+ await dispatch(setTableLoading(true))
+ }
+
+ const [err, res] = await to(getFunctionsApi({ metalake, catalog, schema,
details: true }))
+ await dispatch(setTableLoading(false))
Review Comment:
`fetchFunctions` toggles the shared `tableLoading` flag. On schema load
(`catalogs/page.js`) functions are fetched in parallel with
tables/filesets/topics/models, so whichever request finishes first will set
`tableLoading(false)` and potentially stop the spinner while other entity
requests are still in flight. Use a dedicated loading flag for functions (e.g.,
`functionsLoading`) or a ref-counted loading mechanism instead of sharing
`tableLoading` across independent requests.
```suggestion
const [err, res] = await to(getFunctionsApi({ metalake, catalog, schema,
details: true }))
```
##########
web-v2/web/src/app/catalogs/rightContent/RightContent.js:
##########
@@ -220,7 +228,16 @@ const RightContent = () => {
...(entity
? [
{
- title: (
+ title: functionName ? (
+ <Link
+
href={`/catalogs?metalake=${currentMetalake}&catalogType=${catalogType}&catalog=${decodeURIComponent(catalog)}&schema=${decodeURIComponent(schema)}`}
+ title={entity}
+ className='inline-block min-w-10 truncate'
+ style={{ maxWidth: `calc((${width}px - 160px)/4)` }}
+ >
+ {decodeURIComponent(entity)}
+ </Link>
Review Comment:
In the new breadcrumb case for functions, the `href` is built using
`decodeURIComponent(catalog)` / `decodeURIComponent(schema)`. These values are
being inserted into a URL query string and should be encoded, not decoded,
otherwise names containing reserved characters can break navigation. Use
`encodeURIComponent(...)` (or `URLSearchParams`) when constructing the link URL.
--
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]
