fix: treat unset IdField in zero-hit search results as empty in chain converter

Project: milvus-io/milvus | Language: Go | Concepts: error-handling, data-structures, testing-strategy | Difficulty: mid
Link: https://github.com/milvus-io/milvus/pull/51156

The problem

When performing a hybrid search with group-by on a collection that returns zero results (a "zero-hit" search), Milvus would sometimes fail with an error: service internal error: unsupported ID type. This happened because the search result could include an empty but non-nil Ids field whose internal IdField was unset, which the downstream converter did not handle correctly. This bug only surfaced in certain single-shard (single-channel) collection cases, but not in multi-shard cases, leading to inconsistent and confusing failures.

Why it's non-trivial

A naive fix might check only for Ids == nil to detect empty results, but in reality, the protobuf message can be non-nil with its internal oneof (IdField) unset. This subtlety means that simply checking for a nil pointer would miss cases where the shell exists but carries no data, leading to errors or panics when the code tries to process the unset field as if it were a valid ID type.

The approach

The fix updates the converter logic to treat a non-nil Ids with an unset IdField as empty, matching the behavior when Ids is nil. This is done by checking the internal oneof field directly:

// Before: only checked for nil Ids
-if ids := resultData.GetIds(); ids != nil {
-    if err := importIDs(builder, ids, offsets, alloc); err != nil {
-        return nil, err
-    }
-} else {
-    if err := importEmptyIDs(builder, offsets, alloc); err != nil {
-        return nil, err
-    }
-}

// After: checks if IdField is set, not just if Ids is non-nil
+if ids := resultData.GetIds(); ids.GetIdField() != nil {
+    if err := importIDs(builder, ids, offsets, alloc); err != nil {
+        return nil, err
+    }
+} else if ids == nil || len(offsets) == 0 {
+    if err := importEmptyIDs(builder, offsets, alloc); err != nil {
+        return nil, err
+    }
+} else if ids != nil {
+    // IdField unset but rows present: malformed input.
+    return nil, importIDs(builder, ids, offsets, alloc)
+}

• The new logic first checks if IdField is set. If so, it imports the IDs as before.
• If IdField is not set (including the case where Ids is nil or there are zero rows), it builds an empty ID column.
• If there are rows but the IdField is unset, it returns an error (malformed input).

The PR also adds new regression tests to cover these edge cases:

+func (s *ConverterSuite) TestFromSearchResultData_EmptyIDsZeroRows() {
+    for name, ids := range map[string]*schemapb.IDs{
+        "nil ids":             nil,
+        "shell without oneof": {}, // IDs with no IdField set
+    } {
+        for _, topks := range [][]int64{{0}, {0, 0}} {
+            resultData := &schemapb.SearchResultData{
+                NumQueries: int64(len(topks)),
+                TopK:       10,
+                Topks:      topks,
+                Scores:     []float32{},
+                Ids:        ids,
+                FieldsData: []*schemapb.FieldData{},
+            }
+            df, err := FromSearchResultData(resultData, s.pool, nil)
+            s.Require().NoError(err, name)
+            s.Equal(int64(0), df.NumRows(), name)
+            s.True(df.HasColumn(types.IDFieldName), name)
+            idType, ok := df.FieldType(types.IDFieldName)
+            s.True(ok, name)
+            s.Equal(schemapb.DataType_Int64, idType, name)
+            s.True(df.HasColumn(types.ScoreFieldName), name)
+            df.Release()
+        }
+    }
+}

• This test ensures both Ids == nil and a shell with unset IdField produce the same correct empty result.

Other new tests pin the behavior for nil IDs with rows, and for the case where there are no queries (Topks empty).

From the review thread

No notable review discussion on this PR.

Takeaways

• Always check the internal state of protobuf oneof fields, not just pointer nil-ness. Protobuf-generated Go structs can be non-nil but still unset internally; logic must check the actual oneof field to determine presence.
• Regression tests should cover both nil and "shell" (unset oneof) cases for optional fields. These subtle distinctions often cause bugs in real-world code.
• Handle all possible input shapes defensively, especially when dealing with results that may be empty or partially filled. This prevents internal errors or panics and ensures consistent downstream behavior.