Skip to content

Conversation

@timsaucer
Copy link
Member

This adds unit testing for write tables APIs to test concurrency.

@timsaucer timsaucer requested a review from Copilot November 3, 2025 13:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds concurrency testing for the write_table API by introducing a parameterized test that validates concurrent write operations with multiple threads.

  • Adds a new test test_concurrent_write_tables that uses threading to test concurrent writes
  • Changes the table_filepath fixture scope from "module" to "function" to ensure test isolation

Reviewed Changes

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

File Description
rerun_py/tests/e2e_redap_tests/test_table_write.py Adds concurrent write test with threading support
rerun_py/tests/e2e_redap_tests/conftest.py Changes fixture scope to function-level for better test isolation

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

@timsaucer timsaucer added 🔨 testing testing and benchmarks exclude from changelog PRs with this won't show up in CHANGELOG.md labels Nov 3, 2025
Copy link
Member

@ntjohnson1 ntjohnson1 left a comment

Choose a reason for hiding this comment

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

Test looks reasonable to me as long as we assume write_table releases the GIL. The test doesn't ensure that the threads run concurrently. If the threads run sequentially this would still pass IIUC.

Approving if that is the desired scope, aince releasing the GIL should be supported by work in datafusion-python (that we both touched parts of).

@timsaucer timsaucer marked this pull request as draft November 6, 2025 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

exclude from changelog PRs with this won't show up in CHANGELOG.md 🔨 testing testing and benchmarks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants