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.

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

refactor(types): Do not derive e.g. Ord on ScalarImpl #5872

Closed
jon-chuang opened this issue Oct 17, 2022 · 3 comments · Fixed by #5881
Closed

refactor(types): Do not derive e.g. Ord on ScalarImpl #5872

jon-chuang opened this issue Oct 17, 2022 · 3 comments · Fixed by #5881
Assignees

Comments

@jon-chuang
Copy link
Contributor

jon-chuang commented Oct 17, 2022

    Turns out the actual issue is due to `#[derive(PartialEq)]` on `ScalarImpl`. Its actually quite a bad thing to have as there should not be any meaningful notion of PartialEq on ScalarImpl. 

Originally posted by @jon-chuang in #5771 (comment)

e.g. Int64() > Int32() is always true due to enum position rather than our actual comparison.

This doesn't seem like behaviour we would want.

If we want, we can define manually semantically correct PartialEq on ScalarImpl that agrees with our expression framework.

@github-actions github-actions bot added this to the release-0.1.14 milestone Oct 17, 2022
@BugenZhao
Copy link
Member

Agree. I guess what you mean is not to derive Ord. #4272 (comment)

BTW, could we borrow the idea of PartialOrd to return None when comparing different scalar types?

@jon-chuang
Copy link
Contributor Author

jon-chuang commented Oct 17, 2022

I guess what you mean is not to derive Ord. #4272 (comment)

Yes

BTW, could we borrow the idea of PartialOrd to return None when comparing different scalar types?

Indeed. Not sure if it would be useful, but that seems the most broad statement that is still correct.

@jon-chuang jon-chuang changed the title refactor(types): Do not derive e.g. PartialEq on ScalarImpl refactor(types): Do not derive e.g. Ord on ScalarImpl Oct 17, 2022
@BugenZhao
Copy link
Member

BugenZhao commented Oct 17, 2022

Actually, I think there should be rare cases to directly compare two datums in production: every comparison should be done by expressions (expect for the current implementation of dynamic filter). 🤔 So to avoid misusing it, maybe we can directly panic when comparing different scalar types.

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 a pull request may close this issue.

2 participants