Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove sync methods in core/ Glue struct, #1247

Merged
merged 2 commits into from Jun 10, 2023

Conversation

panarch
Copy link
Member

@panarch panarch commented Jun 10, 2023

Update Glue struct to provide async methods only.
Apply the changes to existing codes (mainly test codes)

Update Glue struct to provide async methods only.
Apply the changes to existing codes (mainly test codes)
@panarch panarch requested review from ever0de and devgony June 10, 2023 06:38
@panarch panarch self-assigned this Jun 10, 2023
@coveralls
Copy link

coveralls commented Jun 10, 2023

Pull Request Test Coverage Report for Build 5228954120

  • 160 of 194 (82.47%) changed or added relevant lines in 18 files are covered.
  • 51 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.1%) to 98.734%

Changes Missing Coverage Covered Lines Changed/Added Lines %
storages/shared-memory-storage/tests/concurrent_access.rs 6 7 85.71%
pkg/rust/tests/glue.rs 7 9 77.78%
storages/json-storage/tests/schema.rs 11 13 84.62%
storages/memory-storage/tests/memory_storage.rs 12 14 85.71%
storages/shared-memory-storage/tests/shared_memory_storage.rs 15 17 88.24%
storages/json-storage/tests/primary_key.rs 6 9 66.67%
storages/composite-storage/tests/error.rs 17 21 80.95%
storages/json-storage/tests/json_dml.rs 14 18 77.78%
storages/sled-storage/tests/export_and_import.rs 15 19 78.95%
storages/composite-storage/tests/basic.rs 13 18 72.22%
Files with Coverage Reduction New Missed Lines %
core/src/ast/function.rs 1 99.88%
core/src/executor/validate.rs 1 99.24%
core/src/plan/expr/function.rs 1 99.56%
storages/sled-storage/tests/sled_transaction.rs 4 96.13%
core/src/executor/evaluate/function.rs 10 97.6%
core/src/executor/aggregate/mod.rs 34 82.47%
Totals Coverage Status
Change from base Build 5221131260: -0.1%
Covered Lines: 45640
Relevant Lines: 46225

💛 - Coveralls

@panarch panarch merged commit fe6c619 into main Jun 10, 2023
9 checks passed
@panarch panarch deleted the remove-sync-method-in-core-glue branch June 10, 2023 08:32
@serprex
Copy link

serprex commented Dec 24, 2023

@panarch I created a PR to update pgwire examples to use latest gluesql, but I found that calling .await on result of execute continued to fail even after moving to storing glue in tokio::sync::Mutex. I even tried getting around having a mutex at all using channels: https://github.com/serprex/pgwire/tree/fix1 to no avail

Are you aware of gluesql having compatibility issues with tokio?

@ever0de
Copy link
Member

ever0de commented Dec 26, 2023

@panarch I created a PR to update pgwire examples to use latest gluesql, but I found that calling .await on result of execute continued to fail even after moving to storing glue in tokio::sync::Mutex. I even tried getting around having a mutex at all using channels: serprex/pgwire@fix1 to no avail

Are you aware of gluesql having compatibility issues with tokio?

hello :)
I've checked the pgwire code, and it appears that the SimpleQueryHandler requires the Send + Sync supertraits.

I think it's due to the failure to implement Send and Sync in gluesql::{GStore, GStoreMut}.

You can check by simply implementing the following async_trait.

pub struct GluesqlProcessor {
    db: SharedMemoryStorage,
}

#[async_trait(?Send)]
trait SendTest {
    async fn query(&self, query: &str);
}

#[async_trait(?Send)] // success, but `#[async_trait]` fails.
impl SendTest for GluesqlProcessor {
    async fn query(&self, query: &str) {
        let db = self.db.clone();
        let mut glue = Glue::new(db.clone());

        let result = glue.execute(query).await;
        println!("{:?}", result);
    }
}

We are working on some ideas to address this issue, but have not yet completed a functional implementation of gluesql that supports the Optional Send combination of async_trait.

Would appreciate your understanding on this point 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants