Object store macros

[skejserjensen]

This PR fixes #278 by adding two procedural macros, data_folder_test and object_store_test that generate #[tokio::test] functions that call the annotated function with all permutations with replacements of DataFolder configurations and ObjectStore's, respectively. For example, the following code executes the annotated function with all permutations with replacements of ObjectStore can be used to call test_object_store_list() and the test completes successfully. Both macros also work with two and three parameters, and with more parameters, the macros generate more test functions as more are needed to test all permutations with replacements.

use modelardb_macros::object_store_test;

#[object_store_test]
async fn test_object_store_list(store_one: &dyn ObjectStore) {
    let mut files = store_one.list(None);
    while let Some(f) = files.next().await {
       assert!(......);
    }
}

The current implementation has a few known limitations that I deferred until we gain more experience with using it so we can improve it based on our experience instead of my guesses about how it might be used.

1. There are no automatic tests as it requires additional dependencies, and while most online recommend implementing Rust procedural macros using a set of extra crates that is not included with Rust, they made the code more complicated in my opinion. So since these macros are fairly simple as soon as one understands how Rust procedural macros work, I decided to remove all the extra dependencies and manually test the macros thoroughly before creating the PR.
2. I could not find a way to use data_folder_test to test DataFolder as it causes Rust to become confused about which version of the DataFolder struct to use. I tried to add a method to DataFolder so one can be created from an existing ObjectStore so we could implement all tests using only object_store_test and solve the problem that way; however, DataFolder performs initialization for LocalFileSystem that I could not figure out a way to implement in a method that accepts any &dyn ObjectStore and creates a correctly configured DataFolder from it. Another benefit of adding this method would also be to decouple the initialization of ObjectStore and DataFolder as DataFolder currently does both and thus has many open_*() methods.
3. data_folder_test does not work with Azurite, but object_store_test does, and as far as I can tell, the code is the same for both. However, since the DataFolder configuration for Azure explicitly states that it needs to be tested and thus probably needed to be changed, I left this for another PR to avoid making changes to large changes to DataFolder in this.

[Copilot]

Pull request overview

Adds reusable test infrastructure to exercise ModelarDB behavior across multiple ObjectStore and DataFolder backends, primarily by introducing two attribute-style procedural macros that generate #[tokio::test] wrappers for all permutations (with replacement) of the supported configurations. The PR also extends Azure DataFolder initialization to support emulator mode and updates call sites accordingly.

Changes:

• Introduces modelardb_macros with #[data_folder_test] and #[object_store_test] to auto-generate Tokio tests across backend permutations.
• Adds modelardb_test::{object_store, data_folder} helpers (and shared constants) for constructing test backends (InMemory, local FS, MinIO, Azurite).
• Extends DataFolder::open_azure with an emulator flag and updates embedded C API usage.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
crates/modelardb_test/src/object_store.rs	Adds constructors for test ObjectStore implementations (memory/local/S3/Azure).
crates/modelardb_test/src/lib.rs	Exposes new test helper modules and adds a shared bucket/container constant.
crates/modelardb_test/src/data_folder.rs	Adds constructors for test DataFolder configurations (memory/local/S3/Azure).
crates/modelardb_test/Cargo.toml	Adds dependencies needed by the new test helper modules (storage, object_store features, url/tempfile).
crates/modelardb_storage/src/parser.rs	Refactors loop parsing to use ? for simpler error propagation.
crates/modelardb_storage/src/data_folder/mod.rs	Adds Azure “use emulator” option plumbing and updates test imports/aliases.
crates/modelardb_storage/src/data_folder/delta_table_writer.rs	Renames the modelardb_test::table import alias for clarity in tests.
crates/modelardb_macros/src/lib.rs	Implements the new procedural macros and supporting token parsing utilities.
crates/modelardb_macros/src/error.rs	Defines a minimal error type for macro parsing failures.
crates/modelardb_macros/Cargo.toml	Adds the new proc-macro crate manifest and dependencies.
crates/modelardb_embedded/src/capi.rs	Updates Azure open path to pass the new emulator flag argument.
Cargo.toml	Adds itertools to workspace dependencies for macro implementation.
Cargo.lock	Locks new/updated dependency graph after adding itertools and the new crate.

Suppressed comments (2)

crates/modelardb_macros/src/lib.rs:256

• Malformed intra-doc link here as well ([ModelarDbMacrosError]`), which will cause rustdoc warnings/errors.

/// Return [`Ok`] if the next [`TokenTree`] from `token_iterator` is an [`Ident`] that contains
/// `content`, otherwise a [`ModelarDbMacrosError] is returned.

crates/modelardb_macros/src/lib.rs:270

• Malformed intra-doc link here as well ([ModelarDbMacrosError]`), which will cause rustdoc warnings/errors.

/// Return [`Ok`] if the next [`TokenTree`] from `token_iterator` is an [`Punct`] that contains
/// `content`, otherwise a [`ModelarDbMacrosError] is returned.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

crates/modelardb_macros/src/lib.rs:138

• The generated test calls pass &modelardb_test::object_store::...() directly, which borrows a temporary Box<dyn ObjectStore> across .await. This can fail to compile (temporary dropped while borrowed) and also relies on deref coercions in a brittle way. Generate locals and pass as_ref() for each store.

        let name = object_store_permutation.iter().join("__");
        let arguments = object_store_permutation
            .iter()
            .map(|osn| format!("&modelardb_test::object_store::{}()", osn))
            .join(", ");

crates/modelardb_test/src/object_store.rs:35

• local_file_system_object_store() creates a TempDir but drops it before returning the LocalFileSystem. Since TempDir deletes the directory on drop, the returned object store ends up pointing at a path that no longer exists.

pub fn local_file_system_object_store() -> Box<dyn ObjectStore> {
    let temp_dir = tempfile::tempdir().unwrap();
    let local_file_system = LocalFileSystem::new_with_prefix(temp_dir.path()).unwrap();
    Box::new(local_file_system)

crates/modelardb_test/src/data_folder.rs:31

• local_file_system_data_folder() creates a TempDir but drops it before returning the DataFolder. Since TempDir deletes the directory on drop, the returned DataFolder may point at a directory that has already been removed.

pub async fn local_file_system_data_folder() -> DataFolder {
    let temp_dir = tempfile::tempdir().unwrap();
    DataFolder::open_local(temp_dir.path()).await.unwrap()
}

crates/modelardb_macros/src/lib.rs:166

• The expect() message includes a literal {parameter_type} placeholder, so it won’t show the actual expected parameter type when the macro panics. Using format!(...) here makes macro failures much easier to diagnose.

    let parameter_count = expect_parameter_type_and_count(function_parameter_group, parameter_type)
        .expect("Assumes all of the function's parameters are of type {parameter_type}.");

crates/modelardb_macros/Cargo.toml:27

• modelardb_test is not used by the proc-macro implementation itself (it only appears in emitted token strings), so keeping it as a dependency increases compile time and introduces an unnecessary risk of dependency cycles.

[dependencies]
itertools.workspace = true
modelardb_test = { path = "../modelardb_test" }

[Copilot]

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

crates/modelardb_macros/Cargo.toml:27

• modelardb_test is not used by the proc-macro crate implementation; it is only referenced in the generated tokens. Keeping it as a dependency increases compile time and can introduce unwanted dependency coupling/cycles.

[dependencies]
itertools.workspace = true
modelardb_test = { path = "../modelardb_test" }

crates/modelardb_macros/src/lib.rs:210

• The expect() message contains a literal {parameter_type} placeholder, so the panic output will be misleading/unhelpful when the macro fails to parse parameters.

    let (function_name_ident, function_parameter_group) =
        next_ident_and_group(input.clone()).expect("Assumes input is a function with parameters.");
    let function_name = function_name_ident.to_string();
    let parameter_count = expect_parameter_type_and_count(function_parameter_group, parameter_type)
        .expect("Assumes all of the function's parameters are of type {parameter_type}.");
    (function_name, parameter_count)

crates/modelardb_macros/src/lib.rs:50

• Doc comment grammar: “An function” should be “A function”.

/// An function that will create an argument to be passed to a function as a borrow.

crates/modelardb_test/src/object_store.rs:70

• aws3_object_store() silently ignores invalid config keys (Err(_) => builder). Since these keys are static and required for correct test setup, swallowing parse errors can lead to misconfigured stores and confusing failures later.

    let amazon_s3 = storage_options
        .iter()
        .fold(
            AmazonS3Builder::new()
                .with_url(url.to_string())
                .with_allow_http(true),
            |builder, (key, value)| match key.parse() {
                Ok(k) => builder.with_config(k, value),
                Err(_) => builder,
            },
        )

[Copilot]

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

crates/modelardb_macros/Cargo.toml:27

• modelardb_test is not used by the proc-macro crate at compile time; it only appears as a path in emitted tokens. Keeping this dependency increases compile time and (together with modelardb_test -> modelardb_storage) can contribute to workspace dependency cycles.

[dependencies]
itertools.workspace = true
modelardb_test = { path = "../modelardb_test" }

crates/modelardb_macros/src/lib.rs:209

• The .expect(...) message contains an uninterpolated {parameter_type} placeholder, so the panic text is misleading and makes debugging harder.

    let parameter_count = expect_parameter_type_and_count(function_parameter_group, parameter_type)
        .expect("Assumes all of the function's parameters are of type {parameter_type}.");

crates/modelardb_macros/src/lib.rs:232

• This doc comment has a stray backtick before "The return", which will render oddly in rustdoc.

/// Returns the number of `parameter_type` parameters in `function_parameter_group`. `The return

crates/modelardb_macros/src/lib.rs:210

• These .expect(...) calls will panic when the macro is applied to an unsupported function shape, producing a generic "proc macro panicked" error. Procedural macros should generally emit a compile_error!(...) token stream with a user-facing message (you already have ModelarDbMacrosError) instead of panicking.

    let (function_name_ident, function_parameter_group) =
        next_ident_and_group(input.clone()).expect("Assumes input is a function with parameters.");
    let function_name = function_name_ident.to_string();
    let parameter_count = expect_parameter_type_and_count(function_parameter_group, parameter_type)
        .expect("Assumes all of the function's parameters are of type {parameter_type}.");
    (function_name, parameter_count)

crates/modelardb_test/src/object_store.rs:38

• Grammar: "a AmazonS3" should be "an AmazonS3".

/// Return a [`AmazonS3`](object_store::aws::AmazonS3) [`ObjectStore`] for testing.

crates/modelardb_macros/src/lib.rs:50

• Grammar: "An function" should be "A function".

/// An function that will create an argument to be passed to a function as a borrow.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (6)

crates/modelardb_macros/Cargo.toml:27

• modelardb_test is listed as a dependency of the proc-macro crate, but the implementation does not use it directly (it only appears in emitted tokens). Keeping this dependency pulls the entire test helper crate into any crate that depends on modelardb_macros, increasing build times and risking dependency cycles.

[dependencies]
itertools.workspace = true
modelardb_test.workspace = true

crates/modelardb_macros/src/lib.rs:238

• The error message in this expect(...) contains an uninterpolated {parameter_type} placeholder, so macro failures will report a confusing literal string. Also, this discards the actual parse error context from expect_parameter_type_and_count().

    let function_name = function_name_ident.to_string();
    let parameter_count = expect_parameter_type_and_count(function_parameter_group, parameter_type)
        .expect("Assumes all of the function's parameters are of type {parameter_type}.");
    (function_name, parameter_count)

crates/modelardb_macros/src/lib.rs:76

• The macro-generated tests call tempfile::tempdir() and functions under modelardb_test::..., but the macro docs don’t mention these required dependencies. Without this, downstream crates may get opaque compile errors like “cannot find crate tempfile”.

/// Macro for generating test functions that use all permutations with replacements of `DataFolder`
/// The macro must be placed on an `async` function without `#[test]` or `#[tokio::test]` that only
/// has `&DataFolder` parameters. It will generate one `#[tokio::test]` function for each
/// permutation with replacement of `DataFolder` configurations that call the annotated function.

crates/modelardb_macros/src/lib.rs:133

• This comment has a couple of grammatical typos (“iterate” / “produce”) which makes the explanation harder to read when maintaining the macro implementation.

    // Create an iterate that produce all permutations with replacements of the items in
    // data_folders. First the code creates an iterator that repeats the data_folders iterator
    // data_folder_parameter_count times. Then these iterators are crossed together to produce each

crates/modelardb_test/src/data_folder.rs:34

• Typo in doc comment: “AWS3” should be “AWS S3”, and the project name is typically spelled “MinIO”.

/// Return a [`DataFolder`] storing data in an Amazon S3 compatible object store for testing using MinIO.

crates/modelardb_test/src/object_store.rs:67

• key.parse() errors are silently ignored here, which can lead to a partially configured S3 client (and much harder-to-debug failures). Since the keys are hard-coded, it’s better to fail fast if a key ever becomes invalid.

            |builder, (key, value)| match key.parse() {
                Ok(k) => builder.with_config(k, value),
                Err(_) => builder,
            },

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

crates/modelardb_storage/src/data_folder/mod.rs:218

• key.parse().unwrap() can panic at runtime if an option key ever becomes invalid (e.g., typo / future refactor), turning what should be a recoverable configuration error into a hard crash. Since open_s3() already returns Result, it should map parse failures into a ModelarDbStorageError instead of panicking.

                |builder, (key, value)| {
                    let key = key.parse().unwrap();
                    builder.with_config(key, value)
                },

crates/modelardb_macros/src/lib.rs:239

• These expect(...) calls will panic inside the procedural macro on invalid input, which yields a "proc macro panicked" compiler error and hides the actionable diagnostics. For user-facing macros, prefer returning a TokenStream containing compile_error!(...) with a clear message (or threading a Result up to the attribute entrypoints and emitting compile_error! there).

    let (function_name_ident, function_parameter_group) =
        next_ident_and_group(input.clone()).expect("Assumes input is a function with parameters.");
    let function_name = function_name_ident.to_string();
    let parameter_count = expect_parameter_type_and_count(function_parameter_group, parameter_type)
        .expect(&format!(
            "Assumes all of the function's parameters are of type {parameter_type}."
        ));

crates/modelardb_macros/src/lib.rs:273

• expect_parameter_type_and_count() currently errors out on an empty parameter list because it always tries to parse at least one parameter. That makes #[data_folder_test] / #[object_store_test] unusable on async fn foo() even though there are valid cases where a test might take zero stores/folders (and should just generate one #[tokio::test]).

    let mut token_peekable_iterator = function_parameter_group.stream().into_iter().peekable();

    let mut parameter_count = 0;
    loop {
        // Return an error if the next parameter does not match parameter_type.
        expect_parameter_type(&mut token_peekable_iterator, parameter_type)?;