laskoviymishka commented on code in PR #1617:
URL: https://github.com/apache/iceberg-go/pull/1617#discussion_r3740300998
##########
table/equality_delete_reader.go:
##########
@@ -547,12 +677,20 @@ func processEqualityDeletesColumnar(ctx context.Context,
eqDeleteSets []*equalit
for _, eqDel := range eqDeleteSets {
encoders := make([]colEncoder, len(eqDel.colNames))
for i, name := range eqDel.colNames {
- indices := r.Schema().FieldIndices(name)
- if len(indices) == 0 {
- return nil, fmt.Errorf("equality delete
column %q not found in data record", name)
+ fieldID := 0
+ if i < len(eqDel.fieldIDs) {
+ fieldID = eqDel.fieldIDs[i]
}
- encoders[i] =
makeColEncoder(r.Column(indices[0]))
+ ref, err := resolveArrowField(r.Schema(),
fieldID, name, dataFilePath)
Review Comment:
I'd resolve these once before returning the closure rather than per batch.
`resolveArrowField` runs on every record batch here, but the schema is constant
across all batches of a file, so we're repeating the full recursive walk with
identical inputs each time, and the recursive struct descent makes that more
expensive than the old flat `FieldIndices` scan it replaced.
`readEqualityDeleteFile` already does this the right way: it resolves the
`fieldRefs` once up front and reuses them in the batch loop. I'd mirror that
here, computing the refs above the `return func(...)` and closing over them.
wdyt?
##########
table/equality_delete_reader.go:
##########
@@ -547,12 +677,20 @@ func processEqualityDeletesColumnar(ctx context.Context,
eqDeleteSets []*equalit
for _, eqDel := range eqDeleteSets {
encoders := make([]colEncoder, len(eqDel.colNames))
for i, name := range eqDel.colNames {
- indices := r.Schema().FieldIndices(name)
- if len(indices) == 0 {
- return nil, fmt.Errorf("equality delete
column %q not found in data record", name)
+ fieldID := 0
Review Comment:
This defensive guard quietly makes the ID-first resolution optional. When `i
>= len(eqDel.fieldIDs)`, `fieldID` stays `0`, so `resolveArrowField` skips the
`fieldID > 0` branch entirely and falls back to name-only lookup, which is the
behavior this PR is trying to get away from.
Today `fieldIDs` and `colNames` are always co-built at equal length so this
never trips, but if that invariant ever slips we'd silently lose ID matching
with no error to point at. I'd drop the guard and index `eqDel.fieldIDs[i]`
directly, or assert the two are equal-length at construction, so a mismatch
fails loudly instead of degrading. wdyt?
##########
table/equality_delete_reader.go:
##########
@@ -350,7 +476,7 @@ func encodeArrowValue(buf *bytes.Buffer, arr arrow.Array,
idx int) {
// rows whose equality key columns match any entry in the delete sets.
// Each set is applied independently (they may have different field IDs).
func processEqualityDeletes(ctx context.Context, eqDeleteSets
[]*equalityDeleteSet) (recProcessFn, error) {
- return processEqualityDeletesColumnar(ctx, eqDeleteSets)
+ return processEqualityDeletesColumnarForFile(ctx, eqDeleteSets, "")
Review Comment:
Now that `arrow_scanner.go` calls `processEqualityDeletesColumnarForFile`
directly, this wrapper (and `processEqualityDeletesColumnar` just below) has no
production callers left: `processEqualityDeletes` is dead, and
`processEqualityDeletesColumnar` only survives because the internal test calls
it.
I'd collapse both into `processEqualityDeletesColumnarForFile` and point the
test at it with a synthetic path. That keeps one entry point and stops the
bench in `equality_delete_reader_bench_test.go` from quietly exercising the
empty-path variant instead of the real one.
##########
table/equality_delete_reader.go:
##########
@@ -46,6 +50,124 @@ type equalityDeleteSet struct {
colNames []string
}
+type arrowFieldRef struct {
+ path []int
+}
+
+func resolveArrowField(schema *arrow.Schema, fieldID int, fieldName, filePath
string) (arrowFieldRef, error) {
+ type candidate struct {
+ ref arrowFieldRef
+ pathName string
+ hasIDMetadata bool
+ }
+
+ var (
+ idMatches []candidate
+ nameMatches []candidate
+ pathMatches []candidate
+ )
+ targetName := fieldName
+ if dot := strings.LastIndexByte(targetName, '.'); dot >= 0 {
+ targetName = targetName[dot+1:]
+ }
+
+ var visit func([]arrow.Field, []int, string)
+ visit = func(fields []arrow.Field, parentPath []int, parentName string)
{
+ for i, field := range fields {
+ path := append(append([]int(nil), parentPath...), i)
+ pathName := field.Name
+ if parentName != "" {
+ pathName = parentName + "." + field.Name
+ }
+ fieldIDValue := getFieldID(field)
+ fieldHasIDMetadata := fieldIDValue != nil
+
+ if fieldIDValue != nil && *fieldIDValue == fieldID {
+ idMatches = append(idMatches, candidate{ref:
arrowFieldRef{path: path}, pathName: pathName, hasIDMetadata:
fieldHasIDMetadata})
+ }
+ if field.Name == targetName {
+ match := candidate{ref: arrowFieldRef{path:
path}, pathName: pathName, hasIDMetadata: fieldHasIDMetadata}
+ nameMatches = append(nameMatches, match)
+ if pathName == fieldName {
+ pathMatches = append(pathMatches, match)
+ }
+ }
+
+ if nested, ok := field.Type.(*arrow.StructType); ok {
+ visit(nested.Fields(), path, pathName)
+ }
+ }
+ }
+ visit(schema.Fields(), nil, "")
+
+ location := filePath
+ if location == "" {
+ location = "data record"
+ }
+
+ if fieldID > 0 {
+ switch len(idMatches) {
+ case 1:
+ return idMatches[0].ref, nil
+ case 0:
Review Comment:
The empty `case 0:` reads like a dropped line. It took me a second to see
that zero ID matches is meant to fall through to the name-based resolution
below rather than being an unfinished branch, and revive's empty-block check
will likely flag it too.
I'd add a `// zero ID matches, fall through to name resolution` line inside
the case, or drop the switch for explicit `if len(idMatches) == 1 {...}` / `if
len(idMatches) > 1 {...}`. Either's fine, just something that signals the
fall-through is intentional.
--
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]
