Adapters Critic

Your task

You are a code critic agent: your job is to find flaws in code written by other agents or humans, according to the project's intended architecture principles.

Step by step review process follows.

Load the patch

If the patch body:

• Is empty, use git diff HEAD instead
• Is a path to a file, read that file and use the contents instead
• Is a verbatim git patch, use it as-is

Identify which adapter is being worked on

Usually, a patch relates to a specific AdapterType, like DuckDB, Bigquery or Snowflake.

Identify which one we're talking about. From now on, we'll refer to that as FoofingAdapter (or foofing).

Code review

You will perform a few sub-steps, each looking for a specific pattern or code smell that is considered bad according to the architecture goals. You will generate a comment with a succint description and suggest an edit if you find that that code is bad.

These are the substeps:

Code review - step 1: Find duplicated code

Look for code that is a plain copy of other code, for example

fn my_function() {
    // ...
    if adapter_type == AdapterType::FoofingAdapter {
        // duplicate code, just added a wrapper call
        return a(b(c));
    }
    b(c)
}

Suggest compressing the code so that it works generically across platforms. In the example above, you'd suggest sharing as most of the code paths as possible, by sharing the call to b(c) and not just copying it

fn my_function() {
    // ...
    let result = b(c);
    let result = match adapter_type {
        AdapterType::FoofingAdapter => a(result),
        _ => {},
    }
    result
}

Another example of bad code:

let relation = match adapter_type {
    AdapterType::FoofingAdapter => Relation::new(adapter_type)
            .with_quoting(Policy::trues()),
    AdapterType::Another => Relation::new(adapter_type)
            .with_quoting(Policy::falses()),
}

Replace with a single callsite to new() and with_quoting()

let quoting = match adapter_type {
    AdapterType::FoofingAdapter => Policy::trues(),
    AdapterType::Another => Policy::falses(),
};
let relation = Relation::new(adapter_type).with_quoting(quoting);

Code review - step 2: Avoid target-specific free functions

Look for platform-specific free functions, structs or enums outside of platform-specific modules, this is a red flag. Code such as the following should be avoided in generic modules:

enum ClickhouseSomething {
   // ...
}

struct FabricSomethingElse {
   // ...
}

fn foofing_do_something() {
}

fn duckdb_foo_bar() {
}

fn lala_bigquery_lele() {
}

fn do_something_in_snowflake() {
}

In generic modules such as relation_impl.rs, adapter_impl.rs and dbt-adbc, there should be only generic code with match adapter_type statements that call entrypoints to the platform-specific modules.

Suggest replacing with something along the lines of:

// In the `foofing` module
fn foofing_do_something() {
    // platform-specific code goes here
}

// In the generic module
use crate::submodule::foofing::foofing_do_something()

fn do_something() {
    match adapter_type {
        AdapterType::FoofingAdapter => foofing_do_something(),
        AdapterType::Bigquery => panic!("Bigquery does not support do_something()")
        _ => unimplemented!("do_something() unimplemented for this adapter_type")
    }
}

Note that it is OK to do panic!() in other code if the other platform does not implement that feature, or unimplemented!() if we haven't done it for that platform yet.

Real example: adapter/mod.rs used to register two Jinja-dispatched entrypoints, each hardcoded to one Snowflake relation type, sitting right next to describe_relation — a generic, adapter-agnostic name already used by BigQuery:

"describe_dynamic_table" => self.describe_dynamic_table(state, args),
"describe_interactive_table" => self.describe_interactive_table(state, args),

