Add IMPORT and EXPORT commands to similarity index.
#5049
Conversation
Generated by 🚫 danger |
tkaemming
force-pushed
the
similarity-import-export
branch
from
a71340b to
f41895e
Compare
|
|
|
(This is done pending any review feedback.) |
JTCunning
left a comment
JTCunning
left a comment
There was a problem hiding this comment.
Python bits look good. Deferring to @LewisJEllis for the actual approval.
setup.py
Outdated
| 'cqlsh', | ||
| # /cassandra | ||
| 'datadog', | ||
| 'msgpack-python', |
There was a problem hiding this comment.
I'd pin this for <0.5.0.
LewisJEllis
left a comment
LewisJEllis
left a comment
There was a problem hiding this comment.
Looks good, comments are just minor tweaks/curiosity.
| local entries = build_variadic_argument_parser({ | ||
| {'index', identity}, | ||
| {'key', identity}, | ||
| {'data', function (value) |
There was a problem hiding this comment.
just pass cmsgpack.unpack here? shouldn't be like in JS where if you do that you mess up on this binding (but maybe i'm wrong)
There was a problem hiding this comment.
Good point, not sure why I didn't think of this earlier.
| end | ||
| local results = {} | ||
| for i = 1, #arguments, #fields do | ||
| local value, _ = parser(table.slice(arguments, i, i + #fields - 1), i) |
There was a problem hiding this comment.
this use of slice in a loop makes me wish it was an in-place slice of some sort instead of copying every time...but these operations are already pretty heavy on IO and this doesn't affect the complexity overall
There was a problem hiding this comment.
I agree, I might look at making a slice iterator real quick.
There was a problem hiding this comment.
Going to kick this one down the road for a bit because I don't think it's going to make a significant performance improvement here, but I'd like to move a lot of the functional stuff to iterators in the future for improved memory utilization so I will probably tackle it then.
| local response = redis.call( | ||
| 'HGETALL', | ||
| source_bucket_frequency_key | ||
| local response = redis_hgetall_response_to_table( |
There was a problem hiding this comment.
I really like how this helper func cleans up that repeated loop pattern and lets variable assignment happen in the for loop w/pairs, good stuff
There was a problem hiding this comment.
Yeah, that weird sequence-as-mapping response was driving me nuts.
| -- The destination bucket frequency key may have not | ||
| -- existed previously, so we need to make sure we set | ||
| -- the expiration on it in case it is new. | ||
| if touched then |
There was a problem hiding this comment.
can we eliminate the touched flag and just do if next(buckets) == nil then here? PIL on next func
There was a problem hiding this comment.
Neat trick, will change.
| --[[ | ||
| Exports data that is located at the provided ``index`` and ``key`` pairs. | ||
| Generally, this data should be treated as opaque method for extracting |
There was a problem hiding this comment.
I kind of wonder why this doesn't just return a single big msgpack blob? Would be theoretically more portable/hands-off for anything passing it around, but your PR comment mentions introspection use cases that I'm guessing motivate doing it this way?
There was a problem hiding this comment.
This way each input pair is associated with a single result. This is consistent with the responses from other commands, and also makes it so that if you do EXPORT A B and then need to IMPORT A and IMPORT B into different scopes that is possible without having to unpack the return value.
There was a problem hiding this comment.
Okay, makes sense. I'll trust that that hypothetical use is some realistic possibility.
| {'key', identity}, | ||
| {'data', function (value) | ||
| return cmsgpack.unpack(value) | ||
| end} |
There was a problem hiding this comment.
consistency nit: some spots have trailing comma in array-like-tables, some spots don't...i think this is the outlier in not having it, not sure if you've made a conscious decision one way or the other
There was a problem hiding this comment.
I probably just forgot to do that here. I've carried over from Python my habit of adding a trailing comma to sequence data structures. I'll fix this for consistency, but is there any sort of general Lua style guide that would apply here?
There was a problem hiding this comment.
I tend to follow this (but not strictly) and it roughly says "trailing comma is okay but discouraged"; I think I agree that the trailing comma is less common but I'm totally fine with it myself. If we want to decide to follow it we can, but I think your Lua style is generally fine and the consistent trailing comma wfm.
| ) | ||
| end | ||
| ) | ||
| end |
There was a problem hiding this comment.
writin some lisp over here except someone macro'd end to be another paren token
| ] | ||
|
|
||
| for idx, key in items: | ||
| arguments.extend([idx, key]) |
There was a problem hiding this comment.
could just do arguments.extend(itertools.chain(*items)) instead of the for loop, not sure which you prefer; the for loop is certainly more self-explanatory but i know you like itertools
There was a problem hiding this comment.
Yeah, this is entirely for readability since I didn't actually write documentation or type annotations for any of this.
| arguments, | ||
| ) | ||
|
|
||
| def import_(self, scope, items, timestamp=None): |
This is basically a combination of several different independent changes:
{index, key}parsing for commands. I'd like to find an even more generic way to split out this pattern for handling different structures like{index, key, data}, but haven't figured out a clean way to do this yet. (Stay tuned!)HGETALLresponses return a mapping-style table rather than an list-style table.IMPORTandEXPORTcommands for extracting and loading group data from the database, as well asMinHashIndexmethods for calling them. This allows for moving data across scopes, as well as provides a method for performing basic introspection of the data contained in the database.#nochanges