Support more primitive types in ObjectWeaver

[xerial]

Summary

• Add ObjectWeaver implementations for Long, Double, Float, Boolean, Byte, Short, and Char primitive types
• Previously, only Int and String primitive types were supported in ObjectWeaver
• All new implementations include comprehensive type conversion support and proper error handling

Test plan

• All existing tests continue to pass
• New comprehensive test suite covers all primitive types with various conversion scenarios
• Range validation tests for numeric types (Byte, Short, Float)
• String parsing tests for Boolean with flexible true/false values
• Char conversion tests from single-character strings and integer char codes
• Error handling tests for unsupported type conversions

🤖 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 here, providing a summary of this pull request to help everyone get up to speed quickly.

This PR, authored by @xerial, aims to significantly enhance the ObjectWeaver's capability by adding support for several primitive data types. Previously, the ObjectWeaver could only handle Int and String. This change introduces dedicated ObjectWeaver implementations for Long, Double, Float, Boolean, Byte, Short, and Char.

The new implementations include comprehensive logic for packing these types and, importantly, unpacking them from various source ValueTypes (like Integer, Float, String, Boolean, Nil) with appropriate type conversion and robust error handling for invalid conversions or out-of-range values. A thorough test suite has also been added to validate the new weavers and their conversion logic.

Highlights

• Expanded Primitive Type Support: Adds ObjectWeaver implementations for Long, Double, Float, Boolean, Byte, Short, and Char, extending the weaver's functionality beyond the existing Int and String support.
• Comprehensive Type Conversion: Each new weaver includes logic to handle unpacking values from different MessagePack ValueTypes (Integer, Float, String, Boolean, Nil) and converting them to the target primitive type.
• Robust Error Handling: Includes specific error handling for invalid type conversions, number format exceptions, and out-of-range values during unpacking.
• New Test Suite: Adds extensive test cases covering basic pack/unpack operations, various type conversion scenarios, range validation for numeric types, specific string parsing for Boolean and Char, and error handling for unsupported conversions.

Changelog

Click here to see the changelog

• ai-core/src/main/scala/wvlet/ai/core/weaver/codec/PrimitiveWeaver.scala
  • Added ObjectWeaver implementations for Long, Double, Float, Boolean, Byte, Short, and Char.
  • Implemented pack methods for each new primitive type.
  • Implemented unpack methods for each new primitive type, including logic for converting from different ValueTypes (Integer, Float, String, Boolean, Nil).
  • Added range validation for Long, Byte, Short, and Char when converting from larger integer or float types.
  • Added specific string parsing logic for Boolean (handling 'true', 'false', '1', '0', 'yes', 'no', 'on', 'off', '').
  • Added logic for converting single-character strings and integer codes to Char.
  • Included error handling for invalid conversions and out-of-range values.
• ai-core/src/test/scala/wvlet/ai/core/weaver/codec/PrimitiveWeaverTest.scala
  • Added basic pack/unpack tests for Long, Double, Float, Boolean, Byte, Short, and Char.
  • Added tests for Long conversion from Boolean, String, and Nil.
  • Added tests for Boolean conversion from Integer (0 and non-zero) and various String representations ('true', 'false', 'yes', 'no', etc.).
  • Added range validation tests for Byte and Short when unpacking from larger integer types.
  • Added tests for Char conversion from single-character strings and integer character codes.
  • Added failure tests for Char conversion from multi-character strings.
  • Added a general test suite for error handling when attempting unsupported conversions (e.g., unpacking an Array into a primitive type).

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 several new primitive types in ObjectWeaver, which is a significant enhancement. The implementations are comprehensive in terms of type conversions and error handling, and the test suite is well-developed.

Summary of Findings

• Unhandled Exception Risk in String Unpacking: Calls to u.unpackString are sometimes outside try-catch blocks that set context.setError. If these unpack operations fail, the exception might not be handled as intended by the WeaverContext.
• Repetitive Try-Catch Blocks: Many try-catch blocks for basic operations are repeated across weavers. A helper method could improve maintainability.
• Test Specificity: Tests use intercept[Exception], which is broad. Intercepting more specific exceptions (e.g., IllegalArgumentException) and checking messages would make tests more robust.
• Missing Test Cases for Specific Conversions: Some specific type conversion paths (e.g., Long from Float, Double from Integer/Boolean/Nil/String, etc.) lack explicit tests.

Merge Readiness

The pull request adds valuable functionality. However, due to the potential unhandled exceptions and several issues related to test coverage and code maintainability, I recommend addressing these points before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from authorized team members.

[gemini-code-assist]

The u.unpackString call is made before the try-catch block. If u.unpackString throws an exception, it won't be caught, potentially bypassing the intended error handling. Should the call to u.unpackString be moved inside the try block to ensure exceptions during unpacking are channeled through context.setError(e)?

This applies to similar patterns in:

