add clear functional option for imports#1699
add clear functional option for imports#1699travisturner merged 7 commits intoFeatureBaseDB:masterfrom
clear functional option for imports#1699Conversation
travisturner
force-pushed
the
import-clear
branch
from
bb70794 to
d01372b
Compare
api.go
Outdated
| } | ||
|
|
||
| // Set up import options. | ||
| options := &ImportOptions{} |
There was a problem hiding this comment.
This block should be moved to a function, the same block is used at lines 704 and 776 and client.go 582.
There was a problem hiding this comment.
I added a function to consolidate these. I can't share the function with http/client.go without exporting it (because it's a separate package). I don't think it's worth exporting for that one instance.
api.go
Outdated
| Clear bool | ||
| } | ||
|
|
||
| // ImportOption is a functional option type for API.Import |
There was a problem hiding this comment.
Dot is missing at the end of the comment
http/client_test.go
Outdated
| t.Fatalf("unexpected values: got sum=%v, count=%v; expected sum=30, cnt=2", sum, cnt) | ||
| } | ||
|
|
||
| // Verify Range |
There was a problem hiding this comment.
Dot at the end of the comment is missing.
http/client.go
Outdated
|
|
||
| url := fmt.Sprintf("%s/index/%s/field/%s/import-roaring/%d?remote=%v", uri, index, field, shard, remote) | ||
| if options.Clear { | ||
| url += "&clear=true" |
There was a problem hiding this comment.
I think it's better to use url.Values to create the query part of the URL instead of string concatenation. Something like:
u := fmt.Sprintf("%s/index/%s/field/%s/import-roaring/%d", uri, index, field, shard)
vs := url.Values{}
vs.Set("remote", strconv.FormatBool(remote))
if options.Clear{
vs.Set("clear", "true")
}
u = fmt.Sprintf("%s?%s", u, vs.Encode())
| count = row.intersectionCount(filter) | ||
| } else { | ||
| count = row.Count() | ||
| consider = consider.Intersect(filter) |
There was a problem hiding this comment.
Isn't intersectionCount faster than separate Intersect and Count?
There was a problem hiding this comment.
Yes, but this change is necessary because of a bug with this sum() method. It wasn't taking the not-null bitmap (row(bitDepth)) into consideration when calculating sum. So if not-null was set to 0, but the BSI rows still contained a non-zero value, that value would be included in the sum. Basically, we need consider later in the method.
With all that said, there may be a way to leverage instersectCount() that I'm missing. Can you suggest a different approach?
There was a problem hiding this comment.
No, that makes sense.
travisturner
force-pushed
the
import-clear
branch
from
b0029dd to
f02605a
Compare
yuce
left a comment
yuce
left a comment
There was a problem hiding this comment.
Roaring import doesn't support the clear flag. Is that intentional?
|
I implemented it at the API. Looks like it needs to be added to the http handler. |
4 participants
Overview
This PR adds functional options to all exposed Import-related api methods. Providing
OptImportOptionsClear(true)to anImport()method will treat the payload as clear-bits.Fixes #1657
TODO:
clearValueforintfields. ThesetValueimport requires acolumn,value, but forclearValueit doesn't really make sense to require the value for clears. In that case you just need thecolumn. Is that a reasonable way to handle it?ImportRoaringto support clearPull request checklist
Code review checklist
This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.