Skip to content

Cast enums in lt(), lte(), gt(), gte()#2014

Merged
tyt2y3 merged 2 commits into
SeaQL:masterfrom
Expurple:enum-ord-operators
Jan 12, 2024
Merged

Cast enums in lt(), lte(), gt(), gte()#2014
tyt2y3 merged 2 commits into
SeaQL:masterfrom
Expurple:enum-ord-operators

Conversation

@Expurple
Copy link
Copy Markdown
Member

@Expurple Expurple commented Dec 17, 2023

Revert 10f3de0, this was discussed under #1527. There weren't any tests for these operators in that initial PR (#258). Let me know if mine can't be merged without adding some. If that's the case, I'm not sure where they should go, bloating tests::active_enum_tests::insert_active_enum doesn't feel right.

@tyt2y3
Copy link
Copy Markdown
Member

tyt2y3 commented Dec 17, 2023

Thank you. Yes, tests are important. I guess the best way is probably for you to add a new table under tests/common/features.

@Expurple
Copy link
Copy Markdown
Member Author

Ok, so I tried to add a new table and use it for tests. Then I realized that comparison results depend on the enum's db_type annotation (whether it's an Integer, String or Enum) and that existing ActiveEnum table conveniently contains columns for all three enum representations. So I ended up using that.

Postgres and SQLite look fine, but I got surprising results for MySQL (see the code comments). If you don't mind, I would like to merge this as is to get Postgres working and create a separate issue for MySQL, which I don't want to dig into at the moment.

Copy link
Copy Markdown
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Happy new year and thank you!

@tyt2y3 tyt2y3 merged commit a73f699 into SeaQL:master Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants