Skip to content

Conversation

alamb
Copy link

@alamb alamb commented Sep 5, 2025

I was reviewing apache#17364 and had some ideas on how to improve it

@alamb alamb changed the title Fix url Simplify implementation, add tests Sep 5, 2025
}

#[tokio::test]
async fn test_list_files() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added new tests

I verified these tests fail without the code changes in this PR

called `Result::unwrap()` on an `Err` value: ObjectStore(NotFound { path: "t", source: NoDataInMemory { path: "t" } })
thread 'url::tests::test_list_files' panicked at datafusion/datasource/src/url.rs:697:14:
called `Result::unwrap()` on an `Err` value: ObjectStore(NotFound { path: "t", source: NoDataInMemory { path: "t" } })
stack backtrace:

}
}

async fn list_with_cache<'b>(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this into its own function and simplified the implementation

@xiedeyantu
Copy link
Owner

Thanks for your detail review! These can help me a lot! @alamb

@xiedeyantu xiedeyantu merged commit bb724d9 into xiedeyantu:fix-url Sep 5, 2025
55 checks passed
@alamb alamb deleted the fix-url branch September 6, 2025 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants