fix(transform): validate SetLimit rejects negative values

[ajitpratap0]

Fixes #291

SetLimit already validated negative values at runtime, but test coverage was incomplete. This adds tests for:

• SetLimit(-100) — large negative value
• SetLimit(0) — valid SQL (LIMIT 0 returns empty result set)
• SetOffset(0) — valid SQL (no offset)

[claude]

Claude finished @ajitpratap0's task —— View job

---

PR Review: SetLimit Validation Tests

Todo List:

• Read and analyze the modified test file
• Examine the broader transform package context
• Review code quality and best practices
• Check for potential bugs or issues
• Assess test coverage improvements
• Provide comprehensive feedback

📋 Code Review Summary

This PR addresses issue #291 by adding comprehensive test coverage for SetLimit and SetOffset validation. The implementation correctly validates negative values as documented.

✅ Positive Aspects

Test Coverage Excellence

The new tests cover crucial edge cases that were missing:

• TestSetLimit_NegativeLarge (lines 75-81): Tests large negative values (-100)
• TestSetLimit_Zero (lines 83-93): Validates that LIMIT 0 is correctly allowed (valid SQL)
• TestSetOffset_Zero (lines 95-105): Validates that OFFSET 0 is correctly allowed (valid SQL)
• TestSetOffset_Negative (lines 107-113): Ensures negative offsets are properly rejected

Code Quality

• Excellent comments: Lines 84-85 and 96-97 clearly explain why zero values should succeed
• Consistent patterns: Follows established test conventions with mustParse(), assertContains()
• Proper error assertions: Uses t.Fatalf() with descriptive messages (lines 88, 100)
• Safe type usage: No unsafe operations or potential panics

Implementation Alignment

The tests perfectly align with the runtime validation in pkg/transform/limit.go:13-15:

if n < 0 {
    return fmt.Errorf("SetLimit: value must be non-negative, got %d", n)
}

🔍 Technical Analysis

Performance Considerations

• Zero overhead: Test additions don't impact runtime performance
• Efficient patterns: Uses existing test helpers (mustParse, format, assertContains)

Security Assessment

• Input validation: Tests confirm proper validation of malicious/invalid inputs
• No vulnerabilities: All test cases use safe, controlled inputs

Memory Management

• Pool compliance: Tests follow GoSQLX memory pool patterns via ParseSQL() wrapper
• No leaks: Proper cleanup through existing test infrastructure

🎯 Test Coverage Impact

Before this PR: Negative value validation was implemented but not fully tested
After this PR: Complete edge case coverage for limit/offset validation

The tests validate the complete specification:

• ✅ Positive values work (TestSetLimit)
• ✅ Zero values work (new tests)
• ✅ Small negative values rejected (TestSetLimit_Negative)
• ✅ Large negative values rejected (new TestSetLimit_NegativeLarge)
• ✅ Offset validation follows same patterns

💡 Minor Enhancement Suggestions

Test Naming Consistency

Consider standardizing test names:

• TestSetLimit_NegativeLarge → TestSetLimit_LargeNegative (to match lexical ordering)

Error Message Validation

While not required for this fix, consider testing specific error messages:

expectedErr := "SetLimit: value must be non-negative, got -100"
if err == nil || !strings.Contains(err.Error(), "non-negative") {
    t.Fatalf("expected non-negative error, got: %v", err)
}

✅ Final Assessment

Recommendation: ✅ APPROVE

This is a high-quality PR that:

• ✅ Addresses the stated issue (transform.SetLimit accepts negative and zero values without error #291) completely
• ✅ Follows project conventions perfectly
• ✅ Adds meaningful test coverage with zero risks
• ✅ Includes clear, helpful comments
• ✅ No performance, security, or correctness concerns

The implementation demonstrates good understanding of SQL semantics (LIMIT 0 and OFFSET 0 are valid) and proper Go testing practices. The tests will prevent regressions and improve confidence in the validation logic.

---

[ajitpratap0]

Consolidated into batch PR #304