• doubleWeaver for ValueType.STRING (line 174)
• floatWeaver for ValueType.STRING (line 226)
• booleanWeaver for ValueType.STRING (line 282 - u.unpackString then s.toLowerCase)
• byteWeaver for ValueType.STRING (line 330)
• shortWeaver for ValueType.STRING (line 386)

try {
              val s = u.unpackString
              val longValue = s.toLong
              context.setLong(longValue)
            } catch {
              case e: NumberFormatException =>
                context.setError(
                  new IllegalArgumentException(s"Cannot convert string '${s}' to Long", e)
                )
              case e: Exception =>
                context.setError(e)
            }

[gemini-code-assist]

There's a recurring pattern of try { operation; context.setX(...) } catch { case e: Exception => context.setError(e) } throughout the new weaver implementations. Consider extracting this into a private helper method to reduce redundancy. For example:

private def attemptSet[A](context: WeaverContext)(operation: => A)(setter: A => Unit): Unit = {
  try {
    setter(operation)
  } catch {
    case e: Exception => context.setError(e)
  }
}

// Usage for this block:
// attemptSet(context)(u.unpackLong)(context.setLong)

[gemini-code-assist]

The tests frequently use intercept[Exception] to catch errors during unweaving. While this confirms an error occurs, it's quite broad. The ObjectWeaver framework typically sets specific IllegalArgumentException instances on the WeaverContext when conversions fail. Consider intercepting IllegalArgumentException and asserting the exception message content to make the tests more robust.

[gemini-code-assist]

The test suite is comprehensive. However, a few specific conversion paths might be missing explicit tests. Adding these could further improve confidence.

For example, for longWeaver:

• Conversion from ValueType.FLOAT: tests for valid whole doubles, non-whole doubles (should error), and out-of-range doubles (should error) seem to be missing. (Relevant longWeaver code: lines 115-124).

Similarly, consider adding explicit tests for these conversions if not already covered by broader tests:

• doubleWeaver: from INTEGER, BOOLEAN, NIL, STRING (lines 167-198).
• floatWeaver: from INTEGER, FLOAT (double source, including range checks for Float.MinValue/MaxValue), BOOLEAN, NIL, STRING (lines 209-250).
• byteWeaver: from FLOAT (with isWhole and range checks), BOOLEAN, NIL, STRING (lines 319-354).
• shortWeaver: from FLOAT (with isWhole and range checks), BOOLEAN, NIL, STRING (lines 375-410).
• charWeaver: from NIL (lines 443-449).

[xerial]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly expands the capabilities of ObjectWeaver by adding support for several new primitive types: Long, Double, Float, Boolean, Byte, Short, and Char. The inclusion of comprehensive type conversion logic and error handling is a valuable addition.

The introduction of helper methods like safeUnpack, safeConvertFromString, and safeUnpackNil is a good step towards reducing boilerplate and improving maintainability in PrimitiveWeaver.scala.
The test suite in PrimitiveWeaverTest.scala has also been commendably expanded to cover many scenarios for these new types.

However, there are a few areas that could be improved, primarily concerning the consistent use of the new helper methods and some aspects of the test coverage and assertions. Addressing these will further enhance the robustness and maintainability of the codebase.

As no specific style guide was provided, this review defaults to common Scala style conventions (e.g., clarity, conciseness, use of functional constructs, standard naming conventions), which are generally well-followed in this PR.

Summary of Findings

• Inconsistent Helper Method Usage: The newly introduced helper methods (safeUnpack, safeConvertFromString) are not consistently used across all new primitive weavers in PrimitiveWeaver.scala. floatWeaver, booleanWeaver, byteWeaver, shortWeaver, and charWeaver reimplement try-catch logic, leading to code duplication and potential maintenance issues. (Severity: High, Commented)
• Incomplete Test Coverage for New Types: Test suites for Float, Byte, and Short in PrimitiveWeaverTest.scala are missing comprehensive tests for conversions from various other types (e.g., String, Boolean, Nil, other numeric types with range checks), similar to those provided for Int and Long. (Severity: Medium, Commented)
• Inconsistent Exception Interception in Tests: Several new tests in PrimitiveWeaverTest.scala use intercept[Exception] and do not assert the exception message content. These should be updated to intercept[IllegalArgumentException] (or the most specific expected exception) and include message assertions for robustness and consistency. (Severity: Medium, Commented)
• Missing Boolean Conversion Tests (Low Severity): The Boolean weaver from various types test is missing scenarios for conversion from float/double (non-zero true, zero false) and from nil. (Severity: Low, Not commented due to review settings)
• Missing Char Conversion Tests (Low Severity): The Char weaver tests are missing range validation for integer-to-char conversion and tests for conversion from nil. (Severity: Low, Not commented due to review settings)

Merge Readiness

This pull request introduces valuable functionality by extending ObjectWeaver support to more primitive types. The core logic and initial tests are well-implemented.

However, before merging, I recommend addressing the identified issues, particularly:

