-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
Delegates `Value.Hash` retrieval to `SimpleRegexTrie` for pattern matching. All operations involving field patterns must be translated to concrete field names.
The bulk of the effort went into |
Also, |
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, nice, so we're using the trie for pattern matching only, not for storing the actual values. I was expecting this to require a larger change, but this looks very good 👍
Is my understanding correct that right now, we're creating a new trie with a single value for every lookup? I think we could make the trie a member of Hash
to avoid multiple instantiations.
I think the wrong order in the disabled tests was from a previous state, where we were not using LinkedHashMap
for the record map.
Yes, your understanding is correct - and yes, we might be able to make it a member. I had a (hypothetical) case where that would've failed, but that might've been before I started checking for the actual pattern ( |
Cannot lead to false positives since the presence of the actual pattern is verified.
Alright, I've changed the trie to a member (432ef88). Should be fine™ ;) |
FTR, we can't use |
…207, #97) `SimpleRegexTrie` is only used to determine whether a field name matches a pattern. The result of this computation only depends on these two values, not on the underlying data. So we can cache and reuse the result across the whole transformation process. It might be possible to implement the matching in a more optimized way for this particular use case, but it's delegated to `SimpleRegexTrie` for compatibility and maintainability purposes.
Delegates
Value.Hash
retrieval toSimpleRegexTrie
for pattern matching.All operations involving field patterns must be translated to concrete field names.
NOTE: Requires (a lot) more tests. But existing tests, including previously disabled ones, are already providing some coverage - with trivial (?) modifications to make them pass (not sure why they were testing the wrong order).
Resolves #89.