Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fuzz tests for metric engine #3741

Closed
WenyXu opened this issue Apr 18, 2024 · 22 comments
Closed

Fuzz tests for metric engine #3741

WenyXu opened this issue Apr 18, 2024 · 22 comments

Comments

@WenyXu
Copy link
Member

WenyXu commented Apr 18, 2024

          @CookiePieWw Thanks! BTW, we introduced a new engine for metric scenarios(called metric engine). Would you like to add some fuzz tests for this new engine?

The metric engine's RFC: https://github.com/GreptimeTeam/greptimedb/blob/main/docs/rfcs/2023-07-10-metric-engine.md

The following tests show how we create tables in the metric engine:

CREATE TABLE phy (ts timestamp time index, val double) engine=metric with ("physical_metric_table" = "");
SHOW TABLES;
DESC TABLE phy;
CREATE TABLE t1 (ts timestamp time index, val double, host string primary key) engine = metric with ("on_physical_table" = "phy");
CREATE TABLE t2 (ts timestamp time index, job string primary key, val double) engine = metric with ("on_physical_table" = "phy");
SELECT table_catalog, table_schema, table_name, table_type, engine FROM information_schema.tables WHERE engine = 'metric' order by table_name;
DESC TABLE phy;
DESC TABLE t1;
DESC TABLE t2;
-- should be failed
-- SQLNESS REPLACE (region\s\d+\(\d+\,\s\d+\)) region
DROP TABLE phy;
-- metadata should be restored
DESC TABLE phy;
DROP TABLE t1;
DROP TABLE t2;
DROP TABLE phy;
-- create one with other primary keys
CREATE TABLE phy2 (ts timestamp time index, val double, abc string, def string, primary key (abc, def)) engine=metric with ("physical_metric_table" = "");
DESC TABLE phy2;
DROP TABLE phy2;

cc @waynexia

Originally posted by @WenyXu in #3732 (comment)

@WenyXu
Copy link
Member Author

WenyXu commented Apr 18, 2024

Thank you for being so interested! @CookiePieWw
For metric engines, there are two types of tables: the physical table and the logical table. However, we may only need to focus on the logical table for fuzz tests. i.e.,

Fuzz tests may not be easy; they always need to figure out the correct behavior of the database. Feel free to contact us if you have any questions.
cc @waynexia

@CookiePieWw
Copy link
Collaborator

CookiePieWw commented Apr 19, 2024

Thanks for your detailed introduction :) @WenyXu. From my perspective, the logical tables seem to be a view on the physical table with some overlapped columns and some newly added columns. I wonder if there are some common practices and requirements for both.

For overlapped columns, do we need to keep the data type, constraints and options? I've noticed the data type and constraints can be different in logical tables and physical tables.

For newly added columns, they are only allowed to be strings. The test example above take all the newly added columns as a primary key, so I wonder if it's necessary.

@WenyXu
Copy link
Member Author

WenyXu commented Apr 20, 2024

For newly added columns, they are only allowed to be strings. The test example above takes all the newly added columns as a primary key, so I wonder if it's necessary.

Yes, it's designed for the scenario to use GreptimeDB as a Prometheus backend.

For overlapped columns, do we need to keep the data type, constraints and options? I've noticed the data type and constraints can be different in logical tables and physical tables.

Sorry, I missed something. Could you provide an example to explain the difference?

@CookiePieWw
Copy link
Collaborator

CookiePieWw commented Apr 20, 2024

Sorry, I missed something. Could you provide an example to explain the difference?

As for the physical table, here I add some constraints for val:

CREATE TABLE phy (ts timestamp time index, val double not null default 0) engine=metric with ("physical_metric_table" = ""); 

Currently we can create a logical table with the val column but without constraints (or add more constraints), and also change the data type of it:

CREATE TABLE phy (ts timestamp time index, val int, host string, primary key (host)) engine=metric with ("on_physical_table" = "phy"); 

I wonder if its appropriate to create sql like this for fuzz test.

BTW, I found the database would crash once I create a metric table with 2 columns with the same name:

mysql> CREATE TABLE `phy`(ts timestamp time index, val double) engine=metric with ("physical_metric_table" = "");