1. Refactoring floatWeaver, booleanWeaver, byteWeaver, shortWeaver, and charWeaver to consistently use the new safeUnpack and safeConvertFromString helper methods.
2. Expanding the test suites for Float, Byte, and Short to include comprehensive "from various types" scenarios.
3. Standardizing exception interception and message assertions in the new tests.

Addressing these points will significantly improve the code's maintainability and test robustness. As a reviewer, I am not authorized to approve pull requests. Please ensure these changes are reviewed and approved by the appropriate maintainers before merging.

[gemini-code-assist]

The floatWeaver (and similarly booleanWeaver, byteWeaver, shortWeaver, charWeaver) reimplements try-catch logic for unpacking, rather than using the new helper methods safeUnpack and safeConvertFromString that were introduced in this PR and used in longWeaver and doubleWeaver.

Could these weavers be refactored to use the helper methods for consistency and to reduce code duplication? This would make the error handling more centralized and easier to maintain.

For example, in floatWeaver:

Case ValueType.FLOAT (lines 202-211):
This block:

case ValueType.FLOAT =>
  try
    val d = u.unpackDouble
    if d >= Float.MinValue && d <= Float.MaxValue then
      context.setFloat(d.toFloat)
    else
      context.setError(new IllegalArgumentException(s"Double ${d} out of Float range"))
  catch
    case e: Exception =>
      context.setError(e)

could potentially be refactored using safeUnpack like so:

case ValueType.FLOAT =>
  safeUnpack(
    context,
    {
      val d = u.unpackDouble
      if d >= Float.MinValue && d <= Float.MaxValue then
        d.toFloat
      else
        throw new IllegalArgumentException(s"Double ${d} out of Float range")
    },
    context.setFloat
  )

Case ValueType.STRING (lines 218-227):
This block:

case ValueType.STRING =>
  try
    val s          = u.unpackString
    val floatValue = s.toFloat
    context.setFloat(floatValue)
  catch
    case e: NumberFormatException =>
      context.setError(new IllegalArgumentException(s"Cannot convert string to Float", e))
    case e: Exception =>
      context.setError(e)

could be refactored using safeConvertFromString:

case ValueType.STRING =>
  safeConvertFromString(context, u, _.toFloat, context.setFloat, "Float")

Applying similar refactoring to the other indicated weavers would improve consistency.

[gemini-code-assist]

The test suite for floatWeaver includes basic operations but seems to be missing tests for conversions from various other types (e.g., Integer, String, Boolean, Nil) and specific range validation tests (e.g., when converting a Double that is out of Float's range but still a valid Double).

Could you consider adding a test("Float weaver from various types") similar to what's done for Long and Double to ensure comprehensive coverage for these conversion scenarios and edge cases?

[gemini-code-assist]

Similar to the comment for floatWeaver, the byteWeaver tests include basic operations and range validation from an integer, but could benefit from a more comprehensive test("Byte weaver from various types").

This would cover conversions from String (valid and invalid, including range checks), Boolean, and Float (checking for isWhole and range), and Nil, similar to the test structure for Int or Long.

[gemini-code-assist]

In the Short weaver range validation test, intercept[Exception] is used. For consistency with other improved tests (like for Int or Byte range validation) and for more precise testing, would it be better to intercept[IllegalArgumentException] and also assert the content of the exception message?

Additionally, similar to Float and Byte, shortWeaver could benefit from a test("Short weaver from various types") covering conversions from String, Boolean, Float, and Nil, including relevant range and format checks.

    val exception = intercept[IllegalArgumentException] {
      ObjectWeaver.unweave[Short](packed)
    }
    exception.getMessage.contains("out of Short range") shouldBe true // Or a more specific message if available

[gemini-code-assist]

In Char weaver from string - failure cases, intercept[Exception] is used. To align with best practices shown elsewhere in the test suite and to make the test more specific, could this be changed to intercept[IllegalArgumentException]? It would also be beneficial to assert the content of the exception message to ensure the correct error is being thrown.

    val exception = intercept[IllegalArgumentException] {
      ObjectWeaver.unweave[Char](packed)
    }
    // Example: Check if the message indicates it's due to string length or format
    exception.getMessage.contains("Cannot convert string") shouldBe true 
    // Or, more specifically, if the message is about single character requirement:
    // exception.getMessage.contains("must be single character") shouldBe true

[gemini-code-assist]

In the Primitive type error handling test, intercept[Exception] is used for each unpack operation. For more precise error checking and consistency, would it be better to intercept[IllegalArgumentException]?

Additionally, asserting the exception message (e.g., to check if it correctly states "Cannot convert ARRAY to [TypeName]") would make these tests more robust.

    for unpackOperation <- primitiveTypes do
      val exception = intercept[IllegalArgumentException] {
        unpackOperation()
      }
      // Example assertion, assuming the message format is consistent
      exception.getMessage.startsWith("Cannot convert ARRAY to") shouldBe true