Hello @deluan, 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 or gemini-code-assist here to provide a summary of this pull request. This PR aims to fix an issue where numeric comparisons on custom tags within smart playlist rules might not work correctly, especially for floating-point values. The core problem is that tag values are stored as text within a JSON structure, and direct comparison might treat them lexicographically rather than numerically.

The solution implemented involves modifying the SQL generation logic. When a comparison is performed on a custom tag and the argument provided for the comparison is detected as a number, the generated SQL is updated to explicitly cast the tag's value to a REAL number type using CAST(value AS REAL). This forces the database to perform a proper numeric comparison.

A new helper function, isNumber, is introduced to reliably check if a given Go value is one of the standard integer or float types. Finally, a dedicated test case is added to the operators test suite to specifically verify that numeric comparisons on custom tags now correctly produce SQL that includes the necessary CAST operation.

Highlights

• Fix Numeric Tag Comparisons: Addresses an issue preventing correct numeric comparisons (including floats) on custom tags in smart playlist rules.
• SQL Casting for Numeric Values: Modifies the SQL generation to include CAST(value AS REAL) for custom tag values when the comparison argument is numeric, ensuring proper numeric comparison.
• Add isNumber Helper: Introduces a new utility function to check if a variable holds a numeric type (int, uint, float types).
• Add Test Case: Includes a new test specifically validating that numeric comparisons on custom tags correctly generate SQL with the necessary casting.

Changelog

• model/criteria/fields.go
  • Added logic in tagCond.ToSql to check if the comparison argument is numeric and, if so, modify the generated SQL to cast the tag value to REAL (lines 148-150).
  • Added the isNumber helper function to check for various integer and float types (lines 159-167).
• model/criteria/operators_test.go
  • Added a new test case within the "Operators" suite to specifically verify that numeric comparisons (using Lt in the example) on custom tags correctly generate SQL with CAST(value AS REAL) (lines 80-86).

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.

Text becomes a number,
Casting makes the math work right,
Playlist rules now sing.

Code Review

This pull request effectively addresses an issue with numeric comparisons for custom tags stored as text by introducing a CAST to REAL in the SQL query when the comparison involves a numeric argument. The addition of the isNumber helper function and the corresponding test case are well-implemented. My main feedback point revolves around the behavior of the CAST operation on non-numeric string values within tags, which could lead to unexpected query results in certain edge cases.

Summary of Findings

• Behavior of SQL CAST on Non-Numeric Tag Values: When a custom tag intended for numeric comparison contains a non-numeric string (e.g., 'N/A', 'Unknown'), SQLite's CAST(value AS REAL) operation will convert such strings to 0.0. This can lead to unexpected outcomes, such as non-numeric tags matching conditions like tag = 0. This was raised as a medium severity issue requiring consideration for potential user impact or documentation.
• Test Coverage for Float Types in Comparisons: The new test case It("casts numeric comparisons") uses an integer value (6) for the comparison. While the isNumber function correctly identifies float types and CAST(... AS REAL) handles them, explicitly testing with a float literal (e.g., Lt{"rate": 6.5}) would offer slightly more direct confirmation for float value comparisons. This was deemed a low severity point and not formally commented on due to the configured review settings.

Merge Readiness

The core functionality of this PR is well-implemented and addresses the stated problem. There is one medium-severity consideration regarding the behavior of CAST on non-numeric strings that I believe should be discussed or acknowledged before merging to ensure there are no unintended consequences for users. Once that point is addressed (e.g., through documentation, further testing of edge cases, or confirmation of acceptable behavior), the PR should be in good shape. As a reviewer, I am not authorized to approve pull requests.

Code Review

This pull request introduces a valuable feature for handling numeric comparisons and sorting for custom tags, especially floats. The approach of adding a numeric flag to mappedField and using CAST AS REAL in SQL queries is generally well-aligned with the goal, particularly for SQLite environments. The inclusion of tests for both sorting and filtering scenarios is a positive step.

I have identified one high-severity issue related to how the CAST operation is applied in tagCond.ToSql, which could lead to incorrect behavior for string-based operators on numeric tags. Additionally, there's a medium-severity consideration regarding database agnosticism for the CAST AS REAL syntax.

Addressing the high-severity issue is recommended before merging.

Summary of Findings

• Broad Numeric Casting in Tag Conditions: In model/criteria/fields.go, the CAST(value AS REAL) operation is applied to any tag marked numeric within tagCond.ToSql(), regardless of the specific comparison operator. This can cause issues with string-based operators (like Contains, StartsWith) by incorrectly casting the value to numeric before the string operation, potentially leading to errors or unexpected results. The cast should be conditional, applying only to numeric comparison operators.
• Database-Specific Casting Syntax: The use of CAST(... AS REAL) is SQLite-specific. While appropriate for the current context if SQLite is the sole database target, it may require changes if support for other SQL databases is introduced in the future. This is a consideration for long-term maintainability and portability.

Merge Readiness

The pull request makes good progress in supporting numeric comparisons for custom tags. However, due to the high-severity issue identified with the broad application of numeric casting in tagCond.ToSql(), I recommend addressing this before merging. The suggested change aims to make the casting more precise and avoid unintended side effects with string-based operators.

The medium-severity point about database-specific syntax is more of a long-term consideration and doesn't block the current functionality for SQLite.

As a reviewer, I am not authorized to approve pull requests. Please ensure further review and approval from authorized maintainers after addressing the feedback.