datafusion-contrib / datafusion-table-providers

DataFusion TableProviders for reading data from other systems
Apache License 2.0
59 stars 17 forks source link

[build] Add PR validation for flight #148

Closed ccciudatu closed 3 weeks ago

ccciudatu commented 3 weeks ago

Also includes a cosmetic commit that isn't worth a PR of its own

ccciudatu commented 3 weeks ago

@phillipleblanc Regarging PR checks, should we perhaps include some flavour of cargo test --all-features to ensure that tests aren't broken?

As a side note, all the tests are currently passing, except for test_duckdb_connection_pool_with_attached_databases which is flaky , i.e. fails about half of the time with:

thread 'sql::db_connection_pool::duckdbpool::test::test_duckdb_connection_pool_with_attached_databases' panicked at src/sql/db_connection_pool/duckdbpool.rs:336:14:
Query should be successful: DuckDBError { source: DuckDBFailure(Error { code: Unknown, extended_code: 1 }, Some("TransactionContext Error: Catalog write-write conflict on create with \"attachment_0\"")) }
phillipleblanc commented 3 weeks ago

@phillipleblanc Regarging PR checks, should we perhaps include some flavour of cargo test --all-features to ensure that tests aren't broken?

As a side note, all the tests are currently passing, except for test_duckdb_connection_pool_with_attached_databases which is flaky , i.e. fails about half of the time with:

thread 'sql::db_connection_pool::duckdbpool::test::test_duckdb_connection_pool_with_attached_databases' panicked at src/sql/db_connection_pool/duckdbpool.rs:336:14:
Query should be successful: DuckDBError { source: DuckDBFailure(Error { code: Unknown, extended_code: 1 }, Some("TransactionContext Error: Catalog write-write conflict on create with \"attachment_0\"")) }

Yeah, I thought we were already doing that - so if we're not its a bug. @peasee can you take a look at the flaky test?

ccciudatu commented 3 weeks ago

Yeah, I thought we were already doing that - so if we're not its a bug.

@phillipleblanc Nope, lib/doc tests aren't even compiled as part of the PR checks.