melgenek commented on code in PR #5253:
URL: https://github.com/apache/arrow-datafusion/pull/5253#discussion_r1103626897
##########
datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs:
##########
@@ -319,3 +271,73 @@ impl sqllogictest::AsyncDB for Postgres {
"postgres"
}
}
+
+fn convert_rows(rows: Vec<Row>) -> Vec<Vec<String>> {
+ rows.iter()
+ .map(|row| {
+ row.columns()
+ .iter()
+ .enumerate()
+ .map(|(idx, column)| cell_to_string(row, column, idx))
+ .collect::<Vec<String>>()
+ })
+ .collect::<Vec<_>>()
+}
+
+macro_rules! make_string {
+ ($row:ident, $idx:ident, $t:ty) => {{
+ let value: Option<$t> = $row.get($idx);
+ match value {
+ Some(value) => value.to_string(),
+ None => NULL_STR.to_string(),
+ }
+ }};
+ ($row:ident, $idx:ident, $t:ty, $convert:ident) => {{
+ let value: Option<$t> = $row.get($idx);
+ match value {
+ Some(value) => $convert(value).to_string(),
+ None => NULL_STR.to_string(),
+ }
+ }};
+}
+
+fn cell_to_string(row: &Row, column: &Column, idx: usize) -> String {
Review Comment:
There are no changes to this function, I just moved it downwards.
##########
datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs:
##########
@@ -290,27 +249,20 @@ impl sqllogictest::AsyncDB for Postgres {
return Ok(DBOutput::StatementComplete(0));
}
let rows = self.client.query(sql, &[]).await?;
- let output = rows
- .iter()
- .map(|row| {
- row.columns()
- .iter()
- .enumerate()
- .map(|(idx, column)| cell_to_string(row, column, idx))
- .collect::<Vec<String>>()
- })
- .collect::<Vec<_>>();
- if output.is_empty() {
- let stmt = self.client.prepare(sql).await?;
- Ok(DBOutput::Rows {
- types: vec![DFColumnType::Any; stmt.columns().len()],
- rows: vec![],
- })
+ if rows.is_empty() {
+ return Ok(DBOutput::StatementComplete(0));
Review Comment:
Datafusion currently considers empty results as "statements"
https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/tests/sqllogictests/src/engines/datafusion/normalize.rs#L32-L35.
This change makes Postgres produce the same result when there are no values.
Technically, an opposite change is possible: update Datafusion to consider
an empty batch as empty query results, but with types.
##########
datafusion/core/tests/sqllogictests/src/engines/datafusion/mod.rs:
##########
@@ -18,7 +18,7 @@
use std::path::PathBuf;
use std::time::Duration;
-use crate::output::{DFColumnType, DFOutput};
+use crate::engines::output::{DFColumnType, DFOutput};
Review Comment:
I moved the `output.rs` into the `engines/` folder. I just prefer narrowing
the scope of files to where they are used, and there's no need to expose the
type outside of the `engines/` folder now.
##########
datafusion/core/tests/sqllogictests/test_files/pg_compat/pg_compat_type_coercion.slt:
##########
@@ -15,108 +15,37 @@
# specific language governing permissions and limitations
# under the License.
-query ??
+query BB
select true and true, false and false;
----
true false
-onlyif DataFusion
Review Comment:
The `arrow_typeof` and `pg_typeof` checks are no longer needed for the
coercion tests. Sqllogictest now checks that the columns are `Boolean`.
##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -524,7 +524,7 @@ select * from decimal_simple where c1 >= 0.00004 order by
c1 limit 5;
0.00005 0.000000000005 9 true 0.000052
-query RRI?R
+query FFIBF
Review Comment:
I updated types using `--complete` in this pr. I pointed datafusion to a
local version of `sqllogictest-rs` with changes from this pr
https://github.com/risinglightdb/sqllogictest-rs/pull/164.
`sqllogictest-rs:0.12` doesn't complete types.
##########
datafusion/core/tests/sqllogictests/README.md:
##########
@@ -105,10 +105,13 @@ query <type_string> <sort_mode>
- `test_name`: Uniquely identify the test name (arrow-datafusion only)
- `type_string`: A short string that specifies the number of result columns
and the expected datatype of each result column. There is one character in the
<type_string> for each result column. The characters codes are:
- - "T" for a text result,
- - "I" for an integer result,
- - "R" for a floating-point result,
- - "?" for any other type.
+ - 'B' - **B**oolean,
+ - 'D' - **D**atetime,
+ - 'I' - **I**nteger,
+ - 'F' - **F**loating-point results,
+ - 'S' - **S**string,
+ - 'T' - **T**imestamp,
+ - "?" - any other types
Review Comment:
Honestly, I wanted to add only `Boolean` at first but then reshuffled the
letters to accommodate more types.
Notable differences here:
- `F` for the floating points. Sqllite defines floating points as `R`.
[Cocroach](https://github.com/cockroachdb/cockroach/blob/899c9112188c22ec140adbeb0cf7d5761da3827d/pkg/sql/logictest/logic.go#L285-L287)
differentiates floating points `F`, and decimals `R`
- `S` for strings. Sqllite and Cocroach define these as `T` text. But I
repurposed `T` for **t**imestamps.
I tried using first letters of the words that have some meaning, but I am
open for suggestions here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
