You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rraulinio (via GitHub)" <gi...@apache.org> on 2026/07/10 10:55:46 UTC
[PR] Add SQL UDF REST catalog read support [iceberg-go]
rraulinio opened a new pull request, #1432:
URL: https://github.com/apache/iceberg-go/pull/1432
## Description
Addressing part 2 from https://github.com/apache/iceberg-go/issues/1360, following the SQL UDF metadata model added in #1418.
This adds read-side SQL UDF support to the REST catalog through a new optional `FunctionCatalog` capability interface. REST catalogs can list functions in a namespace, load a function and check whether a function exists. Loaded functions are represented by a new `udf.UDF` wrapper containing the catalog identifier, parsed UDF metadata and optional metadata location.
The REST implementation supports the two function endpoints currently defined by the Iceberg REST OpenAPI:
- `GET /v1/{prefix}/namespaces/{namespace}/functions`
- `GET /v1/{prefix}/namespaces/{namespace}/functions/{function}`
Function endpoints are not part of the REST spec's assumed default endpoint set, so they are used only when advertised by the server through `ConfigResponse.endpoints`. An unsupported load or existence check returns `ErrEndpointNotSupported`, while an unsupported listing yields no results without issuing a request, matching the existing list behavior for optional endpoints.
`ListFunctions` is a paginated iterator and handles the function-specific `CatalogObjectIdentifier` wire shape: each identifier is a flat array of hierarchy levels, such as `["accounting", "tax", "paid"]`, rather than the `{namespace, name}` object used for tables and views. `LoadFunction` parses the returned metadata using the `udf` package and returns all overloads from the single metadata response. The optional `metadata-location` is preserved when present.
The catalog adds `ErrNoSuchFunction` for `NoSuchFunctionException` responses and validates function identifiers independently from table and view identifiers. `CheckFunctionExists` uses `LoadFunction` because the REST spec does not define a HEAD endpoint for functions; invalid identifiers remain errors, while a server-reported missing function returns `false` without an error.
This PR does not add create, replace or drop operations because standardized REST write endpoints for functions are not currently defined. It also does not resolve overloads or execute UDF SQL bodies; those remain query-engine responsibilities.
The tests cover list and load responses, pagination, early iterator termination, page-size handling, subsequent-page errors, missing namespaces and functions, malformed metadata, identifier validation, GET-based existence checks and endpoint capability negotiation. They also add focused coverage for existing UDF metadata initialization and nil-properties safeguards.
## Tests
- `go test ./...`
- `go test ./catalog/rest -run 'Function|Endpoint' -count=10`
- `go test -race ./catalog ./catalog/rest ./udf -run 'Function|Endpoint|TestUDF|TestBuilderFromBaseNilProperties|TestMetadataInitDefaults' -count=1`
- `go vet ./...`
- `golangci-lint run --timeout=10m`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569391773
##########
catalog/catalog.go:
##########
@@ -39,6 +39,7 @@ import (
"github.com/apache/iceberg-go"
iceinternal "github.com/apache/iceberg-go/internal"
"github.com/apache/iceberg-go/table"
+ "github.com/apache/iceberg-go/udf"
Review Comment:
Agreed. I moved `FunctionCatalog` into `catalog/rest`, removed the `catalog -> udf` dependency and removed the base-package interface assertion. Callers holding a `catalog.Catalog` can now assert `rest.FunctionCatalog`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569415827
##########
catalog/rest/rest.go:
##########
@@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v
return view.New(identifier, metadata, rsp.MetadataLoc), nil
}
+
+// Catalog implements the optional function (SQL UDF) read support defined
+// by the REST spec: list and load. The function endpoints are not part of
+// the spec's assumed default endpoint set, so they are only used when the
+// server advertises them.
+var _ catalog.FunctionCatalog = (*Catalog)(nil)
+
+// ListFunctions returns the function (SQL UDF) identifiers under a
+// namespace, with the returned identifiers containing the information
+// required to load the function via this catalog.
+func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ pageSize := r.getPageSize(ctx)
+ var pageToken string
+
+ for {
+ functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize)
+ if err != nil {
+ yield(table.Identifier{}, err)
+
+ return
+ }
+ for _, function := range functions {
+ if !yield(function, nil) {
+ return
+ }
+ }
+ if nextPageToken == "" {
+ return
+ }
+ pageToken = nextPageToken
+ }
+ }
+}
+
+func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) {
+ if err := checkValidNamespace(namespace); err != nil {
+ return nil, "", err
+ }
+ // Unsupported listing yields an empty result rather than an error.
+ if !r.endpoints.allowed(endpointListFunctions) {
+ return nil, "", nil
+ }
+ ns := r.encodeNamespace(namespace)
+ path, err := endpointListFunctions.reqPath(ns)
+ if err != nil {
+ return nil, "", err
+ }
+ uri := r.baseURI.JoinPath(path...)
+
+ v := url.Values{}
+ if pageSize > 0 {
+ v.Set("pageSize", strconv.Itoa(pageSize))
+ }
+ if pageToken != "" {
+ v.Set("pageToken", pageToken)
+ }
+
+ uri.RawQuery = v.Encode()
+ // Function identifiers are CatalogObjectIdentifier values: ordered
+ // hierarchy levels (the namespace levels followed by the function
+ // name), not the {namespace, name} object tables and views use.
+ type resp struct {
+ Identifiers [][]string `json:"identifiers"`
+ NextPageToken string `json:"next-page-token,omitempty"`
+ }
+
+ rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace})
+ if err != nil {
+ return nil, "", err
+ }
+
+ out := make([]table.Identifier, len(rsp.Identifiers))
+ for i, levels := range rsp.Identifiers {
+ out[i] = table.Identifier(levels)
+ }
+
+ return out, rsp.NextPageToken, nil
+}
+
+// loadFunctionResponse contains the response from loading a function.
+type loadFunctionResponse struct {
+ MetadataLoc string `json:"metadata-location"`
+ RawMetadata json.RawMessage `json:"metadata"`
+}
+
+// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded
+// definitions are included in the single metadata response.
+func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) {
+ if err := r.endpoints.check(endpointLoadFunction); err != nil {
+ return nil, err
+ }
+
+ ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ if err != nil {
+ return nil, err
+ }
+
+ path, err := endpointLoadFunction.reqPath(ns, fn)
+ if err != nil {
+ return nil, err
+ }
+
+ rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path,
+ r.cl, map[int]error{
+ http.StatusNotFound: catalog.ErrNoSuchFunction,
+ })
+ if err != nil {
+ return nil, err
+ }
+
+ metadata, err := udf.ParseMetadataBytes(rsp.RawMetadata)
Review Comment:
Added, a successful response without the required `metadata` field now returns a clear `ErrRESTError`, with regression coverage.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569420533
##########
udf/udf.go:
##########
@@ -0,0 +1,58 @@
+// 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.
+
+package udf
+
+import (
+ "slices"
+
+ "github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/table"
+)
+
+// UDF is a function (SQL UDF) loaded from a catalog: its catalog identifier,
+// its metadata, and the location the metadata was loaded from.
+type UDF struct {
+ identifier table.Identifier
+ metadata Metadata
+ metadataLocation string
+}
+
+// New creates a UDF from its catalog identifier, metadata and metadata
+// location.
+func New(ident table.Identifier, meta Metadata, metadataLocation string) *UDF {
+ return &UDF{
+ identifier: ident,
+ metadata: meta,
+ metadataLocation: metadataLocation,
+ }
+}
+
+// Equals reports whether two UDFs have the same identifier, metadata
+// location and metadata.
+func (u UDF) Equals(other UDF) bool {
Review Comment:
Kept the value receiver and argument to match `view.View` and added documentation making the intentional value-copy semantics and pointer-call usage explicit.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569404981
##########
catalog/rest/rest.go:
##########
@@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v
return view.New(identifier, metadata, rsp.MetadataLoc), nil
}
+
+// Catalog implements the optional function (SQL UDF) read support defined
+// by the REST spec: list and load. The function endpoints are not part of
+// the spec's assumed default endpoint set, so they are only used when the
+// server advertises them.
+var _ catalog.FunctionCatalog = (*Catalog)(nil)
+
+// ListFunctions returns the function (SQL UDF) identifiers under a
+// namespace, with the returned identifiers containing the information
+// required to load the function via this catalog.
+func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ pageSize := r.getPageSize(ctx)
+ var pageToken string
+
+ for {
+ functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize)
+ if err != nil {
+ yield(table.Identifier{}, err)
+
+ return
+ }
+ for _, function := range functions {
+ if !yield(function, nil) {
+ return
+ }
+ }
+ if nextPageToken == "" {
+ return
+ }
+ pageToken = nextPageToken
+ }
+ }
+}
+
+func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) {
+ if err := checkValidNamespace(namespace); err != nil {
+ return nil, "", err
+ }
+ // Unsupported listing yields an empty result rather than an error.
+ if !r.endpoints.allowed(endpointListFunctions) {
+ return nil, "", nil
+ }
+ ns := r.encodeNamespace(namespace)
+ path, err := endpointListFunctions.reqPath(ns)
+ if err != nil {
+ return nil, "", err
+ }
+ uri := r.baseURI.JoinPath(path...)
+
+ v := url.Values{}
+ if pageSize > 0 {
+ v.Set("pageSize", strconv.Itoa(pageSize))
+ }
+ if pageToken != "" {
+ v.Set("pageToken", pageToken)
+ }
+
+ uri.RawQuery = v.Encode()
+ // Function identifiers are CatalogObjectIdentifier values: ordered
+ // hierarchy levels (the namespace levels followed by the function
+ // name), not the {namespace, name} object tables and views use.
+ type resp struct {
+ Identifiers [][]string `json:"identifiers"`
+ NextPageToken string `json:"next-page-token,omitempty"`
+ }
+
+ rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace})
+ if err != nil {
+ return nil, "", err
+ }
+
+ out := make([]table.Identifier, len(rsp.Identifiers))
+ for i, levels := range rsp.Identifiers {
+ out[i] = table.Identifier(levels)
+ }
+
+ return out, rsp.NextPageToken, nil
+}
+
+// loadFunctionResponse contains the response from loading a function.
+type loadFunctionResponse struct {
+ MetadataLoc string `json:"metadata-location"`
+ RawMetadata json.RawMessage `json:"metadata"`
+}
+
+// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded
+// definitions are included in the single metadata response.
+func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) {
+ if err := r.endpoints.check(endpointLoadFunction); err != nil {
+ return nil, err
+ }
+
+ ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ if err != nil {
+ return nil, err
+ }
+
+ path, err := endpointLoadFunction.reqPath(ns, fn)
+ if err != nil {
+ return nil, err
+ }
+
+ rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path,
+ r.cl, map[int]error{
+ http.StatusNotFound: catalog.ErrNoSuchFunction,
Review Comment:
Fixed. `LoadFunction` now inspects the REST error type and maps `NoSuchNamespaceException` to `ErrNoSuchNamespace`, while `NoSuchFunctionException` remains `ErrNoSuchFunction`. Coverage was added for both `LoadFunction` and `CheckFunctionExists`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#issuecomment-4934631946
cc @laskoviymishka @tanmayrauth
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(udf): add SQL UDF REST catalog read support [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569418130
##########
catalog/rest/rest_functions_test.go:
##########
@@ -0,0 +1,428 @@
+// 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.
+
+package rest_test
+
+import (
+ "context"
+ "encoding/json"
+ "net/http"
+ "net/http/httptest"
+ "strconv"
+ "testing"
+
+ "github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/catalog/rest"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// testFunctionMetadataJSON is minimal valid UDF metadata for add_one(int).
+const testFunctionMetadataJSON = `{
+ "function-uuid": "42fd3f91-bc10-41c1-8a52-92b57dd0a9b2",
+ "format-version": 1,
+ "definitions": [
+ {
+ "definition-id": "int",
+ "parameters": [{"name": "x", "type": "int"}],
+ "return-type": "int",
+ "function-type": "udf",
+ "versions": [
+ {
+ "version-id": 1,
+ "representations": [{"type": "sql", "dialect": "trino", "sql": "x + 1"}],
+ "timestamp-ms": 1000
+ }
+ ],
+ "current-version-id": 1
+ }
+ ],
+ "definition-log": [
+ {"timestamp-ms": 1000, "definition-versions": [{"definition-id": "int", "version-id": 1}]}
+ ]
+}`
+
+func (r *RestCatalogSuite) TestListFunctions200() {
+ customPageSize := 100
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ pageToken := req.URL.Query().Get("pageToken")
+ pageSize := req.URL.Query().Get("pageSize")
+ r.Equal("", pageToken)
+ r.Equal(strconv.Itoa(customPageSize), pageSize)
+
+ // Function identifiers are CatalogObjectIdentifier values: flat
+ // arrays of hierarchy levels, not {namespace, name} objects.
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{
+ []string{"accounting", "tax", "paid"},
+ []string{"accounting", "tax", "owed"},
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+ // Passing in a custom page size through context
+ ctx := cat.SetPageSize(context.Background(), customPageSize)
+
+ var lastErr error
+ functions := make([]table.Identifier, 0)
+ for function, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) {
+ functions = append(functions, function)
+ if err != nil {
+ lastErr = err
+ r.FailNow("unexpected error:", err)
+ }
+ }
+
+ r.Equal([]table.Identifier{
+ {"accounting", "tax", "paid"},
+ {"accounting", "tax", "owed"},
+ }, functions)
+ r.Require().NoError(lastErr)
+}
+
+func (r *RestCatalogSuite) TestListFunctionsPagination() {
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ switch req.URL.Query().Get("pageToken") {
+ case "":
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{[]string{"accounting", "add_one"}},
+ "next-page-token": "token-1",
+ })
+ case "token-1":
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{[]string{"accounting", "add_two"}},
+ })
+ default:
+ r.FailNow("unexpected page token")
+ }
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ functions := make([]table.Identifier, 0)
+ for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ r.Require().NoError(err)
+ functions = append(functions, function)
+ }
+
+ r.Equal([]table.Identifier{
+ {"accounting", "add_one"},
+ {"accounting", "add_two"},
+ }, functions)
+
+ // Breaking out of the iteration stops paging without error.
+ for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ r.Require().NoError(err)
+ r.Equal(table.Identifier{"accounting", "add_one"}, function)
+
+ break
+ }
+}
+
+func (r *RestCatalogSuite) TestListFunctionsZeroPageSizeNotSent() {
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+ r.Equal("", req.URL.Query().Get("pageSize"), "pageSize must not be sent when set to 0")
+ json.NewEncoder(w).Encode(map[string]any{"identifiers": []any{}})
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ ctx := cat.SetPageSize(context.Background(), 0)
+ for _, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) {
+ r.Require().NoError(err)
+ }
+}
+
+func (r *RestCatalogSuite) TestListFunctionsPaginationErrorOnSubsequentPage() {
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ pageToken := req.URL.Query().Get("pageToken")
+
+ // First page succeeds
+ if pageToken == "" {
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{
+ []string{namespace, "paid1"},
+ []string{namespace, "paid2"},
+ },
+ "next-page-token": "token1",
+ })
+
+ return
+ }
+
+ // Second page fails with an error
+ if pageToken == "token1" {
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "Token expired or invalid",
+ "type": "NoSuchPageTokenException",
+ "code": 404,
+ },
+ })
+
+ return
+ }
+
+ r.FailNow("unexpected page token:", pageToken)
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ functions := make([]table.Identifier, 0)
+ var lastErr error
+ for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ if err != nil {
+ lastErr = err
+
+ break
+ }
+ functions = append(functions, function)
+ }
+
+ // Check that we got the functions from the first page
+ r.Equal([]table.Identifier{
+ {"accounting", "paid1"},
+ {"accounting", "paid2"},
+ }, functions)
+
+ // Check that we got the error from the second page
+ r.Error(lastErr)
+ r.ErrorContains(lastErr, "Token expired or invalid")
+}
+
+func (r *RestCatalogSuite) TestListFunctions404() {
+ namespace := "nonexistent"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "The given namespace does not exist",
+ "type": "NoSuchNamespaceException",
+ "code": 404,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ var lastErr error
+ for _, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ if err != nil {
+ lastErr = err
+ }
+ }
+ r.ErrorIs(lastErr, catalog.ErrNoSuchNamespace)
+ r.ErrorContains(lastErr, "The given namespace does not exist")
+}
+
+func (r *RestCatalogSuite) TestLoadFunction200() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/add_one", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ w.Write([]byte(`{
+ "metadata-location": "s3://bucket/functions/add_one/metadata/00000-x.metadata.json",
+ "metadata": ` + testFunctionMetadataJSON + `
+ }`))
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ fn, err := cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "add_one"))
+ r.Require().NoError(err)
+
+ r.Equal(table.Identifier{"accounting", "add_one"}, fn.Identifier())
+ r.Equal("s3://bucket/functions/add_one/metadata/00000-x.metadata.json", fn.MetadataLocation())
+ r.Equal("42fd3f91-bc10-41c1-8a52-92b57dd0a9b2", fn.Metadata().FunctionUUID().String())
+
+ def, ok := fn.Metadata().DefinitionByID("int")
+ r.Require().True(ok, "expected the int overload in the loaded metadata")
+ r.Equal(1, def.CurrentVersionID)
+ r.Len(fn.Definitions(), 1)
+}
+
+func (r *RestCatalogSuite) TestLoadFunction404() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/missing_fn", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "The requested function does not exist",
+ "type": "NoSuchFunctionException",
+ "code": 404,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ _, err = cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "missing_fn"))
+ r.ErrorIs(err, catalog.ErrNoSuchFunction)
+ r.ErrorContains(err, "The requested function does not exist")
+}
+
+func (r *RestCatalogSuite) TestLoadFunctionMalformedMetadata() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/broken_fn", func(w http.ResponseWriter, req *http.Request) {
+ w.Write([]byte(`{"metadata-location": "s3://x", "metadata": {"format-version": 1}}`))
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ _, err = cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "broken_fn"))
+ r.ErrorContains(err, "failed to parse function metadata")
+}
+
+func (r *RestCatalogSuite) TestCheckFunctionExists() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/add_one", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method, "existence must be checked via GET: the spec has no HEAD endpoint")
+
+ w.Write([]byte(`{"metadata": ` + testFunctionMetadataJSON + `}`))
+ })
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/missing_fn", func(w http.ResponseWriter, req *http.Request) {
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "The requested function does not exist",
+ "type": "NoSuchFunctionException",
+ "code": 404,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ exists, err := cat.CheckFunctionExists(context.Background(), catalog.ToIdentifier("accounting", "add_one"))
+ r.Require().NoError(err)
+ r.True(exists)
+
+ exists, err = cat.CheckFunctionExists(context.Background(), catalog.ToIdentifier("accounting", "missing_fn"))
+ r.Require().NoError(err)
+ r.False(exists)
+}
+
+func (r *RestCatalogSuite) TestLoadFunctionInvalidIdentifier() {
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ // An invalid identifier reports the function sentinel, not the table one.
+ _, err = cat.LoadFunction(context.Background(), table.Identifier{})
+ r.ErrorIs(err, catalog.ErrNoSuchFunction)
+ r.NotErrorIs(err, catalog.ErrNoSuchTable)
+ r.ErrorContains(err, "missing namespace or invalid identifier")
+
+ // The existence check surfaces the invalid identifier as an error, as the
+ // table and view existence checks do.
+ exists, err := cat.CheckFunctionExists(context.Background(), table.Identifier{})
+ r.False(exists)
+ r.ErrorIs(err, catalog.ErrNoSuchFunction)
+}
+
+func (r *RestCatalogSuite) TestListFunctionsInvalidNamespace() {
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ var lastErr error
+ for _, err := range cat.ListFunctions(context.Background(), table.Identifier{}) {
+ if err != nil {
+ lastErr = err
+ }
+ }
+ r.Error(lastErr)
Review Comment:
Fixed, the invalid-namespace test now pins `ErrNoSuchNamespace`, and the affected iterator loops stop after receiving the first error.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569508132
##########
catalog/rest/rest.go:
##########
@@ -1038,6 +1039,14 @@ func (r *Catalog) splitViewIdentForPath(ident table.Identifier) (string, string,
return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.TableNameFromIdent(ident), nil
}
+func (r *Catalog) splitFunctionIdentForPath(ident table.Identifier) (string, string, error) {
+ if err := catalog.ValidateFunctionIdentifier(ident); err != nil {
+ return "", "", err
+ }
+
+ return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.TableNameFromIdent(ident), nil
Review Comment:
Addressed, I added `catalog.ObjectNameFromIdent`, retained `TableNameFromIdent` as a compatibility alias and updated the table, view and function REST path splitters to use the type-neutral helper.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569398286
##########
catalog/rest/rest.go:
##########
@@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v
return view.New(identifier, metadata, rsp.MetadataLoc), nil
}
+
+// Catalog implements the optional function (SQL UDF) read support defined
+// by the REST spec: list and load. The function endpoints are not part of
+// the spec's assumed default endpoint set, so they are only used when the
+// server advertises them.
+var _ catalog.FunctionCatalog = (*Catalog)(nil)
+
+// ListFunctions returns the function (SQL UDF) identifiers under a
+// namespace, with the returned identifiers containing the information
+// required to load the function via this catalog.
+func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ pageSize := r.getPageSize(ctx)
+ var pageToken string
+
+ for {
+ functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize)
+ if err != nil {
+ yield(table.Identifier{}, err)
+
+ return
+ }
+ for _, function := range functions {
+ if !yield(function, nil) {
+ return
+ }
+ }
+ if nextPageToken == "" {
+ return
+ }
+ pageToken = nextPageToken
+ }
+ }
+}
+
+func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) {
+ if err := checkValidNamespace(namespace); err != nil {
+ return nil, "", err
+ }
+ // Unsupported listing yields an empty result rather than an error.
+ if !r.endpoints.allowed(endpointListFunctions) {
+ return nil, "", nil
+ }
+ ns := r.encodeNamespace(namespace)
+ path, err := endpointListFunctions.reqPath(ns)
+ if err != nil {
+ return nil, "", err
+ }
+ uri := r.baseURI.JoinPath(path...)
+
+ v := url.Values{}
+ if pageSize > 0 {
+ v.Set("pageSize", strconv.Itoa(pageSize))
+ }
+ if pageToken != "" {
+ v.Set("pageToken", pageToken)
+ }
+
+ uri.RawQuery = v.Encode()
+ // Function identifiers are CatalogObjectIdentifier values: ordered
+ // hierarchy levels (the namespace levels followed by the function
+ // name), not the {namespace, name} object tables and views use.
+ type resp struct {
+ Identifiers [][]string `json:"identifiers"`
+ NextPageToken string `json:"next-page-token,omitempty"`
+ }
+
+ rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace})
+ if err != nil {
+ return nil, "", err
+ }
+
+ out := make([]table.Identifier, len(rsp.Identifiers))
+ for i, levels := range rsp.Identifiers {
+ out[i] = table.Identifier(levels)
+ }
+
+ return out, rsp.NextPageToken, nil
+}
+
+// loadFunctionResponse contains the response from loading a function.
+type loadFunctionResponse struct {
+ MetadataLoc string `json:"metadata-location"`
+ RawMetadata json.RawMessage `json:"metadata"`
+}
+
+// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded
+// definitions are included in the single metadata response.
+func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) {
+ if err := r.endpoints.check(endpointLoadFunction); err != nil {
+ return nil, err
+ }
+
+ ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ if err != nil {
+ return nil, err
+ }
+
+ path, err := endpointLoadFunction.reqPath(ns, fn)
+ if err != nil {
+ return nil, err
+ }
+
+ rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path,
+ r.cl, map[int]error{
+ http.StatusNotFound: catalog.ErrNoSuchFunction,
+ })
+ if err != nil {
+ return nil, err
+ }
+
+ metadata, err := udf.ParseMetadataBytes(rsp.RawMetadata)
+ if err != nil {
+ return nil, fmt.Errorf("failed to parse function metadata: %w", err)
+ }
+
+ return udf.New(identifier, metadata, rsp.MetadataLoc), nil
+}
+
+// CheckFunctionExists returns if the function exists. The REST spec defines
+// no HEAD endpoint for functions, so existence is checked by loading the
+// function; if loading is unsupported, its ErrEndpointNotSupported surfaces
+// rather than a bogus "not found".
+func (r *Catalog) CheckFunctionExists(ctx context.Context, identifier table.Identifier) (bool, error) {
+ // Validate first, as the table and view existence checks do: an invalid
+ // identifier surfaces as an error, so ErrNoSuchFunction below can only
+ // mean a server-reported "not found".
+ if err := catalog.ValidateFunctionIdentifier(identifier); err != nil {
Review Comment:
Fixed. `LoadFunction` and `CheckFunctionExists` now validate the identifier once and share a private post-validation loader. This removes the duplicate validation while preserving the existing behavior that malformed identifiers return an error and only a server-reported missing function maps to `(false, nil)`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#issuecomment-4958813370
@laskoviymishka Addressed, PTAL.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3559026652
##########
catalog/rest/rest_functions_test.go:
##########
@@ -0,0 +1,428 @@
+// 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.
+
+package rest_test
+
+import (
+ "context"
+ "encoding/json"
+ "net/http"
+ "net/http/httptest"
+ "strconv"
+ "testing"
+
+ "github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/catalog/rest"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// testFunctionMetadataJSON is minimal valid UDF metadata for add_one(int).
+const testFunctionMetadataJSON = `{
+ "function-uuid": "42fd3f91-bc10-41c1-8a52-92b57dd0a9b2",
+ "format-version": 1,
+ "definitions": [
+ {
+ "definition-id": "int",
+ "parameters": [{"name": "x", "type": "int"}],
+ "return-type": "int",
+ "function-type": "udf",
+ "versions": [
+ {
+ "version-id": 1,
+ "representations": [{"type": "sql", "dialect": "trino", "sql": "x + 1"}],
+ "timestamp-ms": 1000
+ }
+ ],
+ "current-version-id": 1
+ }
+ ],
+ "definition-log": [
+ {"timestamp-ms": 1000, "definition-versions": [{"definition-id": "int", "version-id": 1}]}
+ ]
+}`
+
+func (r *RestCatalogSuite) TestListFunctions200() {
+ customPageSize := 100
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ pageToken := req.URL.Query().Get("pageToken")
+ pageSize := req.URL.Query().Get("pageSize")
+ r.Equal("", pageToken)
+ r.Equal(strconv.Itoa(customPageSize), pageSize)
+
+ // Function identifiers are CatalogObjectIdentifier values: flat
+ // arrays of hierarchy levels, not {namespace, name} objects.
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{
+ []string{"accounting", "tax", "paid"},
+ []string{"accounting", "tax", "owed"},
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+ // Passing in a custom page size through context
+ ctx := cat.SetPageSize(context.Background(), customPageSize)
+
+ var lastErr error
+ functions := make([]table.Identifier, 0)
+ for function, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) {
+ functions = append(functions, function)
Review Comment:
This appends `function` before checking `err`. If the iterator ever yields `(empty-ident, err)`, the empty identifier gets appended before the error is caught — the assertion on the collected slice would then be checking corrupted data.
`TestListFunctionsPaginationErrorOnSubsequentPage` gets this right (checks `err`, breaks, then appends). I'd invert this loop to match — check `err` first, append on success.
##########
catalog/catalog.go:
##########
@@ -39,6 +39,7 @@ import (
"github.com/apache/iceberg-go"
iceinternal "github.com/apache/iceberg-go/internal"
"github.com/apache/iceberg-go/table"
+ "github.com/apache/iceberg-go/udf"
Review Comment:
Putting `FunctionCatalog` in the base `catalog` package means `catalog` now imports `udf`, and since `LoadFunction` returns `*udf.UDF`, every consumer of the base package transitively depends on `udf`.
That's also asymmetric with views: there's no `ViewCatalog` interface here — `LoadView`/`ListViews`/`CheckViewExists` are just methods on `*rest.Catalog`. I'd follow that precedent and keep `FunctionCatalog` local to `catalog/rest` (the `var _ catalog.FunctionCatalog` assertion in rest.go goes away with it). If we genuinely need a cross-package interface, I'd at least avoid the concrete `*udf.UDF` return type so the base package stays leaf-independent. wdyt?
##########
catalog/rest/rest.go:
##########
@@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v
return view.New(identifier, metadata, rsp.MetadataLoc), nil
}
+
+// Catalog implements the optional function (SQL UDF) read support defined
+// by the REST spec: list and load. The function endpoints are not part of
+// the spec's assumed default endpoint set, so they are only used when the
+// server advertises them.
+var _ catalog.FunctionCatalog = (*Catalog)(nil)
+
+// ListFunctions returns the function (SQL UDF) identifiers under a
+// namespace, with the returned identifiers containing the information
+// required to load the function via this catalog.
+func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ pageSize := r.getPageSize(ctx)
+ var pageToken string
+
+ for {
+ functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize)
+ if err != nil {
+ yield(table.Identifier{}, err)
+
+ return
+ }
+ for _, function := range functions {
+ if !yield(function, nil) {
+ return
+ }
+ }
+ if nextPageToken == "" {
+ return
+ }
+ pageToken = nextPageToken
+ }
+ }
+}
+
+func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) {
+ if err := checkValidNamespace(namespace); err != nil {
+ return nil, "", err
+ }
+ // Unsupported listing yields an empty result rather than an error.
+ if !r.endpoints.allowed(endpointListFunctions) {
+ return nil, "", nil
+ }
+ ns := r.encodeNamespace(namespace)
+ path, err := endpointListFunctions.reqPath(ns)
+ if err != nil {
+ return nil, "", err
+ }
+ uri := r.baseURI.JoinPath(path...)
+
+ v := url.Values{}
+ if pageSize > 0 {
+ v.Set("pageSize", strconv.Itoa(pageSize))
+ }
+ if pageToken != "" {
+ v.Set("pageToken", pageToken)
+ }
+
+ uri.RawQuery = v.Encode()
+ // Function identifiers are CatalogObjectIdentifier values: ordered
+ // hierarchy levels (the namespace levels followed by the function
+ // name), not the {namespace, name} object tables and views use.
+ type resp struct {
+ Identifiers [][]string `json:"identifiers"`
+ NextPageToken string `json:"next-page-token,omitempty"`
+ }
+
+ rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace})
+ if err != nil {
+ return nil, "", err
+ }
+
+ out := make([]table.Identifier, len(rsp.Identifiers))
+ for i, levels := range rsp.Identifiers {
+ out[i] = table.Identifier(levels)
+ }
+
+ return out, rsp.NextPageToken, nil
+}
+
+// loadFunctionResponse contains the response from loading a function.
+type loadFunctionResponse struct {
+ MetadataLoc string `json:"metadata-location"`
+ RawMetadata json.RawMessage `json:"metadata"`
+}
+
+// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded
+// definitions are included in the single metadata response.
+func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) {
+ if err := r.endpoints.check(endpointLoadFunction); err != nil {
+ return nil, err
+ }
+
+ ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ if err != nil {
+ return nil, err
+ }
+
+ path, err := endpointLoadFunction.reqPath(ns, fn)
+ if err != nil {
+ return nil, err
+ }
+
+ rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path,
+ r.cl, map[int]error{
+ http.StatusNotFound: catalog.ErrNoSuchFunction,
+ })
+ if err != nil {
+ return nil, err
+ }
+
+ metadata, err := udf.ParseMetadataBytes(rsp.RawMetadata)
Review Comment:
If a server returns `metadata-location` with no `metadata` key, `RawMetadata` is nil and this surfaces as `"unexpected end of JSON input"`, which is confusing. The spec marks metadata required so only a buggy server hits it, but a `len(rsp.RawMetadata) == 0` guard with a clearer message would be a cheap robustness win. Non-blocking.
##########
catalog/rest/rest.go:
##########
@@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v
return view.New(identifier, metadata, rsp.MetadataLoc), nil
}
+
+// Catalog implements the optional function (SQL UDF) read support defined
+// by the REST spec: list and load. The function endpoints are not part of
+// the spec's assumed default endpoint set, so they are only used when the
+// server advertises them.
+var _ catalog.FunctionCatalog = (*Catalog)(nil)
+
+// ListFunctions returns the function (SQL UDF) identifiers under a
+// namespace, with the returned identifiers containing the information
+// required to load the function via this catalog.
+func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ pageSize := r.getPageSize(ctx)
+ var pageToken string
+
+ for {
+ functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize)
+ if err != nil {
+ yield(table.Identifier{}, err)
+
+ return
+ }
+ for _, function := range functions {
+ if !yield(function, nil) {
+ return
+ }
+ }
+ if nextPageToken == "" {
+ return
+ }
+ pageToken = nextPageToken
+ }
+ }
+}
+
+func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) {
+ if err := checkValidNamespace(namespace); err != nil {
+ return nil, "", err
+ }
+ // Unsupported listing yields an empty result rather than an error.
+ if !r.endpoints.allowed(endpointListFunctions) {
+ return nil, "", nil
+ }
+ ns := r.encodeNamespace(namespace)
+ path, err := endpointListFunctions.reqPath(ns)
+ if err != nil {
+ return nil, "", err
+ }
+ uri := r.baseURI.JoinPath(path...)
+
+ v := url.Values{}
+ if pageSize > 0 {
+ v.Set("pageSize", strconv.Itoa(pageSize))
+ }
+ if pageToken != "" {
+ v.Set("pageToken", pageToken)
+ }
+
+ uri.RawQuery = v.Encode()
+ // Function identifiers are CatalogObjectIdentifier values: ordered
+ // hierarchy levels (the namespace levels followed by the function
+ // name), not the {namespace, name} object tables and views use.
+ type resp struct {
+ Identifiers [][]string `json:"identifiers"`
+ NextPageToken string `json:"next-page-token,omitempty"`
+ }
+
+ rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace})
+ if err != nil {
+ return nil, "", err
+ }
+
+ out := make([]table.Identifier, len(rsp.Identifiers))
+ for i, levels := range rsp.Identifiers {
+ out[i] = table.Identifier(levels)
+ }
+
+ return out, rsp.NextPageToken, nil
+}
+
+// loadFunctionResponse contains the response from loading a function.
+type loadFunctionResponse struct {
+ MetadataLoc string `json:"metadata-location"`
+ RawMetadata json.RawMessage `json:"metadata"`
+}
+
+// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded
+// definitions are included in the single metadata response.
+func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) {
+ if err := r.endpoints.check(endpointLoadFunction); err != nil {
+ return nil, err
+ }
+
+ ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ if err != nil {
+ return nil, err
+ }
+
+ path, err := endpointLoadFunction.reqPath(ns, fn)
+ if err != nil {
+ return nil, err
+ }
+
+ rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path,
+ r.cl, map[int]error{
+ http.StatusNotFound: catalog.ErrNoSuchFunction,
+ })
+ if err != nil {
+ return nil, err
+ }
+
+ metadata, err := udf.ParseMetadataBytes(rsp.RawMetadata)
+ if err != nil {
+ return nil, fmt.Errorf("failed to parse function metadata: %w", err)
+ }
+
+ return udf.New(identifier, metadata, rsp.MetadataLoc), nil
+}
+
+// CheckFunctionExists returns if the function exists. The REST spec defines
+// no HEAD endpoint for functions, so existence is checked by loading the
+// function; if loading is unsupported, its ErrEndpointNotSupported surfaces
+// rather than a bogus "not found".
+func (r *Catalog) CheckFunctionExists(ctx context.Context, identifier table.Identifier) (bool, error) {
+ // Validate first, as the table and view existence checks do: an invalid
+ // identifier surfaces as an error, so ErrNoSuchFunction below can only
+ // mean a server-reported "not found".
+ if err := catalog.ValidateFunctionIdentifier(identifier); err != nil {
Review Comment:
This validation is redundant — `LoadFunction` immediately calls `splitFunctionIdentForPath`, which calls `ValidateFunctionIdentifier` again, so the identifier is validated twice.
The comment is also inaccurate: `CheckTableExists`/`CheckViewExists` don't do an explicit up-front validate, they call `splitIdentForPath`/`splitViewIdentForPath` directly (one pass) and then HEAD/Load. I'd drop the explicit call and the comment here — `TestLoadFunctionInvalidIdentifier` still passes because `LoadFunction` validates first.
##########
catalog/catalog.go:
##########
@@ -192,6 +194,30 @@ type PurgeableTable interface {
PurgeTable(ctx context.Context, identifier table.Identifier) error
}
+// FunctionCatalog is an optional interface implemented by catalogs that
+// support the Iceberg REST function (SQL UDF) endpoints. The function
+// endpoints are not part of the spec's assumed default endpoint set, so
+// servers must advertise them: unsupported loads report an
+// endpoint-not-supported error and unsupported listings yield no results.
Review Comment:
This doc promises "unsupported loads report an endpoint-not-supported error," but `ErrEndpointNotSupported` lives in `catalog/rest`, not `catalog`. A caller using the documented type-assertion pattern imports only `catalog` and can't `errors.Is` against a sentinel it can't name.
Either move `ErrEndpointNotSupported` up to `catalog`, or drop the endpoint-negotiation language from this doc and describe it only in the rest.go methods. This one resolves naturally if we go with the "keep it in `catalog/rest`" direction from the layering comment above.
##########
catalog/rest/rest.go:
##########
@@ -2045,3 +2054,145 @@ func (r *Catalog) LoadView(ctx context.Context, identifier table.Identifier) (*v
return view.New(identifier, metadata, rsp.MetadataLoc), nil
}
+
+// Catalog implements the optional function (SQL UDF) read support defined
+// by the REST spec: list and load. The function endpoints are not part of
+// the spec's assumed default endpoint set, so they are only used when the
+// server advertises them.
+var _ catalog.FunctionCatalog = (*Catalog)(nil)
+
+// ListFunctions returns the function (SQL UDF) identifiers under a
+// namespace, with the returned identifiers containing the information
+// required to load the function via this catalog.
+func (r *Catalog) ListFunctions(ctx context.Context, namespace table.Identifier) iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ pageSize := r.getPageSize(ctx)
+ var pageToken string
+
+ for {
+ functions, nextPageToken, err := r.listFunctionsPage(ctx, namespace, pageToken, pageSize)
+ if err != nil {
+ yield(table.Identifier{}, err)
+
+ return
+ }
+ for _, function := range functions {
+ if !yield(function, nil) {
+ return
+ }
+ }
+ if nextPageToken == "" {
+ return
+ }
+ pageToken = nextPageToken
+ }
+ }
+}
+
+func (r *Catalog) listFunctionsPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) {
+ if err := checkValidNamespace(namespace); err != nil {
+ return nil, "", err
+ }
+ // Unsupported listing yields an empty result rather than an error.
+ if !r.endpoints.allowed(endpointListFunctions) {
+ return nil, "", nil
+ }
+ ns := r.encodeNamespace(namespace)
+ path, err := endpointListFunctions.reqPath(ns)
+ if err != nil {
+ return nil, "", err
+ }
+ uri := r.baseURI.JoinPath(path...)
+
+ v := url.Values{}
+ if pageSize > 0 {
+ v.Set("pageSize", strconv.Itoa(pageSize))
+ }
+ if pageToken != "" {
+ v.Set("pageToken", pageToken)
+ }
+
+ uri.RawQuery = v.Encode()
+ // Function identifiers are CatalogObjectIdentifier values: ordered
+ // hierarchy levels (the namespace levels followed by the function
+ // name), not the {namespace, name} object tables and views use.
+ type resp struct {
+ Identifiers [][]string `json:"identifiers"`
+ NextPageToken string `json:"next-page-token,omitempty"`
+ }
+
+ rsp, err := doGet[resp](ctx, uri, []string{}, r.cl, map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace})
+ if err != nil {
+ return nil, "", err
+ }
+
+ out := make([]table.Identifier, len(rsp.Identifiers))
+ for i, levels := range rsp.Identifiers {
+ out[i] = table.Identifier(levels)
+ }
+
+ return out, rsp.NextPageToken, nil
+}
+
+// loadFunctionResponse contains the response from loading a function.
+type loadFunctionResponse struct {
+ MetadataLoc string `json:"metadata-location"`
+ RawMetadata json.RawMessage `json:"metadata"`
+}
+
+// LoadFunction loads a function (SQL UDF) from the catalog. All overloaded
+// definitions are included in the single metadata response.
+func (r *Catalog) LoadFunction(ctx context.Context, identifier table.Identifier) (*udf.UDF, error) {
+ if err := r.endpoints.check(endpointLoadFunction); err != nil {
+ return nil, err
+ }
+
+ ns, fn, err := r.splitFunctionIdentForPath(identifier)
+ if err != nil {
+ return nil, err
+ }
+
+ path, err := endpointLoadFunction.reqPath(ns, fn)
+ if err != nil {
+ return nil, err
+ }
+
+ rsp, err := doGet[loadFunctionResponse](ctx, r.baseURI, path,
+ r.cl, map[int]error{
+ http.StatusNotFound: catalog.ErrNoSuchFunction,
Review Comment:
This maps every 404 to `ErrNoSuchFunction`, but the spec's GET load defines two 404 bodies — `NoSuchNamespaceException` and `NoSuchFunctionException`. The status-code-only override ignores `error.type`, so `LoadFunction` on a deleted namespace returns `ErrNoSuchFunction`, and `CheckFunctionExists` then reports `(false, nil)` for a missing namespace.
Java's view/table handlers discriminate on the type string before falling through. This is a pre-existing gap for `LoadView`/`LoadTable` too, so not a blocker for this PR — but worth a look as a consistent follow-up, since the function path is a fresh chance to get it right.
##########
udf/udf.go:
##########
@@ -0,0 +1,58 @@
+// 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.
+
+package udf
+
+import (
+ "slices"
+
+ "github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/table"
+)
+
+// UDF is a function (SQL UDF) loaded from a catalog: its catalog identifier,
+// its metadata, and the location the metadata was loaded from.
+type UDF struct {
+ identifier table.Identifier
+ metadata Metadata
+ metadataLocation string
+}
+
+// New creates a UDF from its catalog identifier, metadata and metadata
+// location.
+func New(ident table.Identifier, meta Metadata, metadataLocation string) *UDF {
+ return &UDF{
+ identifier: ident,
+ metadata: meta,
+ metadataLocation: metadataLocation,
+ }
+}
+
+// Equals reports whether two UDFs have the same identifier, metadata
+// location and metadata.
+func (u UDF) Equals(other UDF) bool {
Review Comment:
`New` returns `*UDF` but `Equals` takes a value, so callers end up writing `fn.Equals(*same)` (visible in the test). Minor friction — I'd either use pointer receivers/argument to match the constructor, or drop a one-line doc noting the value-copy semantics are intentional.
##########
catalog/rest/rest.go:
##########
@@ -1038,6 +1039,14 @@ func (r *Catalog) splitViewIdentForPath(ident table.Identifier) (string, string,
return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.TableNameFromIdent(ident), nil
}
+func (r *Catalog) splitFunctionIdentForPath(ident table.Identifier) (string, string, error) {
+ if err := catalog.ValidateFunctionIdentifier(ident); err != nil {
+ return "", "", err
+ }
+
+ return r.encodeNamespace(catalog.NamespaceFromIdent(ident)), catalog.TableNameFromIdent(ident), nil
Review Comment:
Using `catalog.TableNameFromIdent` to pull the function name works (it's the last element) but reads oddly — `LoadView` has the same wart. Not for this PR, but a type-agnostic `catalog.ObjectNameFromIdent` would clean up all three call sites.
##########
catalog/rest/rest_functions_test.go:
##########
@@ -0,0 +1,428 @@
+// 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.
+
+package rest_test
+
+import (
+ "context"
+ "encoding/json"
+ "net/http"
+ "net/http/httptest"
+ "strconv"
+ "testing"
+
+ "github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/catalog/rest"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// testFunctionMetadataJSON is minimal valid UDF metadata for add_one(int).
+const testFunctionMetadataJSON = `{
+ "function-uuid": "42fd3f91-bc10-41c1-8a52-92b57dd0a9b2",
+ "format-version": 1,
+ "definitions": [
+ {
+ "definition-id": "int",
+ "parameters": [{"name": "x", "type": "int"}],
+ "return-type": "int",
+ "function-type": "udf",
+ "versions": [
+ {
+ "version-id": 1,
+ "representations": [{"type": "sql", "dialect": "trino", "sql": "x + 1"}],
+ "timestamp-ms": 1000
+ }
+ ],
+ "current-version-id": 1
+ }
+ ],
+ "definition-log": [
+ {"timestamp-ms": 1000, "definition-versions": [{"definition-id": "int", "version-id": 1}]}
+ ]
+}`
+
+func (r *RestCatalogSuite) TestListFunctions200() {
+ customPageSize := 100
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ pageToken := req.URL.Query().Get("pageToken")
+ pageSize := req.URL.Query().Get("pageSize")
+ r.Equal("", pageToken)
+ r.Equal(strconv.Itoa(customPageSize), pageSize)
+
+ // Function identifiers are CatalogObjectIdentifier values: flat
+ // arrays of hierarchy levels, not {namespace, name} objects.
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{
+ []string{"accounting", "tax", "paid"},
+ []string{"accounting", "tax", "owed"},
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+ // Passing in a custom page size through context
+ ctx := cat.SetPageSize(context.Background(), customPageSize)
+
+ var lastErr error
+ functions := make([]table.Identifier, 0)
+ for function, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) {
+ functions = append(functions, function)
+ if err != nil {
+ lastErr = err
+ r.FailNow("unexpected error:", err)
+ }
+ }
+
+ r.Equal([]table.Identifier{
+ {"accounting", "tax", "paid"},
+ {"accounting", "tax", "owed"},
+ }, functions)
+ r.Require().NoError(lastErr)
+}
+
+func (r *RestCatalogSuite) TestListFunctionsPagination() {
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ switch req.URL.Query().Get("pageToken") {
+ case "":
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{[]string{"accounting", "add_one"}},
+ "next-page-token": "token-1",
+ })
+ case "token-1":
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{[]string{"accounting", "add_two"}},
+ })
+ default:
+ r.FailNow("unexpected page token")
+ }
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ functions := make([]table.Identifier, 0)
+ for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ r.Require().NoError(err)
+ functions = append(functions, function)
+ }
+
+ r.Equal([]table.Identifier{
+ {"accounting", "add_one"},
+ {"accounting", "add_two"},
+ }, functions)
+
+ // Breaking out of the iteration stops paging without error.
+ for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ r.Require().NoError(err)
+ r.Equal(table.Identifier{"accounting", "add_one"}, function)
+
+ break
+ }
+}
+
+func (r *RestCatalogSuite) TestListFunctionsZeroPageSizeNotSent() {
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+ r.Equal("", req.URL.Query().Get("pageSize"), "pageSize must not be sent when set to 0")
+ json.NewEncoder(w).Encode(map[string]any{"identifiers": []any{}})
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ ctx := cat.SetPageSize(context.Background(), 0)
+ for _, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) {
+ r.Require().NoError(err)
+ }
+}
+
+func (r *RestCatalogSuite) TestListFunctionsPaginationErrorOnSubsequentPage() {
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ pageToken := req.URL.Query().Get("pageToken")
+
+ // First page succeeds
+ if pageToken == "" {
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{
+ []string{namespace, "paid1"},
+ []string{namespace, "paid2"},
+ },
+ "next-page-token": "token1",
+ })
+
+ return
+ }
+
+ // Second page fails with an error
+ if pageToken == "token1" {
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "Token expired or invalid",
+ "type": "NoSuchPageTokenException",
+ "code": 404,
+ },
+ })
+
+ return
+ }
+
+ r.FailNow("unexpected page token:", pageToken)
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ functions := make([]table.Identifier, 0)
+ var lastErr error
+ for function, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ if err != nil {
+ lastErr = err
+
+ break
+ }
+ functions = append(functions, function)
+ }
+
+ // Check that we got the functions from the first page
+ r.Equal([]table.Identifier{
+ {"accounting", "paid1"},
+ {"accounting", "paid2"},
+ }, functions)
+
+ // Check that we got the error from the second page
+ r.Error(lastErr)
+ r.ErrorContains(lastErr, "Token expired or invalid")
+}
+
+func (r *RestCatalogSuite) TestListFunctions404() {
+ namespace := "nonexistent"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "The given namespace does not exist",
+ "type": "NoSuchNamespaceException",
+ "code": 404,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ var lastErr error
+ for _, err := range cat.ListFunctions(context.Background(), catalog.ToIdentifier(namespace)) {
+ if err != nil {
+ lastErr = err
+ }
+ }
+ r.ErrorIs(lastErr, catalog.ErrNoSuchNamespace)
+ r.ErrorContains(lastErr, "The given namespace does not exist")
+}
+
+func (r *RestCatalogSuite) TestLoadFunction200() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/add_one", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ w.Write([]byte(`{
+ "metadata-location": "s3://bucket/functions/add_one/metadata/00000-x.metadata.json",
+ "metadata": ` + testFunctionMetadataJSON + `
+ }`))
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ fn, err := cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "add_one"))
+ r.Require().NoError(err)
+
+ r.Equal(table.Identifier{"accounting", "add_one"}, fn.Identifier())
+ r.Equal("s3://bucket/functions/add_one/metadata/00000-x.metadata.json", fn.MetadataLocation())
+ r.Equal("42fd3f91-bc10-41c1-8a52-92b57dd0a9b2", fn.Metadata().FunctionUUID().String())
+
+ def, ok := fn.Metadata().DefinitionByID("int")
+ r.Require().True(ok, "expected the int overload in the loaded metadata")
+ r.Equal(1, def.CurrentVersionID)
+ r.Len(fn.Definitions(), 1)
+}
+
+func (r *RestCatalogSuite) TestLoadFunction404() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/missing_fn", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "The requested function does not exist",
+ "type": "NoSuchFunctionException",
+ "code": 404,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ _, err = cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "missing_fn"))
+ r.ErrorIs(err, catalog.ErrNoSuchFunction)
+ r.ErrorContains(err, "The requested function does not exist")
+}
+
+func (r *RestCatalogSuite) TestLoadFunctionMalformedMetadata() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/broken_fn", func(w http.ResponseWriter, req *http.Request) {
+ w.Write([]byte(`{"metadata-location": "s3://x", "metadata": {"format-version": 1}}`))
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ _, err = cat.LoadFunction(context.Background(), catalog.ToIdentifier("accounting", "broken_fn"))
+ r.ErrorContains(err, "failed to parse function metadata")
+}
+
+func (r *RestCatalogSuite) TestCheckFunctionExists() {
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/add_one", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method, "existence must be checked via GET: the spec has no HEAD endpoint")
+
+ w.Write([]byte(`{"metadata": ` + testFunctionMetadataJSON + `}`))
+ })
+ r.mux.HandleFunc("/v1/namespaces/accounting/functions/missing_fn", func(w http.ResponseWriter, req *http.Request) {
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "The requested function does not exist",
+ "type": "NoSuchFunctionException",
+ "code": 404,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ exists, err := cat.CheckFunctionExists(context.Background(), catalog.ToIdentifier("accounting", "add_one"))
+ r.Require().NoError(err)
+ r.True(exists)
+
+ exists, err = cat.CheckFunctionExists(context.Background(), catalog.ToIdentifier("accounting", "missing_fn"))
+ r.Require().NoError(err)
+ r.False(exists)
+}
+
+func (r *RestCatalogSuite) TestLoadFunctionInvalidIdentifier() {
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ // An invalid identifier reports the function sentinel, not the table one.
+ _, err = cat.LoadFunction(context.Background(), table.Identifier{})
+ r.ErrorIs(err, catalog.ErrNoSuchFunction)
+ r.NotErrorIs(err, catalog.ErrNoSuchTable)
+ r.ErrorContains(err, "missing namespace or invalid identifier")
+
+ // The existence check surfaces the invalid identifier as an error, as the
+ // table and view existence checks do.
+ exists, err := cat.CheckFunctionExists(context.Background(), table.Identifier{})
+ r.False(exists)
+ r.ErrorIs(err, catalog.ErrNoSuchFunction)
+}
+
+func (r *RestCatalogSuite) TestListFunctionsInvalidNamespace() {
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ var lastErr error
+ for _, err := range cat.ListFunctions(context.Background(), table.Identifier{}) {
+ if err != nil {
+ lastErr = err
+ }
+ }
+ r.Error(lastErr)
Review Comment:
`r.Error(lastErr)` passes for any error, including a wrong sentinel — compare `TestListTables404`, which pins `ErrorIs(ErrNoSuchNamespace)`. I'd add an `ErrorIs` here to nail down which error we expect.
Same file has a couple of loops (this one, `TestListFunctions404`) that keep iterating after capturing the error instead of breaking — harmless since the iterator stops after an error, but it reads as if multiple errors could arrive. I'd break after the first for clarity while we're here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569400880
##########
catalog/rest/rest_functions_test.go:
##########
@@ -0,0 +1,428 @@
+// 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.
+
+package rest_test
+
+import (
+ "context"
+ "encoding/json"
+ "net/http"
+ "net/http/httptest"
+ "strconv"
+ "testing"
+
+ "github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/catalog/rest"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// testFunctionMetadataJSON is minimal valid UDF metadata for add_one(int).
+const testFunctionMetadataJSON = `{
+ "function-uuid": "42fd3f91-bc10-41c1-8a52-92b57dd0a9b2",
+ "format-version": 1,
+ "definitions": [
+ {
+ "definition-id": "int",
+ "parameters": [{"name": "x", "type": "int"}],
+ "return-type": "int",
+ "function-type": "udf",
+ "versions": [
+ {
+ "version-id": 1,
+ "representations": [{"type": "sql", "dialect": "trino", "sql": "x + 1"}],
+ "timestamp-ms": 1000
+ }
+ ],
+ "current-version-id": 1
+ }
+ ],
+ "definition-log": [
+ {"timestamp-ms": 1000, "definition-versions": [{"definition-id": "int", "version-id": 1}]}
+ ]
+}`
+
+func (r *RestCatalogSuite) TestListFunctions200() {
+ customPageSize := 100
+ namespace := "accounting"
+ r.mux.HandleFunc("/v1/namespaces/"+namespace+"/functions", func(w http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ pageToken := req.URL.Query().Get("pageToken")
+ pageSize := req.URL.Query().Get("pageSize")
+ r.Equal("", pageToken)
+ r.Equal(strconv.Itoa(customPageSize), pageSize)
+
+ // Function identifiers are CatalogObjectIdentifier values: flat
+ // arrays of hierarchy levels, not {namespace, name} objects.
+ json.NewEncoder(w).Encode(map[string]any{
+ "identifiers": []any{
+ []string{"accounting", "tax", "paid"},
+ []string{"accounting", "tax", "owed"},
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+ // Passing in a custom page size through context
+ ctx := cat.SetPageSize(context.Background(), customPageSize)
+
+ var lastErr error
+ functions := make([]table.Identifier, 0)
+ for function, err := range cat.ListFunctions(ctx, catalog.ToIdentifier(namespace)) {
+ functions = append(functions, function)
Review Comment:
Fixed, the iterator error is now checked before appending the returned identifier.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add SQL UDF REST catalog read support [iceberg-go]
Posted by "rraulinio (via GitHub)" <gi...@apache.org>.
rraulinio commented on code in PR #1432:
URL: https://github.com/apache/iceberg-go/pull/1432#discussion_r3569395798
##########
catalog/catalog.go:
##########
@@ -192,6 +194,30 @@ type PurgeableTable interface {
PurgeTable(ctx context.Context, identifier table.Identifier) error
}
+// FunctionCatalog is an optional interface implemented by catalogs that
+// support the Iceberg REST function (SQL UDF) endpoints. The function
+// endpoints are not part of the spec's assumed default endpoint set, so
+// servers must advertise them: unsupported loads report an
+// endpoint-not-supported error and unsupported listings yield no results.
Review Comment:
The endpoint-negotiation behavior is now documented on the REST-local interface and methods, alongside `rest.ErrEndpointNotSupported`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org