Skip to content

feat: Add YawnComparison enum to yawn-api#125

Closed
chinhtrung wants to merge 7 commits intomainfrom
cursor/yawn-comparison-enum-0309
Closed

feat: Add YawnComparison enum to yawn-api#125
chinhtrung wants to merge 7 commits intomainfrom
cursor/yawn-comparison-enum-0309

Conversation

@chinhtrung
Copy link
Copy Markdown
Contributor

@chinhtrung chinhtrung commented Mar 25, 2026

Follow up from this backend PR, we want to create a way to pass a typesafe comparison operators (EQ, LT, LE, GT, GE) as a parameter in Kotlin.

Summary

Adds a YawnComparison enum to the com.faire.yawn.query package in yawn-api. This provides a type-safe way to pass comparison operators (EQ, LT, LE, GT, GE) as a parameter, working around Kotlin's lack of higher-rank polymorphism on function types.

Motivation

When writing helper functions that apply the same comparison operator to multiple columns with different generic types (e.g. ColumnDef<DateTime> and ColumnDef<DateTime?>), you can't pass YawnRestrictions::le directly as a function parameter because ColumnDef<F> is invariant in F and Kotlin function types require F to be fixed. An enum with a generic compare method solves this — F is resolved independently at each call site.

Changes

  • New file: yawn-api/src/main/kotlin/com/faire/yawn/query/YawnComparison.kt
    • enum class YawnComparison with variants EQ, LT, LE, GT, GE
    • Generic compare method that delegates to the corresponding YawnRestrictions method
    • Placed in the same package as YawnRestrictions and YawnQueryCriterion
    • Public API for consumers of yawn-api
  • New file: yawn-api/src/test/kotlin/com/faire/yawn/query/YawnComparisonTest.kt
    • Unit tests demonstrating the realistic use case: a helper function that applies the same comparison to columns of different generic types (Int, String, Long)
    • Each column's restriction type is asserted separately for readability
    • Verifies switching the comparison variant changes all generated restrictions
    • Verifies each variant maps to the correct restriction type
    • Verifies all five variants are present
  • New file: yawn-database-test/src/test/kotlin/com/faire/yawn/database/YawnComparisonDatabaseTest.kt
    • Database integration tests using H2 with the BookFixtures dataset
    • Tests each comparison variant (LT, LE, GT, EQ) against real queries
    • Tests applying a single YawnComparison to columns of different types (Long and String) in the same query via a shared helper function
    • Tests that switching the comparison variant changes query results

Adds a type-safe enum for comparison operators (LT, LE, GT, GE) with a
generic compare method that delegates to YawnRestrictions. This works
around Kotlin's lack of higher-rank polymorphism on function types when
passing comparison operators as parameters to helper functions that apply
the same operator to columns with different generic types.

Co-authored-by: Nguyen Chinh Trung <chinhtrung596@gmail.com>
@cursor cursor bot changed the title Add YawnComparison enum to yawn-api feat: Add YawnComparison enum to yawn-api Mar 25, 2026
cursoragent and others added 5 commits March 25, 2026 18:10
- Add YawnComparisonTest with 6 tests covering all enum variants
- Fix compare() parameter from 'value: F' to 'value: F & Any' to match
  the YawnRestrictions method signatures

Co-authored-by: Nguyen Chinh Trung <chinhtrung596@gmail.com>
Include equality comparison alongside the ordering comparisons,
delegating to YawnRestrictions.eq. Add corresponding test.

Co-authored-by: Nguyen Chinh Trung <chinhtrung596@gmail.com>
Replace mechanical delegation tests with tests that demonstrate the
actual use case: a helper function that applies the same comparison
operator to columns of different generic types (Int, String, Long).
This is the pattern that Kotlin function references cannot express
due to invariance of the type parameter F.

Co-authored-by: Nguyen Chinh Trung <chinhtrung596@gmail.com>
…ottom

- Destructure results into named variables (amountCriterion, etc.)
  and assert each column's restriction type individually
- Move private buildOrderCriteria helper to the bottom of the class
- Return Triple instead of List to enable destructuring

Co-authored-by: Nguyen Chinh Trung <chinhtrung596@gmail.com>
Put each argument on its own line with named parameters to satisfy
the ArgumentListWrapping lint rule.

