This ensures that we can't overflow when adding `pql.Decimal`s together. The
only place we can possibly overflow is when converting pql.Decimal to an Int64,
but that is a risk we have to take. Also, the only place we do this is in our
ToRowser. We could maybe change that to strings, so the presentation of data
doesn't indicate an overflow, but that is a later decision to make. It will
also involve fixing the generate-proto-grpc make command, because that's broken
rn.
This way we can avoid annoying floating point rounding errors.
Check out FB-1359 for an example:
```
--- FAIL: TestExecutor_GroupByStrings (0.55s)
--- FAIL: TestExecutor_GroupByStrings/3 (0.00s)
executor_test.go:5433: unexpected result at 0:
got:{Group:[generals.1.r1] Count:5 Agg:2775
DecimalAgg:27.749999999999996}
want:{Group:[generals.1.r1] Count:5 Agg:2775 DecimalAgg:27.75}
```
* ugly first cut at supportings Rows(in=[...])
need tests, better handling of various combinations of arguments and
error cases
* explicitly error when other arguments passed with 'in' to Rows
* first cut at supporting Rows(in=[...])
'in' is explicitly not supported with any other arguments (except the
field of course), and will error. It works both as a standalone Rows
call and in GroupBy.
* bitmapfilter require ordered rowids
* remove log message
Co-authored-by: Todd Gruben <todd@molecula.com>
* add bsi base back to int value
* test bsi base/min/max for IntFields
motivated by bsi base not being added back to values
in extract calls when min was a positive integer.
we have an issue where nodes get into UNKNOWN state rather frequently
during periods of high load when they aren't actually down. We want to
allow queries in this situation rather than giving a "shard
unavailable" message.
We default to the jmp-hash method which we had previously, and allow a
user to set the "modulus" option which uses a simple mod operation to
ensure an even spread of partitions across nodes.
I think that ideally we would have new indexes uses modulus and
existing indexes use jmp-hash which implies supporting this
configuration on a per-index basis.
If we don't do per index, we should probably run the whole test suite
both ways.
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.
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.
Check if queries that have a 'like' argument are applied to keyed
fields. If not, log that the user is trying to use 'like' on an
unsupported field type (as opposed to reporting that there
are no results.)
When deletion is started, _exists field is updated with row+1.
After deletion is completed, we delete _exists=row+1.
If _exists>=1, then deletion was not completed.
Updated go version in docker to match other requirements.
Removed duplicate error check for grpc.
There's an obvious bug, plus another bug that I hit trying to reproduce
the first bug, plus another... it's a long story.
Basically: If you get nothing back from executeDistinctShardBSI on a
Timestamp field, the request for a large enough pool of strings to hold
timestamp conversions of the nothing segfaults because r.Columns() on
a nil row segfaults.
To try to test this better, I added a filter to the executor test that
we use for this case, which got me a different result complaining about
a DistinctTimestamp result not being a SignedRow.
So, there's a couple of issues. One is that, in the case where a filter
is present, if the filter comes up with nothing, we can bail early
and return a result of the SignedRow type, which then breaks the reduce
part of our map/reduce when we try to reduce DistinctTimestamp values
into a SignedRow. To fix this, we make sure that we return the expected
type even in the case where we're bailing early.
A simpler way to see the actual original bug is, rather than having
a filter, just have a shard that has a value in *some other field*
but not in the timestamp field. So we add that to the test, too.
But also, really, since this is a problem that's happened more than
once, I propose that we also just make nil rows allow you to request
their columns and get back nil, so things like this don't bite us as
much. This wouldn't be a sufficient fix for the filter case, and I
still have the short-circuit for the nil row case explicitly in this
particular case because relying on the nil behavior bugs me, but I
think it's safer to allow .Columns on nil rows.
This adopts the task pool functionality to let us spawn new worker
threads when worker threads are blocked. The underlying reason for
this is the same as the reason for the previous worker-pool-growing
strategy; while our design persistently has at least one thing which
can proceed, it can be the case that there are N things blocked,
where N is the size of our worker pool. Blocked workers shouldn't
count against our desired number of workers.
Originally, the intent was to thread this into RBF, and provide
backpressure from RBF on the pool when blocking on writes. Unfortunately,
that's not good enough, because while a write is blocked, the Qcx
calling it is *also* holding the Qcx's mutex, which means that any other
NewTx on that Qcx will *also* block. So we need to block for the
entire time of the NewTx.
Removing the existing worker spawning code resulted in a subtle
and maybe-harmless change; prior to this, each invocation of `mapperLocal`
would hold a lock, which meant that all the tasks for a given local mapper
would be put in the queue *sequentially*, ensuring that they'd all be
picked up by workers before things from later workers.
With the new pushback, that's not, strictly, necessary. Also, if you
disable it, you can end up with 300,000 goroutines at once, most of them
blocked.
A smallish run does, in fact, eventually complete anyway -- it will
indeed keep making workers until everything gets one. However, while
it's *correct*, it's also noticably *slower*. The same test workload
goes from around 33 seconds to a bit over 40 seconds when that lock
isn't present. (But that's with an extremely small WAL write cap
introduced to make the previous deadlock possible.)
With large numbers of shards, the practical impact is that you can
have quite a lot of things in process, with hundreds of goroutines
each, all blocked waiting for one writer. If we force them to all be
processed at the same time, all the reads that are connected to
each other are much more likely to get all processed at once, before
something new comes along.
In short, that lock isn't strictly necessary but it seems to help
noticably with performance and reduce simultaneous goroutines
significantly.
if we have more than twice our starting worker pool, and have had no
tasks when checking the queue for multiple rounds, send a job telling
the system to retire a worker. eventually we'll get down to about 2x
the starting pool size if we stay idle.
