From 4b55926d53066efa418d84cc278acb82307e970d Mon Sep 17 00:00:00 2001
From: Noel Kwan <47273164+kwannoel@users.noreply.github.com>
Date: Sat, 26 Nov 2022 15:59:13 +0800
Subject: [PATCH] fix(sqlsmith): skip division by zero error (#6599)

* update validation of errors

* test skipped query counts

* docs, naming

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
---
 src/tests/sqlsmith/src/runner.rs | 69 +++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/src/tests/sqlsmith/src/runner.rs b/src/tests/sqlsmith/src/runner.rs
index 1d824985eee56..d008e72091e2c 100644
--- a/src/tests/sqlsmith/src/runner.rs
+++ b/src/tests/sqlsmith/src/runner.rs
@@ -19,10 +19,15 @@ use tokio_postgres::error::{DbError, Error as PgError, SqlState};
 
 use crate::{create_table_statement_to_table, mview_sql_gen, parse_sql, sql_gen, Table};
 
+/// e2e test runner for sqlsmith
 pub async fn run(client: &tokio_postgres::Client, testdata: &str, count: usize) {
     let mut rng = rand::rngs::SmallRng::from_entropy();
     let (tables, mviews, setup_sql) = create_tables(&mut rng, testdata, client).await;
 
+    // Test sqlsmith first
+    test_sqlsmith(client, &mut rng, tables.clone(), &setup_sql).await;
+    tracing::info!("Passed sqlsmith tests");
+
     // Test batch
     // Queries we generate are complex, can cause overflow in
     // local execution mode.
@@ -49,6 +54,47 @@ pub async fn run(client: &tokio_postgres::Client, testdata: &str, count: usize)
     drop_tables(&mviews, testdata, client).await;
 }
 
+/// Sanity checks for sqlsmith
+pub async fn test_sqlsmith<R: Rng>(
+    client: &tokio_postgres::Client,
+    rng: &mut R,
+    tables: Vec<Table>,
+    setup_sql: &str,
+) {
+    // Test percentage of skipped queries <=5% of sample size.
+    let mut batch_skipped = 0;
+    let batch_sample_size = 100;
+    client
+        .query("SET query_mode TO distributed;", &[])
+        .await
+        .unwrap();
+    for _ in 0..batch_sample_size {
+        let sql = sql_gen(rng, tables.clone());
+        tracing::info!("Executing: {}", sql);
+        let response = client.query(sql.as_str(), &[]).await;
+        batch_skipped +=
+            validate_response_with_skip_count(setup_sql, &format!("{};", sql), response);
+    }
+    if batch_skipped > 5 {
+        panic!("skipped >5% batch queries");
+    }
+
+    let mut stream_skipped = 0;
+    let stream_sample_size = 100;
+    for _ in 0..stream_sample_size {
+        let (sql, table) = mview_sql_gen(rng, tables.clone(), "stream_query");
+        tracing::info!("Executing: {}", sql);
+        let response = client.execute(&sql, &[]).await;
+        stream_skipped +=
+            validate_response_with_skip_count(setup_sql, &format!("{};", sql), response);
+        drop_mview_table(&table, client).await;
+    }
+
+    if stream_skipped > 5 {
+        panic!("skipped >5% stream queries");
+    }
+}
+
 fn get_seed_table_sql(testdata: &str) -> String {
     let seed_files = vec!["tpch.sql", "nexmark.sql"];
     seed_files
@@ -136,24 +182,35 @@ fn is_broken_chan_err(db_error: &DbError) -> bool {
         .contains("internal error: broken fifo_channel")
 }
 
+/// Certain errors are permitted to occur. This is because:
+/// 1. It is more complex to generate queries without these errors.
+/// 2. These errors seldom occur, skipping them won't affect overall effectiveness of sqlsmith.
 fn is_permissible_error(db_error: &DbError) -> bool {
     let is_internal_error = *db_error.code() == SqlState::INTERNAL_ERROR;
-    is_internal_error
-        && (is_numeric_out_of_range_err(db_error)
-            || is_broken_chan_err(db_error)
-            || is_division_by_zero_err(db_error))
+    (is_internal_error && is_broken_chan_err(db_error))
+        || is_numeric_out_of_range_err(db_error)
+        || is_division_by_zero_err(db_error)
 }
 
 /// Validate client responses
 fn validate_response<_Row>(setup_sql: &str, query: &str, response: Result<_Row, PgError>) {
+    validate_response_with_skip_count(setup_sql, query, response);
+}
+
+/// Validate client responses, returning a count of skipped queries.
+fn validate_response_with_skip_count<_Row>(
+    setup_sql: &str,
+    query: &str,
+    response: Result<_Row, PgError>,
+) -> i64 {
     match response {
-        Ok(_) => {}
+        Ok(_) => 0,
         Err(e) => {
             // Permit runtime errors conservatively.
             if let Some(e) = e.as_db_error()
                 && is_permissible_error(e)
             {
-                return;
+                return 1;
             }
             panic!(
                 "