Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bug: PrimitiveLiteral and Literal should not be Ord. #378

Closed
liurenjie1024 opened this issue May 23, 2024 · 2 comments · Fixed by #386
Closed

bug: PrimitiveLiteral and Literal should not be Ord. #378

liurenjie1024 opened this issue May 23, 2024 · 2 comments · Fixed by #386
Labels
bug Something isn't working
Milestone
0.3.0 Release

Comments

@liurenjie1024
Copy link
Contributor

In rust, Ord means total order, while PartiarOrd mean partital order. Currently we have derived Ord for both PrimitiveLiteral and Literal, which is incorrect. How could we compare Struct with Map, or Date with String? We should do following changes:

  1. Remove PartialOrd, Ord for both Literal.
  2. Remove Ord for PrimitiveLiteral. I think we should also remove PartialOrd for PrimitiveLiteral, which misses type information for decimal.
  3. Implement PartialOrd for Datum.
@liurenjie1024 liurenjie1024 added the bug Something isn't working label May 23, 2024
@liurenjie1024 liurenjie1024 added this to the 0.3.0 Release milestone May 23, 2024
@liurenjie1024
Copy link
Contributor Author

I think this is blocking prs with filter related, such as #363 and #367

@marvinlanhenke marvinlanhenke mentioned this issue May 23, 2024
Merged
@liurenjie1024
Copy link
Contributor Author

cc @sdd @marvinlanhenke @Xuanwo @Fokko

@ZENOTME ZENOTME mentioned this issue May 29, 2024
Merged
@liurenjie1024 liurenjie1024 mentioned this issue Jun 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
iceberg-rust
Status: Done
Milestone
0.3.0 Release
Development

Successfully merging a pull request may close this issue.

fix: make PrimitiveLiteral and Literal not be Ord ZENOTME/iceberg-rust
1 participant
@liurenjie1024