Add CrateDB database support to the time series benchmark suite. #71
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #71 +/- ##
==========================================
- Coverage 66.03% 64.96% -1.07%
==========================================
Files 77 84 +7
Lines 3819 4282 +463
==========================================
+ Hits 2522 2782 +260
- Misses 1266 1463 +197
- Partials 31 37 +6
Continue to review full report at Codecov.
|
|
Hi, thanks for doing this! We'll take a look and should have a review shortly. Apologies for the delay |
cmd/tsbs_load_cratedb/creator.go
Outdated
| sql := fmt.Sprintf(` | ||
| CREATE TABLE %s ( | ||
| tags object as (%s), | ||
| ts timestamptz, |
There was a problem hiding this comment.
This does not appear to work with crate 3.3.2, I get:
SQLParseException: Cannot find data type: timestamptz (SQLSTATE XX000)
There was a problem hiding this comment.
Thank you for reviewing the PR!
I've been using a nightly build for development that contains some new date time types and aliases. I've pushed a fixup, i think in this case it is ok to use the timestamp data type, it has the time with time zone semantic and will remain as it is for a couple of future releases.
README.md
Outdated
| @@ -101,7 +102,9 @@ Variables needed: | |||
| 1. a start time for the data's timestamps. E.g., `2016-01-01T00:00:00Z` | |||
| 1. an end time. E.g., `2016-01-04T00:00:00Z` | |||
| 1. how much time should be between each reading per device, in seconds. E.g., `10s` | |||
| 1. and which database(s) you want to generate for. E.g., `timescaledb` (choose from `cassandra`, `clickhouse`, `influx`, `mongo`, `siridb` or `timescaledb`) | |||
| 1. and which database(s) you want to generate for. E.g., `timescaledb` | |||
| (choose from `cassandra`, `clickhouse`, `influx`, `mongo`, `siridb`, `cratedb` | |||
There was a problem hiding this comment.
cratedb should go after clickhouse (list is in alphabetical order)
a59672f to
fcfe4b5
Compare
|
@RobAtticus please take a look at the last |
|
Hi @RobAtticus, is there anything from my side that is blocking this PR from being merged? |
|
Sorry @kovrus , we've been having an off-site this week so things have been a bit slow on our end preparing for it and this week. I'll hopefully have more time to look at it next week. As long as it's up to date rebased on master, it should be good from your end and I'll check ASAP. |
| for i, key := range p.tagKeys { | ||
| buf = append(buf, '"') | ||
| buf = append(buf, key...) | ||
| buf = append(buf, []byte{'"', ':', '"'}...) |
There was a problem hiding this comment.
this might be easier read as:
buf = append(buf, []byte("\":\"")...)
same with the two lines down
| @@ -0,0 +1,76 @@ | |||
| package utils | |||
There was a problem hiding this comment.
this file was moved to internal/utils/, so you'll need to remove this and update imports
There was a problem hiding this comment.
slipped out of my sight, removing it
| cols []string | ||
| } | ||
|
|
||
| func (t *tableDef) fqn() string { |
There was a problem hiding this comment.
// fqn returns the fully-qualified name of a table
took me a bit to figure out what fqn stood for, so maybe a comment will help others 😄
There was a problem hiding this comment.
godoc comments start with the name of the function, so it should start with // fqn returns...
|
@RobAtticus thanks for your time and the review! 👍 I've addressed you comments. |
| cols []string | ||
| } | ||
|
|
||
| func (t *tableDef) fqn() string { |
There was a problem hiding this comment.
godoc comments start with the name of the function, so it should start with // fqn returns...
|
Thanks for submitting this btw, looks great and we're happy to add CrateDB! |
71740eb to
02630a3
Compare
This includes support for data generation and querying for the devops use case.
|
Merged in, thanks again for contributing this! |
|
@RobAtticus Thanks for the review! |
The PR adds the CrateDB database support to the time series benchmark suit. CrateDB supports the PostgreSQL wire protocol v3 so we use the pgx PostgreSQL driver to connect to the database.