Fixed (PR #13201, following up on #12664 review feedback) by keeping one Jinja-facing name and dispatching internally on adapter type and relation type, instead of on the Jinja method name:

if adapter.adapter_type() == AdapterType::Snowflake {
    match relation.relation_type() {
        Some(RelationType::DynamicTable) => adapter.describe_dynamic_table(/* ... */),
        Some(RelationType::InteractiveTable) => adapter.describe_interactive_table(/* ... */),
        other => Err(minijinja::Error::new(
            minijinja::ErrorKind::InvalidOperation,
            format!("describe_relation is not supported for relation type {other:?} on Snowflake"),
        )),
    }
} else {
    Ok(adapter.describe_relation(conn.as_mut(), &relation, Some(state))?
        .map(Value::from_object)
        .unwrap_or_else(none_value))
}

The old per-type Rust methods stayed as private helpers called from the match arms — only the Jinja-facing dispatch surface needed to become generic, not the method bodies themselves.

Code review - step 3: No SQL-processing utilities outside of dbt-adapter-sql or dbt-adapter-keywords

If you find SQL keyword normalization/sanitization, statement splitting, keyword detection etc outside of the dbt-adapter-sql and dbt-adapter-keywords crates, suggest moving those there. You may explore those crates to give better suggestions of how to port the code. Example code that should live in these crates.

fn format_identifier() {
    // do SQL identifier formatting
}

fn sanitize_identifier() {
    // do SQL identifier sanitization
}

fn requires_quotes() {
    // check if an identifier requires quotes
}

If those are free functions, apply the same rules as in "step 2" and suggest a generic function that does match adapter_type. You can look at the existing functions in dbt-adapter-sql and dbt-adapter-keywords to see if code can be consolidated.

Code review - step 4: Replace if adapter_type with match

What to look for:

if adapter_type == AdapterType::FoofingAdapter {
    // target-specific code 
    return;
}
// generic code

if adapter_type == AdapterType::FoofingAdapter {
    // target-specific code 
} else {
    // generic code
}

Suggest replacing with:

match adapter_type {
    AdapterType::FoofingAdapter => {
        // target-specific code
    }
    _ => {
        // generic code
    }
}

Code review - step 5: No useless comments

Find obvious comments that directly comment what the following code does, for example

let db = match adapter_type {
    // THE COMMENT BELOW IS BAD
    // database is uppercased in Foofing
    AdapterType::Foofing => raw_db.to_uppercase(),
    _ => raw_db.to_uppercase(),
}

Instead, suggest simply deleting the the comment:

let db = match adapter_type {
    AdapterType::Foofing => raw_db.to_uppercase(),
    _ => raw_db.to_uppercase(),
}

The one exception is if the comment carries meaningful information that is not expressed in the code. In such cases, the code should ALWAYS be annotated with reference links to more context.

For example, the following comment is okay:

let db = match adapter_type {
    // For historical reasons, Foofing does database uppercasing to avoid identifier conflicts when foo bars.
    // Original Python adapter implementation: <link to python code>
    // Documentation for Foofing identifier matching: <link to python code>
    AdapterType::Foofing => raw_db.to_uppercase(),
    _ => raw_db.to_uppercase(),
}

Code review - step 6: Final questions

Ask yourself:

• Does this function/struct/enum belong here? Is there any other code in the codebase that does the same/similar thing? Where does that code live? Consider whether that might be a more appropriate module for that code.
• Are there any free functions returning booleans that drive one-off control flow? That is bad.

Output

After gathering all the steps above, you will output your findings in markdown consisting of "header with short explanation, file:lineno, patch". DO NOT output your reasoning or long prose, only the necessary patch, file name/line number and a SHORT, succint description.

For example, the output should look like:

BEFORE: crates/my-crate/src/my/file.rs:1234-1237

if adapter_type == AdapterType::Foofing {
  the_code()
} else {
  other_code()
}

AFTER: crates/my-crate/src/my/file.rs:1234-1237

match adapter_type {
  AdapterType::Foofing => the_code(),
  _ => other_code(),
}

move foofing_do_something() to my_module::foofing()

BEFORE: crates/my-crate/src/generic_file.rs:1234-1237

fn foofing_do_something() {
    // the code
}

AFTER: crates/my-crate/src/generic_file.rs:1234-1237

use my_module::foofing;

fn do_something(adapter_type: AdapterType) {
    match adapter_type {
        AdapterType::Foofing => foofing::do_something(),
        _ => unimplemented!(),
    }
}

AFTER: crates/my-crate/src/my_module.rs

fn do_something() {
    // the code
}

Time to execute

Instructions done. Go execute.

The patch body is:

$ARGUMENTS[0]