-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fast versions of isequal() and isless() for Nullables #18304
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
277c21f
Make isequal(::Nullable, ::Nullable) faster for safe types
nalimilan 4046cd9
Add isless(::Nullable, ::Nullable)::Bool
nalimilan 2660cb0
Add docstrings for isequal() and isless() on Nullables
nalimilan 4777f7a
Inline isequal() and isless() for Nullable
nalimilan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,16 +63,88 @@ get(x::Nullable) = x.isnull ? throw(NullException()) : x.value | |
|
||
isnull(x::Nullable) = x.isnull | ||
|
||
function isequal(x::Nullable, y::Nullable) | ||
if x.isnull && y.isnull | ||
return true | ||
elseif x.isnull || y.isnull | ||
return false | ||
|
||
## Operators | ||
|
||
""" | ||
null_safe_op(f::Any, ::Type, ::Type...)::Bool | ||
|
||
Returns whether an operation `f` can safely be applied to any value of the passed type(s). | ||
Returns `false` by default. | ||
|
||
Custom types should implement methods for some or all operations `f` when applicable: | ||
returning `true` means that the operation may be called on any bit pattern without | ||
throwing an error (though returning invalid or nonsensical results is not a problem). | ||
In particular, this means that the operation can be applied on the whole domain of the | ||
type *and on uninitialized objects*. As a general rule, these properties are only true for | ||
safe operations on `isbits` types. | ||
|
||
Types declared as safe can benefit from higher performance for operations on nullable: by | ||
always computing the result even for null values, a branch is avoided, which helps | ||
vectorization. | ||
""" | ||
null_safe_op(f::Any, ::Type, ::Type...) = false | ||
|
||
typealias NullSafeSignedInts Union{Int128, Int16, Int32, Int64, Int8} | ||
typealias NullSafeUnsignedInts Union{Bool, UInt128, UInt16, UInt32, UInt64, UInt8} | ||
typealias NullSafeInts Union{NullSafeSignedInts, NullSafeUnsignedInts} | ||
typealias NullSafeFloats Union{Float16, Float32, Float64} | ||
typealias NullSafeTypes Union{NullSafeInts, NullSafeFloats} | ||
|
||
null_safe_op{S<:NullSafeTypes, | ||
T<:NullSafeTypes}(::typeof(isequal), ::Type{S}, ::Type{T}) = true | ||
null_safe_op{S<:NullSafeTypes, | ||
T<:NullSafeTypes}(::typeof(isequal), ::Type{Complex{S}}, ::Type{Complex{T}}) = true | ||
null_safe_op{S<:NullSafeTypes, | ||
T<:NullSafeTypes}(::typeof(isequal), ::Type{Rational{S}}, ::Type{Rational{T}}) = true | ||
|
||
""" | ||
isequal(x::Nullable, y::Nullable) | ||
|
||
If neither `x` nor `y` is null, compare them according to their values | ||
(i.e. `isequal(get(x), get(y))`). Else, return `true` if both arguments are null, | ||
and `false` if one is null but not the other: nulls are considered equal. | ||
""" | ||
@inline function isequal{S,T}(x::Nullable{S}, y::Nullable{T}) | ||
if null_safe_op(isequal, S, T) | ||
(x.isnull & y.isnull) | (!x.isnull & !y.isnull & isequal(x.value, y.value)) | ||
else | ||
return isequal(x.value, y.value) | ||
(x.isnull & y.isnull) || (!x.isnull & !y.isnull && isequal(x.value, y.value)) | ||
end | ||
end | ||
|
||
isequal(x::Nullable{Union{}}, y::Nullable{Union{}}) = true | ||
isequal(x::Nullable{Union{}}, y::Nullable) = y.isnull | ||
isequal(x::Nullable, y::Nullable{Union{}}) = x.isnull | ||
|
||
null_safe_op{S<:NullSafeTypes, | ||
T<:NullSafeTypes}(::typeof(isless), ::Type{S}, ::Type{T}) = true | ||
null_safe_op{S<:NullSafeTypes, | ||
T<:NullSafeTypes}(::typeof(isless), ::Type{Complex{S}}, ::Type{Complex{T}}) = true | ||
null_safe_op{S<:NullSafeTypes, | ||
T<:NullSafeTypes}(::typeof(isless), ::Type{Rational{S}}, ::Type{Rational{T}}) = true | ||
|
||
""" | ||
isless(x::Nullable, y::Nullable) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was this elsewhere in helpdb or the rst? should it be added to the rst if not? edit: likewise with |
||
|
||
If neither `x` nor `y` is null, compare them according to their values | ||
(i.e. `isless(get(x), get(y))`). Else, return `true` if only `y` is null, and `false` | ||
otherwise: nulls are always considered greater than non-nulls, but not greater than | ||
another null. | ||
""" | ||
@inline function isless{S,T}(x::Nullable{S}, y::Nullable{T}) | ||
# NULL values are sorted last | ||
if null_safe_op(isless, S, T) | ||
(!x.isnull & y.isnull) | (!x.isnull & !y.isnull & isless(x.value, y.value)) | ||
else | ||
(!x.isnull & y.isnull) || (!x.isnull & !y.isnull && isless(x.value, y.value)) | ||
end | ||
end | ||
|
||
isless(x::Nullable{Union{}}, y::Nullable{Union{}}) = false | ||
isless(x::Nullable{Union{}}, y::Nullable) = false | ||
isless(x::Nullable, y::Nullable{Union{}}) = !x.isnull | ||
|
||
==(x::Nullable, y::Nullable) = throw(NullException()) | ||
|
||
const nullablehash_seed = UInt === UInt64 ? 0x932e0143e51d0171 : 0xe51d0171 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also add
Rational{T}
andComplex{T}
ifT <: NullSafeTypes
. If you are interested in vectorization, then respectiveTuple
andVecElement
types would also be useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I can't yet define a
typealias
withT <: NullSafeTypes
, can I? So the best strategy is probably to definenull_safe_op(S<:NullSafeTypes,T<:NullSafeTypes}(::typeof(isequal), ::Type{Complex{S}}, ::Type{Complex{T}})
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As regards containers, I'm not sure it's a good idea to mark them as safe: this means
isequal
will be called even for nulls, and it potentially implies checking equality of all entries. So it will only be interesting for small tuples. Anyway, I think we should wait for a real use case combiningNullable
andTuple
/VecElement
before implementing this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a Holy trait here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null_safe_op
is basically a trait, but which applies to combinations of operations and types rather than only on types. Indeed, e.g.Int
is safe for+
but not for/
(division by0
error).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, my mistake. I thought we were dispatching on the Union but that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added
Complex
andRational
. Unfortunately, there's no way to also support combinations of e.g.NullSafeTypes
andRational{<:NullSafeTypes}
without defining one method for each combination. Since these cases are less unlikely to be common and performance-critical, I'll leave them out until we have a way to define these more easily (UnionAll
).`There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible;
though admittedly not quite as nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for comparing
Rational
toComplex
(probably a rare situation), but not for comparing e.g.Complex{Float64}
withFloat64
. I'd have to define several methods for each combination.