tanmayrauth commented on code in PR #1857: URL: https://github.com/apache/iceberg-go/pull/1857#discussion_r3836466962
########## table/scan_planning_remote.go: ########## @@ -0,0 +1,80 @@ +// 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 table + +import ( + "slices" + + "github.com/apache/iceberg-go" +) + +type fullRemoteScanPlanner interface { + SupportsFullRemoteScanPlanning() bool +} + +// supportsAutomaticRemotePlanning is deliberately more conservative than +// explicit remote mode when a planner exposes a split capability surface. A +// REST server that only advertises the initial /plan endpoint may complete an +// inline plan, but auto mode cannot know that before submitting and has no safe +// local fallback once the server responds with a continuation handle. +func supportsAutomaticRemotePlanning(planner ScanPlanner) bool { + if planner == nil { + return false + } + if full, ok := planner.(fullRemoteScanPlanner); ok { + return full.SupportsFullRemoteScanPlanning() + } + + return planner.SupportsRemoteScanPlanning() +} + +// remotePlanningSelectedFields returns the fully qualified physical field names +// sent for a wildcard REST scan projection. It mirrors Java's +// TypeUtil.getProjectedIds + Schema.findColumnName behavior. Java includes +// struct field IDs as well as primitive/variant field IDs, while list/map field +// IDs are represented by their nested element/key/value fields. Explicit +// projections keep their user-provided names unchanged. +func remotePlanningSelectedFields(scan *Scan, schema *iceberg.Schema) ([]string, error) { + if schema == nil || !slices.Contains(scan.selectedFields, "*") { + return scan.remoteSelectedFields(schema), nil + } + + idToField, err := iceberg.IndexByID(schema) + if err != nil { + return nil, err + } + + ids := make([]int, 0, len(idToField)) + for id, field := range idToField { + switch field.Type.(type) { + case *iceberg.ListType, *iceberg.MapType: Review Comment: I think the wildcard expansion might not line up with Java once a column is a list<struct> or map<*,struct>, and since a plain scan defaults to select ["*"] (table.go:1305) it'd be the common path rather than an edge case. Skipping only the fields whose own type is List/Map and then taking the rest from IndexByID keeps the synthetic container ids that Java's getProjectedIds leaves out: - list<struct<a,b>>: this produces ["col.element","col.element.a","col.element.b"], where Java keeps only the leaves ["col.element.a","col.element.b"] (it adds the element id only when the element is a primitive). - map<int,struct<a,b>>: this produces ["col.key","col.value","col.value.a","col.value.b"], where Java keeps only ["col.value.a","col.value.b"] — dropping both the map key and the value-struct container. The concern is that a select * scan over a table with a map<k,struct> or list<struct> column would hand the server col.key / col.element / col.value, and a server that derives the same projected-id set as the Java reference might reject those names or plan a different projection than the client expects. The existing test passes because a plain struct is the one nested shape where the two happen to agree, so it doesn't catch this. One option would be to follow getProjectedIds directly: keep primitive leaves and struct ids only when the struct is a real (top-level or struct-nested) field, add a list element id only when the element is a primitive, and for a map add key+value only when the value is a primitive — otherwise just the value's leaves, dropping the key. Then map the ids through FindColumnName as you do now. Might be worth adding list<struct> and map<*,struct> cases to the test as well. ########## catalog/rest/scan_planning.go: ########## @@ -223,50 +220,72 @@ func (r *Catalog) PlanFiles(ctx context.Context, req table.ScanPlanningRequest) case PlanStatusSubmitted: completed, err = r.WaitForPlan(ctx, req.Identifier, *resp.PlanID, WaitForPlanOptions{}) if err != nil { + // WaitForPlan already abandons plans when its retry budget or the + // caller's context is exhausted. Other terminal client/transport + // errors can leave a submitted plan active, so release it here. + if !errors.Is(err, context.Canceled) && + !errors.Is(err, context.DeadlineExceeded) && + !errors.Is(err, ErrPlanPollExhausted) && + !errors.Is(err, ErrPlanFailed) && + !errors.Is(err, ErrPlanCancelled) && + !errors.Is(err, ErrPlanExpired) && + !errors.Is(err, catalog.ErrNoSuchTable) && + !errors.Is(err, catalog.ErrNoSuchNamespace) { + cleanup() + } + return table.ScanPlanningResult{}, err } default: return table.ScanPlanningResult{}, fmt.Errorf( "%w: unexpected plan status %q from planTableScan", ErrRESTError, resp.Status) } - files, deletes, err := r.collectScanTasks(ctx, req.Identifier, completed.ScanTasks) + envelopes, err := r.collectScanTasks(ctx, req.Identifier, completed.ScanTasks) if err != nil { + cleanup() + return table.ScanPlanningResult{}, err } - tasks, err := remoteScanTasks(files, deletes) + tasks, err := remoteScanTasks(envelopes, req) if err != nil { + cleanup() + return table.ScanPlanningResult{}, err } - return table.ScanPlanningResult{ + result := table.ScanPlanningResult{ Tasks: tasks, - IO: planIOFromCredentials(completed.StorageCredentials, req.MetadataLocation, r.planIOBaseProps(req.Metadata)), - }, nil + IO: planIOFromCredentials(completed.StorageCredentials, req.MetadataLocation, r.planIOBaseProps(req)), + } + cleanup() Review Comment: One thing I wanted to flag here: the plan gets cancelled before any files are read, which is the reverse of the ordering Java uses. The IO from planIOFromCredentials is lazy, so ReadTasks doesn't open the data/delete files (using the vended plan.storage-credentials) until after PlanFiles returns — by which point this cleanup() has already sent DELETE /plan/{id}. And since completed responses carry a plan-id (enforced at line 1043), this would happen on every successful remote scan. Java handles it the other way around: it wraps the task iterable in whenComplete(..., cancelPlan) so the DELETE only runs when the iterable is closed — after the files are read — and keeps the credential-bearing FileIO alive past he cancel. The spec doesn't actually say whether vended creds outlive the plan, so I don't think this is a guaranteed break; it's more that Java avoids the question by not deleting first. If a server does tie those creds (or file access) to the plan being alive, reads here could come back 403 where Java's wouldn't, and even on a lenient server it's an extra DELETE per scan. Might be worth mirroring Java: when planIOFromCredentials returns a non-nil IO, defer the cancel to the plan-scoped IO's close path (closePlanIO / releasePlanIOAfter) so the plan lives as long as the reads that need its creds; when nothing's vended (nil), cancelling right here is fine, and the error paths above can keep cancelling eagerly as they do. That'd also mean adjusting TestPlanFilesCancelsAfterSuccessfulMaterialization, which currently pins the eager behavior. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