mysql> CREATE TABLE t1(ts timestamp time index, val double, host text, host string) engine=metric with ("on_physical_tab
le" = "phy");
ERROR 1815 (HY000): Internal error: 1003

mysql> show tables;
ERROR 1815 (HY000): Internal error: 1003

Is it the intended behavior?

@WenyXu
Copy link
Member Author

WenyXu commented Apr 20, 2024

BTW, I found the database would crash once I create a metric table with 2 columns with the same name:

mysql> CREATE TABLE `phy`(ts timestamp time index, val double) engine=metric with ("physical_metric_table" = "");

mysql> CREATE TABLE t1(ts timestamp time index, val double, host text, host string) engine=metric with ("on_physical_tab
le" = "phy");
ERROR 1815 (HY000): Internal error: 1003

mysql> show tables;
ERROR 1815 (HY000): Internal error: 1003

Is it the intended behavior?

Good catch! It's a bug🥲

@WenyXu
Copy link
Member Author

WenyXu commented Apr 20, 2024

Sorry, I missed something. Could you provide an example to explain the difference?

As for the physical table, here I add some constraints for val:

CREATE TABLE phy (ts timestamp time index, val double not null default 0) engine=metric with ("physical_metric_table" = ""); 

Currently we can create a logical table with the val column but without constraints (or add more constraints), and also change the data type of it:

CREATE TABLE phy (ts timestamp time index, val int, host string, primary key (host)) engine=metric with ("on_physical_table" = "phy"); 

I wonder if its appropriate to create sql like this for fuzz test.

I got you. It's a bug; it will throw an error if users try to insert values into a logical table. Some validation needs to be added while creating the logical table(It will be tracked in another issue).

Reproduce steps:

  1. Create tables
CREATE TABLE `phy`(ts timestamp time index, val double) engine=metric with ("physical_metric_table" = "");

CREATE TABLE t1 (ts timestamp time index, val int, host string, primary key (host)) engine=metric with ("on_physical_table" = "phy");

CREATE TABLE t2 (ts timestamp time index, val string, host string, primary key (host)) engine=metric with ("on_physical_table" = "phy");
  1. Insert values
mysql> insert t2 (host,ts,val) values ("hi",1702433146000,"10");
ERROR 1815 (HY000): Invalid request to region 4398046511104(1024, 0), reason: column val expect type Float64(Float64Type), given: STRING(12)

mysql> insert t1 (host,ts,val) values ("hi",1702433146000,1);
ERROR 1815 (HY000): Invalid request to region 4398046511104(1024, 0), reason: column val expect type Float64(Float64Type), given: INT32(3)

For fuzz tests, let's start with naive cases.
e.g.,

  1. Create a logical table with some random additional columns(e.g., idc, host)
  2. Validate both logical table columns.
  3. Validate the physical table columns(the newly added)
CREATE TABLE t1 (ts timestamp time index, val double, host string, idc string, primary key(idc, host)) engine = metric with ("on_physical_table" = "phy");

@CookiePieWw
Copy link
Collaborator

For fuzz tests, let's start with naive cases. e.g.,

  1. Create a logical table with some random additional columns(e.g., idc, host)
  2. Validate both logical table columns.
  3. Validate the physical table columns(the newly added)

Got it. I'll make some changes later.

@CookiePieWw
Copy link
Collaborator

I've noticed that the existing insert fuzz test for the mito engine has not been activated yet. Are there some works ahead for it? If so, maybe I could solve it first before proceeding to the insert test for the metric engine.

@WenyXu
Copy link
Member Author

WenyXu commented Apr 29, 2024

has not been activated yet

Let's activate the test. cc @zhongzc

@CookiePieWw
Copy link
Collaborator

CookiePieWw commented Apr 30, 2024

The order of columns of logical table is sorted instead of following the declaration order in create expr, as described in #3694 as an expected behavior. Since the issue has not been closed, I wonder if the behavior would be changed later. If not, I prepare to sort the columns in the create expr for insert fuzz test of the logical table.

@WenyXu
Copy link
Member Author

WenyXu commented Apr 30, 2024

I wonder if the behavior would be changed later.

No, at least not recently.

I prepare to sort the columns in the create expr for insert fuzz test of the logical table.

The insert statement includes the columns explicitly; I guess it should work well without any change.

INSERT INTO table_name (column1, column2, column3, ...)
VALUES (value1, value2, value3, ...);

@CookiePieWw
Copy link
Collaborator

The insert statement includes the columns explicitly; I guess it should work well without any change.

Got it. I found that the omit_column_list option is generated internally, so I'll move it out to control by upper callers.

@CookiePieWw
Copy link
Collaborator

I'm working on the validator of inserted rows recently and encountered a weird thing. The select query failed after an insertion. Minimal case here:

CREATE TABLE `Nam`(
  `NIsi` TIMESTAMP(9) TIME INDEX,
  `uNDe` FLOAT,
  `miNus` INT NULL,
  ut BOOLEAN,
  `doloRuM` DOUBLE NULL,
  sit DOUBLE DEFAULT 0.282613557176039,
  `bLandItiis` INT,
  `DOLOREs` BIGINT,
  excepturi INT,
  `coRPorIS` BIGINT DEFAULT -3634152213850520260,
  `maGNAm` FLOAT NULL,
  est SMALLINT NOT NULL,
  eos BOOLEAN DEFAULT true,
  `ipSAM` BOOLEAN,
  `viTAE` BOOLEAN NULL,
  `vElit` DOUBLE NULL,
  `doloRem` FLOAT NOT NULL,
  `EXPeDITA` SMALLINT DEFAULT -21787,
  `QuIA` FLOAT,
  `FACILIs` BIGINT,
  `Et` BIGINT,
  `VEnIAM` FLOAT DEFAULT 0.67037076,
  PRIMARY KEY(
    ut,
    `FACILIs`,
    `DOLOREs`,
    `Et`,
    `bLandItiis`,
    `QuIA`
  )
) ENGINE = mito;

INSERT INTO
  `Nam` (
    ut,
    `Et`,
    `doloRem`,
    `maGNAm`,
    eos,
    `NIsi`,
    est,
    `vElit`,
    `doloRuM`,
    `FACILIs`
  )
VALUES
  (
    false,
    -659085894348132302,
    0.38609815,
    0.6564076,
    DEFAULT,
    '1776-06-28 17:25:38.207225128+0000',
    7684,
    0.40258472138796686,
    NULL,
    NULL
  );

> SELECT * from `Nam`;
ERROR 1815 (HY000): Internal error: 1003

I wonder if it's expected.

@WenyXu
Copy link
Member Author

WenyXu commented May 13, 2024

Hi @CookiePieWw , Thanks a lot. I tried on the main branch, and didn't reproduce it. It should be fixed in #3876

@CookiePieWw
Copy link
Collaborator

Hi @CookiePieWw , Thanks a lot. I tried on the main branch, and didn't reproduce it. It should be fixed in #3876

Thanks! I forgot to re-compile with the recent commit. The patch works for me.

I almost finished but get stuck on multiple timestamp types. I'm utilizing try_get of sqlx's Row to extract the internal value from the select query, like:

let fetched_value = {
    let value_type = fetched_row.column(idx).type_info().name();
    match value_type {
        "BOOL" | "BOOLEAN" => RowValue::Value(Value::Boolean(
            fetched_row.try_get::<bool, usize>(idx).unwrap(),
        )),
        ... // other types,
        "TIMESTAMP" => RowValue::Value(Value::Timestamp(
        Timestamp::from_chrono_datetime(
            fetched_row
            .try_get::<DateTime<Utc>, usize>(idx)
            .unwrap()
            .naive_utc(),
            )
            .unwrap(),
        )),
    }
}

But for the timestamp, with different time stamp types, the results will have a consistent shift compared to the original value:

// For TIMESTAMP(0)
Expected value: '-261245-03-17 18:48:20+0000', got: '0899-03-17 18:48:20+0000'
Expected value: '-179184-12-17 05:26:54+0000', got: '+17424-12-17 05:26:54+0000'
Expected value: '-177707-06-01 00:41:21+0000', got: '+18901-06-01 00:41:21+0000'
// For TIMESTAMP(6)
Expected value: '-261794-08-31 14:46:02.445288+0000', got: '0350-08-31 14:46:02.445288+0000'
Expected value: '-261714-09-03 10:13:07.194687+0000', got: '0430-09-03 10:13:07.194687+0000'
Expected value: '-261460-02-22 02:24:41.115722+0000', got: '0684-02-22 02:24:41.115722+0000'
// For TIMESTAMP(9), it perfectly matched
Expected value: '1686-07-16 08:29:43.983304336+0000', got: '1686-07-16 08:29:43.983304+0000'
Expected value: '1686-08-25 17:41:03.238567505+0000', got: '1686-08-25 17:41:03.238567+0000'
Expected value: '1686-10-12 02:16:43.698871931+0000', got: '1686-10-12 02:16:43.698871+0000'

It seems sqlx treat all the types as the same. I wonder how to correct it, maybe perform some conversions based on the original value type, but the unit field of the timestamp is private.

@WenyXu
Copy link
Member Author

WenyXu commented May 14, 2024

Thanks @CookiePieWw , would you like to provide a repository for reproduction (or debug)?

@waynexia
Copy link
Member

Does sqlx handle different precision of timestamp? From the code snippet above it looks like only

"TIMESTAMP" => RowValue::Value(Value::Timestamp(

@CookiePieWw
Copy link
Collaborator

Does sqlx handle different precision of timestamp?

I mostly follow the docs about types of mysql in sqlx here: https://docs.rs/sqlx/latest/sqlx/mysql/types/index.html. It seems sqlx only consider one kind of timestamp:

image

@CookiePieWw
Copy link
Collaborator

Thanks @CookiePieWw , would you like to provide a repository for reproduction (or debug)?

I created a draft pr in #3932

@WenyXu
Copy link
Member Author

WenyXu commented May 14, 2024

Thanks @CookiePieWw , would you like to provide a repository for reproduction (or debug)?

I created a draft pr in #3932

Thanks, I will take a look ASAP.

@WenyXu
Copy link
Member Author

WenyXu commented May 14, 2024

Thanks @CookiePieWw , would you like to provide a repository for reproduction (or debug)?

I created a draft pr in #3932

Hi @CookiePieWw , After some investigation, I found the key problem:

The encoding of timestamps in the MySQL protocol is defined here:
https://github.com/datafuselabs/opensrv/blob/66140cc266e1eb712a0821c112a3ec743f9cccd4/mysql/src/value/encode.rs#L487-L512

At line 497, it casts self.year() into u16. Consequently, the value -262122 will be converted to 0022.

The range of Timestamp in our codebase is defined as follows:

/// - for [TimeUnit::Second]: [-262144-01-01 00:00:00, +262143-12-31 23:59:59]
/// - for [TimeUnit::Millisecond]: [-262144-01-01 00:00:00.000, +262143-12-31 23:59:59.999]
/// - for [TimeUnit::Microsecond]: [-262144-01-01 00:00:00.000000, +262143-12-31 23:59:59.999999]
/// - for [TimeUnit::Nanosecond]: [1677-09-21 00:12:43.145224192, 2262-04-11 23:47:16.854775807]

One workaround here is to restrict the range of generated timestamps. e.g., we can restrict the range of TimeUnit::Second to [1970-01-01 00:00:00, +1970+U32::MAX-01-01 00:00:00]

fn generate_random_timestamp<R: Rng>(rng: &mut R, ts_type: TimestampType) -> Value {
let v = match ts_type {
TimestampType::Second(_) => {
let min = i64::from(Timestamp::MIN_SECOND);
let max = i64::from(Timestamp::MAX_SECOND);
let value = rng.gen_range(min..=max);
Timestamp::new_second(value)
}
TimestampType::Millisecond(_) => {
let min = i64::from(Timestamp::MIN_MILLISECOND);
let max = i64::from(Timestamp::MAX_MILLISECOND);
let value = rng.gen_range(min..=max);
Timestamp::new_millisecond(value)
}
TimestampType::Microsecond(_) => {
let min = i64::from(Timestamp::MIN_MICROSECOND);
let max = i64::from(Timestamp::MAX_MICROSECOND);
let value = rng.gen_range(min..=max);
Timestamp::new_microsecond(value)
}
TimestampType::Nanosecond(_) => {
let min = i64::from(Timestamp::MIN_NANOSECOND);
let max = i64::from(Timestamp::MAX_NANOSECOND);
let value = rng.gen_range(min..=max);
Timestamp::new_nanosecond(value)
}
};
Value::from(v)
}

Maybe we need some functions like generate_random_timestamp_for_mysql, generate_random_value_for_mysql.

@CookiePieWw
Copy link
Collaborator

Ah I see. So GreptimeDB supports a much wider range of timestamp then MySQL, then the protocol fails to convert the timestamps of different ranges.

Maybe we need some functions like generate_random_timestamp_for_mysql, generate_random_value_for_mysql.

I'll firstly add a simple generate_random_timestamp_for_mysql for it and other types if needed.

@WenyXu WenyXu closed this as completed May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants