alamb commented on code in PR #5015:
URL: https://github.com/apache/arrow-datafusion/pull/5015#discussion_r1083439831


##########
datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs:
##########
@@ -29,69 +30,157 @@ use postgres_types::Type;
 use rust_decimal::Decimal;
 use tokio_postgres::{Column, Row};
 
-pub mod image;
+// default connect string, can be overridden by environment PG_DSN
+const PG_DSN: &str = "postgresql://[email protected]/test";
+
+/// DataFusion sql-logicaltest error
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error("Postgres error: {0}")]
+    Postgres(#[from] tokio_postgres::error::Error),
+    #[error("Error handling copy command: {0}")]
+    Copy(String),
+}
+
+pub type Result<T, E = Error> = std::result::Result<T, E>;
 
 pub struct Postgres {
-    client: Arc<tokio_postgres::Client>,
+    client: tokio_postgres::Client,
     join_handle: JoinHandle<()>,
+    /// Filename, for display purposes
     file_name: String,
 }
 
 impl Postgres {
-    pub async fn connect_with_retry(
-        file_name: String,
-        host: &str,
-        port: u16,
-        db: &str,
-        user: &str,
-        pass: &str,
-    ) -> Result<Self, tokio_postgres::error::Error> {
+    /// Creates a runner for executiong queries against an existing
+    /// posgres connection. `file_name` is used for display output
+    ///
+    /// The database connection details can be overridden by the
+    /// `PG_DSN` environment variable.
+    ///
+    /// This defaults to
+    ///
+    /// ```text
+    /// PG_DSN="postgresql://[email protected]/test"
+    /// ```
+    ///
+    /// See 
https://docs.rs/tokio-postgres/latest/tokio_postgres/config/struct.Config.html#url
 for format
+    pub async fn connect(file_name: impl Into<String>) -> Result<Self> {
+        let file_name = file_name.into();
+
+        let dsn = if let Ok(val) = std::env::var("PG_DSN") {
+            val
+        } else {
+            PG_DSN.to_string()
+        };
+
+        debug!("Using posgres dsn: {dsn}");
+
+        let config = tokio_postgres::Config::from_str(&dsn)?;
+
         let mut retry = 0;

Review Comment:
   I think some of the options from the container setup (that I copied from 
@Jimexist ) may help. 
   
   ```
           ports:
             - 5432/tcp
           options: >-
             --health-cmd pg_isready
             --health-interval 10s
             --health-timeout 5s
             --health-retries 5
   ```



##########
datafusion/core/tests/sqllogictests/test_files/pg_compat_simple.slt:
##########
@@ -15,6 +15,67 @@
 # specific language governing permissions and limitations
 # under the License.
 
+###
+## Setup test for postgres
+###
+
+onlyif postgres
+statement ok
+DROP TABLE IF EXISTS aggregate_test_100_by_sql;

Review Comment:
   Thanks -- this is a good point @melgenek 
   
   At the very least the tests should clean up after themselves (as in I should 
put a `drop_table` at the end). I like the idea of a separate schema, however. 
I will try and add that shortly



-- 
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]

Reply via email to