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

Fix scalar eq for booleans #2168

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Fix scalar eq for booleans #2168

merged 1 commit into from
Jan 31, 2025

Conversation

gatesn
Copy link
Contributor

@gatesn gatesn commented Jan 31, 2025

And derive partial eq where we can

@robert3005
Copy link
Member

What was broken here? Hard to figure out from the diff

@joseph-isaacs
Copy link
Member

We want scalar to only be comparable to other scalars of the same dtype

@robert3005
Copy link
Member

I thought top level scalar did the cast. But I guess you can still get a typed view

@gatesn
Copy link
Contributor Author

gatesn commented Jan 31, 2025

This check makes sure they have the exact same nullability

@gatesn
Copy link
Contributor Author

gatesn commented Jan 31, 2025

This is raising some bigger questions:

  • When we convert scalars from DataFusion, we do a really bad thing... which is return Scalar::Null for null literals...
    .unwrap_or_else(|| Scalar::null(DType::Null))
  • Vortex has chosen to never implicitly cast... but should we relax this rule for nullability only? If we do, then our conversion from datafusion should be as tight as possible and then upcast where necessary.

@gatesn gatesn merged commit 46c770b into develop Jan 31, 2025
16 checks passed
@gatesn gatesn deleted the ngates/scalar-eq branch January 31, 2025 14:24
@robert3005
Copy link
Member

I think we can fix up the conversion to be scalar null of dtype?

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.

3 participants