feature: Add List type support to ObjectWeaver

[xerial]

Summary

• Add generic listWeaver given instance for List[A] types in PrimitiveWeaver
• Implement proper array packing/unpacking with MessagePack format
• Add comprehensive test coverage for various List scenarios

Test plan

• Test List[Int] serialization/deserialization
• Test empty List handling
• Test List[String] support
• Test JSON conversion for List types
• Test nested List[List[Int]] structures
• All existing tests continue to pass

🤖 Generated with Claude Code

[gemini-code-assist]

Hello @xerial, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team, gemini-code-assist here to provide a summary of this pull request. The primary goal of this PR is to enhance the ObjectWeaver component by adding support for serializing and deserializing Scala List types. This involves implementing the necessary packing and unpacking logic using the MessagePack format and adding comprehensive test coverage to ensure correct behavior for various list scenarios, including different element types, empty lists, and nested structures.

Highlights

• List Type Support: Introduces a generic ObjectWeaver instance for List[A] types within PrimitiveWeaver, allowing ObjectWeaver to handle Scala Lists.
• MessagePack Implementation: Adds the specific pack and unpack methods for List[A], utilizing MessagePack's array header and element-by-element processing.
• Comprehensive Testing: Includes new test cases covering serialization/deserialization of List[Int], empty List[Int], List[String], JSON conversion for lists, and nested List[List[Int]] structures.

Changelog

Click here to see the changelog

• ai-core/src/main/scala/wvlet/ai/core/weaver/codec/PrimitiveWeaver.scala
  • Added scala.collection.mutable.ListBuffer import (line 5).
  • Implemented given listWeaver[A] providing ObjectWeaver[List[A]] (lines 395-431).
  • Added pack method to write array header and pack each element (lines 397-399).
  • Added unpack method to handle MessagePack arrays, unpack elements into a ListBuffer, and convert to List (lines 401-425).
  • Included error handling and support for NIL values during unpacking (lines 414-417, 426-430).
• ai-core/src/test/scala/wvlet/ai/core/weaver/WeaverTest.scala
  • Added test weave List[Int] for basic List serialization/deserialization (lines 35-40).
  • Added test weave empty List[Int] to verify handling of empty lists (lines 42-47).
  • Added test weave List[String] for lists of String elements (lines 49-54).
  • Added test List[Int] toJson to check JSON conversion for lists (lines 56-61).
  • Added test nested List[List[Int]] to ensure support for nested list structures (lines 63-68).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces support for List[A] types in ObjectWeaver, complete with MessagePack packing/unpacking and JSON conversion capabilities. The implementation is generally well-structured, and the accompanying tests cover various important scenarios, including empty lists, lists of different primitive types, and nested lists.

Summary of Findings

• Unpacker State on Partial Array Read: In listWeaver.unpack, if an error occurs while deserializing an element of a list, the remaining elements in that list are not skipped from the input stream. This could leave the Unpacker in an inconsistent state, potentially causing issues for subsequent deserialization operations on the same stream. This was flagged as a 'high' severity issue.

Merge Readiness

The pull request is a valuable addition, significantly enhancing ObjectWeaver's capabilities. The code is mostly in good shape. However, there is one high-severity issue related to Unpacker state management during list deserialization that should be addressed to ensure robustness. Once this concern is resolved, the PR should be in a great state for merging. As always, further review and approval by other maintainers are recommended. I am not authorized to approve this pull request.

[gemini-code-assist]

When an error occurs while unpacking an element within a list (i.e., elementContext.hasError is true), the error is correctly propagated to the parent context, and the hasError flag is set to terminate the loop. However, the remaining elements of the array in the input stream are not skipped. If the Unpacker instance (u) is reused after this operation (e.g., to read subsequent data from the same stream), it might be positioned in the middle of the partially read array, potentially leading to parsing errors for the next data structure. Could we consider skipping the remaining elements of the array in the Unpacker when an element-level error is detected to ensure the unpacker is in a consistent state (i.e., at the end of the array structure it was attempting to parse)?

                if elementContext.hasError then
                  context.setError(elementContext.getError.get)
                  hasError = true
                  // Skip the rest of the elements in the array to leave the unpacker in a consistent state.
                  // Current element 'i' caused the error, so we need to skip (arraySize - 1 - i) more elements.
                  val elementsProcessedIncludingError = i + 1
                  val remainingElementsToSkip = arraySize - elementsProcessedIncludingError
                  if remainingElementsToSkip > 0 then
                    try {
                      var k = 0
                      while k < remainingElementsToSkip do
                        u.skipValue()
                        k += 1
                    } catch {
                      case e: Exception => 
                        // Optional: Log this secondary error, but the primary error is already set.
                        // Be cautious not to overwrite the original error on the context.
                        System.err.println(s"[WARN] Failed to skip remaining ${remainingElementsToSkip - k} elements in array after error: ${e.getMessage}")
                    }
                else
                  buffer += elementContext.getLastValue.asInstanceOf[A]
                  i += 1

[xerial]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces support for List[A] types in ObjectWeaver, including MessagePack serialization/deserialization and comprehensive test cases. The changes look solid and well-tested.

I have one suggestion regarding the deserialization of NIL values for lists, aiming for consistency with how other primitive types handle NIL (e.g., String deserializes NIL to an empty string) and to align with common Scala practices of preferring empty collections over null.

No explicit style guide was provided for this repository. The review assumes adherence to common Scala best practices and conventions, such as those found in the Scala Style Guide or Effective Scala. The submitted code generally aligns well with these practices.

Overall, great work on adding this feature and the thorough test coverage!

Summary of Findings

• NIL Deserialization for Lists: Currently, a NIL value in MessagePack deserializes to a null List. It might be more consistent and idiomatic to deserialize NIL to List.empty[A].

Merge Readiness

The pull request is in good shape and the core functionality is well-implemented and tested. There's one medium-severity suggestion regarding NIL deserialization that would be good to discuss and potentially address for improved consistency. After considering that point, this PR should be ready for merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from authorized maintainers.

[gemini-code-assist]

When encountering a ValueType.NIL for a List[A], the current implementation safeUnpackNil(context, u) results in context.setNull, meaning the list will be deserialized as null.

Consider deserializing NIL to an empty list (List.empty[A]) instead. This would be more consistent with how stringWeaver handles NIL (it produces an empty string "") and generally aligns better with Scala idioms that prefer empty collections over null values.

What are your thoughts on this change for consistency and idiomatic Scala usage?