The intent of these lines was "if there's no filter, return
immediately rather than doing operations".
But actually we didn't write that, so we were calling intersectionCallback
on empty filters, which didn't matter since it failed out quickly, but
it's still a waste of effort.
Except we shouldn't get to these anyway because ConsiderKey already
correctly rejected these cases. I think. But still.
In fact, we *do* want to skip ahead sometimes to the next thing, and
only call our callback for things that match our filter. I was thinking
that we needed to call the callback for all data regardless, because
what if you're writing to a mutex and adding new data.
But even if you're adding new data, it's still in the filter, because
it has to be, because we don't start out knowing there's no existing
data. So the mutex actually works fine.
So the rule for BitmapBitmapTrimmer is that your filter doesn't have
any meaning other than (1) it tells us which containers you need
to see, (2) we provide it to your callback function. Maybe you want
to subtract those. Maybe you want to add them. That's up to you to
decide.
Now that we have ApplyRewriter, it's a viable way to implement ImportMutex.
It can be slower on low-density writes, because it's checking more things
than it otherwise might -- the other filter form can skip ahead and only
check the containers it's modfying, in principle, while this one doesn't
know it can do that. (The decision as to how far to skip ahead has to
be made in the BitmapBitmapTrimmer, while it's the callback provided to
it that knows when it next has data to write.)
On the other hand, it's probably faster in some cases, and would be
more-faster if we could improve the cursor management a bit, and it's
skipping at least some seeking because it doesn't need to use
ImportPositions after reading the whole thing.
This uses the shiny new ApplyRewriter logic for ClearRecords,
mostly to verify that ApplyRewriter works at all.
This also implies separating the cache update code out from
importPositions so it can be used also by this.
We also use fragment.ClearRecords instead of the different clearFragment
code in executor. The clearFragment implementation did not update TopN
caches and the like. Standardize it on the clearRecords implementation
which does.
We don't need to manually copy each individual item and keep
checking for the second index being out of bounds for every
item, we know it can't change at this point, so we can just bump
it over. We want this operation because BitmapRewriter can
use it to simplify trimming in some cases.
The filter and rewrite logic are unlocking and relocking but I don't
think they should. I think those locks were added early on during
testing of the filter stuff, but I don't think they should be needed,
and I've been unable to find a case where they were. I think probably
I had something where a ConsiderData function was trying to run a Tx.
This in a parallel to ApplyFilter/BitmapFilter which allows writebacks
while it's running. It's a write operation, so it needs a write lock
on the Tx, and needs to create bitmaps if they don't already exist.
The semantics are a bit messy and need better documentation still.
We frequently want to grab the set of values from a []uint64
that correspond to a given key, and make a container from them, but
sometimes we only want to do one of these. This implementation
lets us do that the same way every time, and do in-place
container creation without extra allocs.
The test for bitmap-to-array succeeding doesn't work with roaringParanoia,
which *does* intentionally panic at that point. Possibly we should also
drop the corresponding logic that tries to prevent it from panicing,
since it won't work with the paranoia flag on anyway.
Follows the nextLink in http response to iterate through
paginated group membership response in order to obtain all
groups that the user is a member of.
Also, checks cache to make sure we don't add empty groups
to the cache.
Because the ToRowser interface was not implemented for DistinctTimestamp, there was
a error when using the GRPC endpoint to call Distinct(All(), field=ts). Implementing
the ToRowser interface for DistinctTimestamp solves that problem.
Related to SUP-210: WebUI, Python - Distinct() does not work for Timestamp field
If you're wondering how something that simple gets a commit
message this long, sit down, because you are in for a ride.
The Row, Rows, TopK, and GroupBy(Rows...) commands had three
different sets of semantics for from/to ranges. We unify these.
Sounds easy, right?
The original purpose of this was to address a bug in GroupBy
where, if you had multiple queries only one of which used time,
we could end up silently returning no results because we tried to
do a time query against a non-time field. This was easy to
fix; just move a boolean flag from outside a loop to inside
the loop so it resets to false on each pass.
In the process of trying to test that, I discovered that
specifying `from=...` without `to=...` in a Rows in a GroupBy
didn't work. Searching around, I discovered that we had three
different answers:
GroupBy, TopK: unspecified 'to=' is 0
Row: unspecified to is tomorrow
Rows: unspecified to is the max time quantum in the field
(A time value of 0 is apparently interpreted as January 1st,
0001.) Note that "GroupBy" is really referring to a Rows()
command in a GroupBy, it's just that this uses completely different
code (because it has to be computing rows potentially matching or
restricted to a filter, or provide the rows it generated so
they can be used to filter something else).
So we fixed that, and made a field method for finding the min/max
values (as done in a Rows command that *isn't* in a GroupBy),
and tried to use that with viewsByTimeRange. Then I tried to write
documentation for this, but the documentation was unclear, and
I tried to clear it up. Which caused me to discover that these
four different places ALSO differed in when or whether they'd
replace a broad query with "just the standard view".
So. Round two of the fix: We create a `field.viewsByTimeRange`,
which tries to fall back to a standard view when one exists
and the specified range covers everything, and treats zero
values as non-restrictive, but also picks a narrow range that
is actually related to the range of dates in the field. This
matters because viewsByTimeRange generates the entire set of
views it would need *even if those views don't exist*.
We drop one test that was testing Rows specifically to verify
that, if you omitted To, we acted as though you'd specified a date
two days in the future. That behavior is not now intended, so
we drop the test that tries to verify it.
Thing that might make this better: Figuring out a way to generate the
list of views more cheaply. Right now, we're redoing all the view
computation, including producing a sorted list of view names, for
every shard. This is excessive, but hard to fix.
In particular, there is no trivial way to generate a sorting such
that you can take slices of it and have them be the right slices,
because we want to skip smaller time quanta when an entire larger
parent quantum is included. e.g., if we're including all of
April 2022, we don't want to include any of the days for April of
2022, but if we're doing up through April 15th, we want to include
the first 15 days of April, but NOT include the whole-month quantum.
And so on. Fixing this cleanly is hard and would require a
significant design effort.
When you backup a cluster, we call /schema which marshals timestamp field options to
json, and restores the fields with those options. If the min and max are missing,
they are set to 0 on restore, which causes an issue on subsequent ingest.
Fixes [SUP-213](https://molecula.atlassian.net/browse/SUP-213) and
[FB-1332](https://molecula.atlassian.net/browse/FB-1332)
During backups on a multi-node cluster, TranslateData was unconditionally redirecting to the primary, regardless of the primary’s status. This is less than ideal. If the primary is down, the backup will fail.
As a consequence of this fix, we will also no longer needlessly redirect to ourselves on a single node cluster. This is a great optimization win!!!!
Fixes # FB-1324 SUP-209
Co-authored-by: tgruben <tgruben@gmail.com>
The ingest API tried to do a Qcx operation that needs a write Tx after
requesting a write Tx. This doesn't work. The ingest API is the only caller
of clearExistenceColumns, so it's easy to just make it work for a given
shard using a provided Tx. This isn't especially clean, but a lot of this
is due for an overhaul anyway because the Qcx/Tx stuff is sort of broken.
Also, add any test case at all for this, since we didn't have one, and
also fix the fact that the test case failed because the test harness
didn't allow empty result sets.
This commit addresses a bug in https://molecula.atlassian.net/browse/SUP-200 where
the Authorization header was not being set correctly when the token was passed via
"userinfo" in the context and not "token".
Now, we prefix the token with "Bearer " when the token comes from userinfo.
This commit also adds a unit test for this function, and simplifies logic. It also
fixes a test that didn't quite test the behavior we wanted.
