* first time quantum queries working
* implement select from timequantum columns
* skip a dax test
* make linter happy
* addressed review feedback
* reverted over eager test elimination
* correct reference for `having count(*)`
It turns out that `having count(*) ...` was always treating
the count(*) as exactly 1. After studying this a lot, I noticed
that in fact, we correctly handle other counts. The reason is
that there's already code to recognize aggregates in `having`
clauses as matching aggregates that are being computed -- but
it only covers the other aggregate clause types, not the newly
added `countStarPlanExpression` from making `count(*)` work even
if there's no `_id` field.
We add several corresponding test cases.
* fix sum(a_decimal) type conversion
Added a test case for this, and also added a fix for it.
Underlying issue: qualifiedRefPlanExpression could end up
producing an int64 instead of a pql.Decimal, even though it
had expected type Decimal.
Originally this worked by politely converting an int64 to
a pql.Decimal in the Evaluate phase, but this was not ideal;
the real question is why it was coming out as an int64 at
that step. Showed this to Pat, who spent a while studying it
and produced a better fix.
* temporarily comment out test which fails in DAX
This forward-ports a number of tests from the previous SQL
implementation. The porting is approximate in a number of ways,
and not all tests are implemented/tested yet.
In particular, several tests are currently disabled because
we don't support `limit n` constructs.
The tests that were primarily tests of the parser have been
brought forward as parser tests. One of them has been altered
to add parentheses, because our parser interprets
fld1 between 1 and 3 and fld2 = 2
as:
fld1 between (1 and 3) and (fld2 = 2)
which is invalid, while the old parser apparently interpreted it
as:
(fld1 between 1 and 3) and (fld2 = 2)
We have not yet verified the SQL spec's requirements here, but
sqlite agrees with our old parser, not our new parser, so this
may be a regression.
The old tests expected an INNER JOIN to suppress duplicate
values. Our new code does not, which is consistent with other
SQL implementations. This is a change, but the old behavior
appears to have been wrong. (You can still suppress duplicate
values by specifying DISTINCT.)
In the previous implementations, a value like `count(*)` had
`count(*)` as its column name. In the new implementation,
it has an empty string as its column name.
Related to this, the prior implementation allowed you to
write
select age, count(*) from grouper group by age having count > 1
but the new implementationt requires that to be spelled as
having count(*) > 1
This is consistent with other SQL implementations, so I think
the new behavior is correct.
The behavior of SHOW COLUMNS and SHOW TABLES has changed, in
that the specific results returned are significantly different.
Perhaps more significantly, the old system spelled the former
query as SHOW FIELDS, rather than SHOW COLUMNS. This may be
considered a regression, in that `SHOW FIELDS` no longer works,
and we should consider whether any hypothetical users might
have been relying on the output of either of these. (I hope
not, the new output is much better.)
Some of the old tests (the ones in handler_test) were accommodated
by adding a couple of specific test cases to existing tests,
specifically:
* handling timestamp values with `Z` rather than `+00:00`
* a join with a WHERE clause referring to fields in both
source tables
We introduce a new "partial" comparison type, because there's
no way for a test of `SHOW TABLES` to contain a correct table
row, because `SHOW TABLES` includes timestamps from when tables
were created. I'm not sure this is the right way to do this.
We add corresponding changes to dax_test, because the DAX tree
tests against the SQL tests.
We change the returned types of field names and field types to
plain strings, ironically because DAX needs this -- the test code
in the DAX tree is getting them back as plain strings, rather
than as dax.FieldName and dax.BaseType.
The tests using `having` are commented out because they don't
seem to be working, a ticket has been filed for this.
Two of the tests that should return strings are instead returning
untranslated integer IDs, but only for DAX, not for the regular
SQL tests, and the `delete` test has been commented out for
DAX-specific errors. If we merge this, the next step is to
ticket those and address them separately.
While fixing a bug that log messages were ending up
in the output buffer for backups, we fixed up a bunch of
things to do with log messages and output for various
commands.
Due to a subtle oversight, this means that since we did
that, executor_test's `chkSumCluster` has been dutifully
printing `hash:blahblahblah` to os.Stdout, and returning
an empty string.
This also, indirectly, fixes a very strange behavior
we've had ever since then, which is that a lot of test
output silently disappears. The reason is probably,
although I haven't found the right code path, that we
were ending up closing os.Stdout.
The caching of test results means that we get instant cached results
rather than actually running tests in some cases, which is virtually
guaranteed not to be what we want. There's almost no cases where
this actually speeds a thing up validly, and a lot where it speeds a
thing up invalidly.
* WIP: json marshal coded errors for http
* WIP: trying to see how best to implement the http-error tests
* finish stubbing out the Schemar methods in the test
* using json to move CodedErrors across boundaries and associated tests
* implemented feedback and fixes
---------
Co-authored-by: Travis Turner <travis@molecula.com>
* Removed support for EPOCH column constraint from TIMESTAMP SQL data type.
* Implicit conversion of integers to timestamp will treat the integer value as seconds since unix epoch.
* Add new ToTimeStamp(num, timeunit) SQL scalar function to help convert integer values to timestamp.
* Add meta-commands \set and \unset (for variables)
* WIP: first pass at variable replacement
* Use a mapReplacer instead of having Command implement replacer
* remove circular reference with variables
* remove the `replacer` interface; just have it be a struct
* use a lexer for variable replacement
* Separate the featurebase and fbsql make build targets
Using the same build target was problematic because they shared the same
flags. Since the `-o` output flag was used, the fbsql binary was
overwriting the featurebase binary.
* make sure the make package target builds fbsql
we want to be sure we delete the files we created during the
run even if the test run panics, but not files other runs
may have created also in /mnt/ramdisk.
So, we were special-casing creating a ramdisk, and setting a special
environment variable for it, for boltdb translate files, to improve
performance.
But actually, etcd and test cluster data and so on all go in $TMPDIR,
and if you move all of those also into a ram disk, you get way better
performance. But 2GB may not be enough for that.
So!
We unify on $TMPDIR, we bump the default size to 4GB, we make the
size configurable, and we stop using the name RAMDISK. This should
improve performance on MacOS significantly for `make test` and
things like it, and also simplifies our lives by not having a special
case for the boltdb translate files.
Also change the environment variable names used in our CI config.
(I don't see where we mount the ramdisks, but I think that's happening
in our setup.)
* Store version-check file in the configured data-directory
This also fixes what I think is a bug.
It also un-exports everything.
I have questions.
* Version checking: clean up code, add server flags
FB-1975
Cleaned up version checking, removed a race condition, and added error checking.
Added server flags for check-in endpoint and UUID storage file.
Incorporates Travis's changes to store UUID file in data directory
and unexport most of verchk.go.
---------
Co-authored-by: Travis Turner <travis@molecula.com>
Co-authored-by: seebs <seebs@molecula.com>
When we close a task pool, we use a condition variable to wait for
workers to exit, if any workers are still running. The workers,
in turn, use the condition variable to notify that they've exited.
Unfortunately, the workers aren't using the lock (the rationale was
that it's all atomic ops so they don't need to), which means that
it's possible to have the following sequence:
Close(): obtain current live count
worker: decrement live count
worker: send broadcast to condition variable
Close(): wait on condition variable
To resolve this, we make the worker update also request the lock.
We add a simple reproducer for this. Note that simple doesn't mean
it fires completely reliably; on my laptop, the test causes a test
timeout about 60% of the time without the fix. If you add a short
delay between sampling the live count and waiting on the condition
variable, the deadlocks move from "60% chance of hitting it in
a million trials" to "nearly always".
* Add viper (for env variable) support to CLI
* Move the "featurebase cli" sub-command to its own "fbsql" command
I don't know if this is the final name, but putting it here as a
placeholder for now.
* Handle single `--command` flags.
This also adds a printer interface so we can opt NOT to print setup
information in non-interactive mode.
* Add support for multiple `--command` flags in the same call
* Add support for multiple `--file` flags
* Move members related to Config into a separate struct
* Make sure non-interactive mode can connect to a database
* comment fix
* support control-C on readline
* Prevent connection message from printing in non-interactive mode
* Return errors (instead of printing them) in non-interactive mode
* make nodeid come from the correct table
* refactored aggregates; added ability to aggregate on expressions not just references
* addressed feedback
* now with the compiler errors fixed after rebase
If we don't set an epoch, we get a cryptic message on the console.
Note, this message isn't logged properly, it doesn't use the
logger, it uses the `log` package.
2023/02/09 11:07:23 ERROR: converting timestamp options for
end_time: checking overflow: custom epoch too far from
Unix epoch: 0001-01-01 00:00:00 +0000 UTC
Because this uses the log package, it doesn't go to the same place
as other messages, making it a pain to debug.
The underlying problem is that a timestamp can't just have a zero
value for its epoch. So, we set a default epoch of 0 Unix Time.
We should possibly revisit the question of whether the conversion
in the top-level schema.go should handle an epoch which IsZero,
but I'm not sure what "base" should be in that case. In practice,
all existing usages except this one are specifying time.Unix(0, 0)
already.
Note: We now skip a test because we can't pass it but fixing
it is presently beyond my understanding. In parser_test.go,
we skip
SELECT * FROM X INNER JOIN Y ON true INNER JOIN Z ON false
because if we stringify it, we put the "ON true" in the wrong
place, and end up with something we can't parse.
The main change here is modifying AssertParseStatement and
AssertStatementStringer (and the corresponding Expression
functions) to also verify that they can clone and round-trip,
and that we can walk expressions. This gives us a ton
more coverage of Clone and conversions to string, and caught
a number of subtle typos and missing type switch cases.
Some of the changes are mostly cosmetic, such as only
including optional words when stringifying expressions
or statements if those optional words were present originally,
as shown by the Pos value stored for those words.
We also drop a lot of trailing apostrophes from some of the
parse test cases, which appear to be harmless but won't be
reproduced when converting back to strings.
We also add a number of additional test cases, or add
clauses to existing test cases, to improve coverage of a
lot of error testing. For instance, we added a decimal
field to the tests of show table, and added cases
using KEYPARTITIONS. (Although it doesn't *do* anything.)
Similarly, whenever we create a statement, we check
the behavior of requesting a list of sources from it, to
verify that source finding code at least runs.
