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

Initial attempt at supporting collections of arbitrary types as keys #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rofinn
Copy link
Collaborator

@rofinn rofinn commented Feb 28, 2018

  • Keys can now be an AbstractString or Vector{T}.
  • Needed to adjust the parameterization of the Trie type and some of its constructors.
  • Added some tests for Vector{Symbol} and Vector{UInt8} keys.

@oxinabox
Copy link
Member

oxinabox commented Mar 1, 2018

How about the following, append_key
so keys can be any iterable.

julia> append_key(prefix::K, x) where K = K([collect(prefix); x])
append_key (generic function with 1 method)

julia> append_key("abc",'c')
"abcc"

julia> append_key([1,2,3],2)
4-element Array{Int64,1}:
 1
 2
 3
 2

@rofinn
Copy link
Collaborator Author

rofinn commented Mar 1, 2018

That would work, but it results in extra allocations in the string condition.

julia> using BenchmarkTools, Compat

julia> append_key1(prefix::AbstractString, x::Char) = string(prefix, x)
append_key1 (generic function with 2 methods)

julia> append_key2(prefix::K, x) where K = K([collect(prefix); x])
append_key2 (generic function with 1 method)

julia> prefix = "abc"; x = 'c'
'c': ASCII/Unicode U+0063 (category Ll: Letter, lowercase)

julia> @btime append_key1($prefix, $x)
  35.271 ns (1 allocation: 32 bytes)
"abcc"

julia> @btime append_key2($prefix, $x)
  10.688 μs (48 allocations: 1.80 KiB)
"abcc"

NOTE: Did a similar check for arrays and it's about the same between the two implementations.

Maybe it would make sense to special case strings for now and make that the fallback?

@oxinabox
Copy link
Member

oxinabox commented Mar 1, 2018

Seems reasonable

- You can use Tuples as keys (which are pretty fast)
- Better fallback `append_key` method w/ special cases for `String` and `Tuple`
@oxinabox
Copy link
Member

oxinabox commented Mar 3, 2018

To be clear
for the usage I proposed in andrewcooke/ParserCombinator.jl#20
it needs to support arbitrary iterators as keys (not just tuples, strings and vectors).
At least for look-up (if not for set) purposes.

@rofinn
Copy link
Collaborator Author

rofinn commented Mar 5, 2018

Okay, I guess since julia doesn't enforce the iterator interface we could just not have a type requirement on the key. Do you have a minimal example of the type of iterators you're working with?

@oxinabox
Copy link
Member

oxinabox commented Mar 6, 2018

I don't have a minimal example, because it is explicitly iterator.
For an example maybe use zip

@rofinn
Copy link
Collaborator Author

rofinn commented Mar 6, 2018

Okay, I don't really know how to make this work for arbitrary iterators easily because the supported constructor syntax seems to vary significantly by iterator. With the existing architecture we require that we can constructor the iterator type w/ 1) an empty constructor and 2) that splats the existing iterable with a new element.

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