Co-authored-by: Nguyen Chinh Trung <chinhtrung596@gmail.com>
@chinhtrung chinhtrung marked this pull request as ready for review March 25, 2026 19:09
@chinhtrung chinhtrung requested a review from luanpotter March 25, 2026 19:10
Test YawnComparison against a real H2 database using the existing
BookFixtures data. Includes tests for each comparison variant (LT, LE,
GT, EQ) and two tests demonstrating the core use case: applying a
single YawnComparison to columns of different types (Long and String)
in the same query via a helper function.

Co-authored-by: Nguyen Chinh Trung <chinhtrung596@gmail.com>
@chinhtrung chinhtrung requested a review from luanpotter March 25, 2026 19:45
@chinhtrung
Copy link
Copy Markdown
Contributor Author

/poke 🙇‍♂️

@faire-pr-bot
Copy link
Copy Markdown

/poke 🙇‍♂️

Sorry, I didn't understand that.

@chinhtrung
Copy link
Copy Markdown
Contributor Author

/poke

@faire-pr-bot
Copy link
Copy Markdown

/poke

Poked 1 reviewers for PR

@luanpotter luanpotter requested a review from a team March 26, 2026 14:06
Copy link
Copy Markdown
Collaborator

@QuinnB73 QuinnB73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this LGTM. However, I'm not sure that this should be part of Yawn's contract or if it should remain internal to our own usage of Yawn 🤔

I.e. are we adding too many ways to do things? Curious for your thoughts (and @luanpotter's as well of course!)

}

@Test
fun `all comparison variants are present`() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test is all that valuable, it's subsumed by the restriction type test

@luanpotter
Copy link
Copy Markdown
Member

@QuinnB73 I think it is worth if there is a good use case. I think the use case here is to provide generic functions that take an operator and do multiple operations using Yawn. but I guess my question is can't it just take the restriction lambda (ie YawnRestrictions.le) instead of an enum? is iterativability a concern? or the ability of having a function of shape fn(criteira, operation, ...). and could that be already solved with the existing api

@QuinnB73
Copy link
Copy Markdown
Collaborator

@QuinnB73 I think it is worth if there is a good use case. I think the use case here is to provide generic functions that take an operator and do multiple operations using Yawn. but I guess my question is can't it just take the restriction lambda (ie YawnRestrictions.le) instead of an enum? is iterativability a concern? or the ability of having a function of shape fn(criteira, operation, ...). and could that be already solved with the existing api

Yeah that makes sense, worth trying @chinhtrung !

@chinhtrung
Copy link
Copy Markdown
Contributor Author

@QuinnB73 I think it is worth if there is a good use case. I think the use case here is to provide generic functions that take an operator and do multiple operations using Yawn. but I guess my question is can't it just take the restriction lambda (ie YawnRestrictions.le) instead of an enum? is iterativability a concern? or the ability of having a function of shape fn(criteira, operation, ...). and could that be already solved with the existing api

Yeah that makes sense, worth trying @chinhtrung !

@QuinnB73 @luanpotter yeah, I gave that a try in this backend PR, but I think we couldn't just pass the operation YawnRestrictions.le as a param.

@QuinnB73
Copy link
Copy Markdown
Collaborator

@QuinnB73 I think it is worth if there is a good use case. I think the use case here is to provide generic functions that take an operator and do multiple operations using Yawn. but I guess my question is can't it just take the restriction lambda (ie YawnRestrictions.le) instead of an enum? is iterativability a concern? or the ability of having a function of shape fn(criteira, operation, ...). and could that be already solved with the existing api

Yeah that makes sense, worth trying @chinhtrung !

@QuinnB73 @luanpotter yeah, I gave that a try in this backend PR, but I think we couldn't just pass the operation YawnRestrictions.le as a param.

I think Luan is saying that you can pass a lambda as a param

@chinhtrung
Copy link
Copy Markdown
Contributor Author

I think Luan is saying that you can pass a lambda as a param

@QuinnB73 @luanpotter, yes I got the idea of passing a lambda as a param too. However, I couldn't get to that solution because of the type checking in Kotlin. I also attempt with ai agent but it seems to mention similar things. Can you give it a try in this case? 🙏

Screenshot 2026-03-26 at 1 33 43 PM

@luanpotter
Copy link
Copy Markdown
Member

could you do <F: DateTime?> ?

@chinhtrung chinhtrung closed this Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants