support extended query mode #65

ZENOTME · 2022-07-29T12:41:03Z

Support extended query mode.
It's compatible with past versions, extended query mode only takes effect when using the flag "--extend"

for now, this pr has the following limit:

we only support most common type in binary format (but not all the type.
rust-postgres only support one-dimensional array type.

Hence we can run all original e2e test now.

xxchan · 2022-07-29T12:49:58Z

src/main.rs

+    /// extended query mode in postgresql(only valid in postgresql)
+    #[clap(long, action)]
+    extend: bool,


I think we can add a different engine postgresql-extended instead of adding the option

do you mean create a different bin using different engine, and we can use different bin to run test in different mode. such as:

sqllogictest 'e2e_test/*.slt' (simeple query) postgresql-extended 'e2e_test/*.slt' (extended query)

I mean using the engine CLI argument.

sqllogictest a.slt --engine postgresql sqllogictest a.slt --engine mysql sqllogictest a.slt --engine postgresql-extended

Signed-off-by: Zejiong Dong <[email protected]>

xxchan

Currenty implementation is OK but IMO this might be slightly better:

fn main() 
let engine: Box<dyn AsyncDB> = match opt.engine {
	"postgres" => Postgres { ... },
	"postgres-extended" => PostgresExtended { ... },
};

impl sqllogictest::AsyncDB for Postgres {}

impl sqllogictest::AsyncDB for PostgresExtended {}

In this way, the engine dispath is more clear, and we don't need the huge if in Postgres.

But since current sqllogictest CLI is a little big coupled with Postgres, maybe we can also refactor it later.

ZENOTME · 2022-08-01T08:08:41Z

Currenty implementation is OK but IMO this might be slightly better:
fn main() 
let engine: Box<dyn AsyncDB> = match opt.engine {
	"postgres" => Postgres { ... },
	"postgres-extended" => PostgresExtended { ... },
};

impl sqllogictest::AsyncDB for Postgres {}

impl sqllogictest::AsyncDB for PostgresExtended {}
In this way, the engine dispath is more clear, and we don't need the huge if in Postgres.

But since current sqllogictest CLI is a little big coupled with Postgres, maybe we can also refactor it later.

Yes..I try this before but CLI has coupled with 'Postgres' such as

if let Err(err) = pg.client.simple_query(&query).await {
                eprintln!("  ignore error: {}", err);
}

So I'm agree with refactoring it later.

ZENOTME · 2022-08-03T13:16:34Z

Sorry, I find someplace need to modify... I test in as many original e2e tests as possible first.

* add single_process to support NULL values. * support '(empty)' in VARCHAR. * support infinity in f32 and f64. * modify output of bool type. Signed-off-by: Zejiong Dong <[email protected]>

ZENOTME · 2022-08-04T08:14:13Z

Now the postgresql-extended can support most of e2e_test!
The current limitation:

can't support array type.
(because the tokio-postgres only support one-dimensional array)
can't support struct type.
can't support infinity of numeric.
(we use rust-decimal to represent numeric and the rust-decimal don't support infinity)
can't support 'interval' type.

skyzh

Better to refine this PR. Doesn't look neat and might make maintenance harder :) But we can merge this first and fix things later, as main.rs is already complex enough. cc @xxchan merge?

skyzh · 2022-08-05T13:21:06Z

Cargo.toml

@@ -16,7 +16,7 @@ bin = ["anyhow", "chrono", "console", "tokio-postgres", "env_logger", "clap", "t
 [dependencies]
 anyhow = { version = "1", optional = true }
 async-trait = "0.1"
-chrono = { version = "0.4", optional = true }
+chrono = { version = "0.4",optional = true }


missing space?

skyzh · 2022-08-05T13:21:32Z

Cargo.toml

@@ -27,7 +27,9 @@ glob = "0.3"
 humantime = "2"
 itertools = "0.10"
 log = "0.4"
+postgres-types = { version = "0.2.3", features = ["derive","with-chrono-0_4"] }


please make this consistent with other lines :) Also I think it would be better to only enable Postgres-types in bin target?

skyzh · 2022-08-05T13:21:40Z

Cargo.toml

 quick-junit = { version = "0.2", optional = true }
+rust_decimal = { version = "1.7.0", features = [ "tokio-pg" ] }


ditto, only enable in bin target

skyzh · 2022-08-05T13:22:17Z

src/main.rs

-                tokio_postgres::SimpleQueryMessage::Row(row) => {
-                    for i in 0..row.len() {
-                        if i != 0 {
+        if !self.extend {


I'd prefer making it a new engine instead of using self.extend to change query mode.

I'd prefer making it a new engine instead of using self.extend to change query mode.

Yes. We can make it when refactor it.

xxchan · 2022-08-05T13:33:21Z

Better to refine this PR. Doesn't look neat and might make maintenance harder :) But we can merge this first and fix things later, as main.rs is already complex enough. cc @xxchan merge?

I can refactor it later 🤤

skyzh · 2022-08-05T13:37:12Z

My plan is like to have a separate crate for bin target of sqllogictest (e.g. sqllogictest-bin). Then I believe things could be a lot more neat.

skyzh · 2022-08-05T13:37:27Z

Anyway, I'd merge this first and not bump version for now.

skyzh · 2022-08-05T13:37:53Z

... after @ZENOTME has resolved some small issues in Cargo.toml.

Signed-off-by: Zejiong Dong <[email protected]>

ZENOTME · 2022-08-05T14:19:36Z

... after @ZENOTME has resolved some small issues in Cargo.toml.

Already resolved it.

ZENOTME mentioned this pull request Jul 29, 2022

Support extended query mode #64

Closed

xxchan reviewed Jul 29, 2022

View reviewed changes

support extended query mode

1dded66

Signed-off-by: Zejiong Dong <[email protected]>

ZENOTME force-pushed the main branch from 080cf7e to 1dded66 Compare July 29, 2022 14:41

support array type(only 1 dimension)

5e60ed2

Signed-off-by: Zejiong Dong <[email protected]>

ZENOTME force-pushed the main branch from 78bf5cd to 5e60ed2 Compare August 1, 2022 06:07

ZENOTME requested a review from xxchan August 1, 2022 06:12

xxchan requested a review from skyzh August 1, 2022 07:36

xxchan approved these changes Aug 1, 2022

View reviewed changes

ZENOTME marked this pull request as draft August 3, 2022 13:14

ZENOTME force-pushed the main branch from 4720169 to 0a1c67f Compare August 4, 2022 08:04

* modify array_process to support NULL value.

03d543b

* add single_process to support NULL values. * support '(empty)' in VARCHAR. * support infinity in f32 and f64. * modify output of bool type. Signed-off-by: Zejiong Dong <[email protected]>

ZENOTME force-pushed the main branch from 0a1c67f to 03d543b Compare August 4, 2022 08:06

ZENOTME marked this pull request as ready for review August 4, 2022 08:14

skyzh reviewed Aug 5, 2022

View reviewed changes

fix format

ecb3e59

Signed-off-by: Zejiong Dong <[email protected]>

xxchan merged commit ab1fbb0 into risinglightdb:main Aug 5, 2022

wangrunji0408 mentioned this pull request Sep 26, 2022

feat(test): support pg extended query in simulation risingwavelabs/risingwave#5552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support extended query mode #65

support extended query mode #65

ZENOTME commented Jul 29, 2022 •

edited

Loading

xxchan Jul 29, 2022

ZENOTME Jul 29, 2022

xxchan Jul 29, 2022 •

edited

Loading

ZENOTME Jul 29, 2022

xxchan left a comment

ZENOTME commented Aug 1, 2022

ZENOTME commented Aug 3, 2022 •

edited

Loading

ZENOTME commented Aug 4, 2022

skyzh left a comment

skyzh Aug 5, 2022

skyzh Aug 5, 2022

skyzh Aug 5, 2022

skyzh Aug 5, 2022

ZENOTME Aug 5, 2022

xxchan commented Aug 5, 2022

skyzh commented Aug 5, 2022

skyzh commented Aug 5, 2022

skyzh commented Aug 5, 2022

ZENOTME commented Aug 5, 2022

		quick-junit = { version = "0.2", optional = true }
		rust_decimal = { version = "1.7.0", features = [ "tokio-pg" ] }

support extended query mode #65

support extended query mode #65

Conversation

ZENOTME commented Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

ZENOTME commented Aug 1, 2022

ZENOTME commented Aug 3, 2022 • edited Loading

ZENOTME commented Aug 4, 2022

skyzh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan commented Aug 5, 2022

skyzh commented Aug 5, 2022

skyzh commented Aug 5, 2022

skyzh commented Aug 5, 2022

ZENOTME commented Aug 5, 2022

ZENOTME commented Jul 29, 2022 •

edited

Loading

xxchan Jul 29, 2022 •

edited

Loading

ZENOTME commented Aug 3, 2022 •

edited

Loading