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

Rust module API rework #1660

Merged
merged 13 commits into from
Sep 27, 2024
Merged

Rust module API rework #1660

merged 13 commits into from
Sep 27, 2024

Conversation

coolreader18
Copy link
Collaborator

WIP - waiting on abi rework implementation.

Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

  • You still have the macros looking for known concrete types for columns marked autoinc.
  • I would really appreciate some comments and docs on the macros. In particular, listing some examples of expected inputs and outputs, and some explanation of why the different parts of the outputs are there and what parts of the runtime system they plug into.

crates/bindings-macro/src/lib.rs Outdated Show resolved Hide resolved
crates/bindings-macro/src/lib.rs Outdated Show resolved Hide resolved
crates/bindings-macro/src/lib.rs Outdated Show resolved Hide resolved
crates/bindings-macro/src/lib.rs Outdated Show resolved Hide resolved
crates/bindings-macro/src/lib.rs Outdated Show resolved Hide resolved
crates/bindings/src/lib.rs Outdated Show resolved Hide resolved
crates/bindings/src/rt.rs Outdated Show resolved Hide resolved
crates/bindings/src/table.rs Show resolved Hide resolved
crates/bindings/src/table.rs Outdated Show resolved Hide resolved
Comment on lines 288 to 298
impl<T> BTreeIndexBoundsTerminator<T> for T {}
impl<T> BTreeIndexBoundsTerminator<T> for &T {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::Range<T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeFrom<&T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeFrom<T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeInclusive<&T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeInclusive<T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeTo<&T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeTo<T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeToInclusive<&T> {}
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeToInclusive<T> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to discuss (on the proposal) whether we actually want to accept all combinations of T and &T.

@coolreader18 coolreader18 force-pushed the noa/rust-module-api branch 5 times, most recently from 1a7f7a1 to a639404 Compare September 10, 2024 16:13
@coolreader18
Copy link
Collaborator Author

Currently just blocked on #1704.

@coolreader18 coolreader18 force-pushed the noa/rust-module-api branch 2 times, most recently from 1ffff46 to 3b6f8a2 Compare September 19, 2024 22:27
@coolreader18
Copy link
Collaborator Author

I guess now it's just blocked on an implementation in ModuleHost

@coolreader18 coolreader18 force-pushed the noa/rust-module-api branch 2 times, most recently from 9b6afb6 to eff0e7e Compare September 19, 2024 22:40
@coolreader18
Copy link
Collaborator Author

The merge is just for testing - I'll rebase once #1697 actually merges.

@coolreader18 coolreader18 force-pushed the noa/rust-module-api branch 3 times, most recently from 64d9963 to 965def9 Compare September 26, 2024 18:11
@coolreader18
Copy link
Collaborator Author

@Centril do you know why this might be failing? In BTreeIndex::filter():

        let index_id = Idx::index_id();
        let args = b.get_args();
        let (prefix, prefix_elems, rstart, rend) = args.args_for_syscall();
        let iter = sys::datastore_btree_scan_bsatn(index_id, prefix, prefix_elems, rstart, rend)
            .unwrap_or_else(|e| panic!("unexpected error from datastore_btree_scan_bsatn: {e}"));
        TableIter::new(iter)

It panics not at the lookup of the index id by name (searches the system tables), but in datastore_btree_scan_bsatn with "No such index" (searches index_id_map). Is the index not getting properly committed to the database state? As far as I can tell it is, but I don't know why else this would be failing.

@coolreader18 coolreader18 marked this pull request as ready for review September 26, 2024 18:11
@coolreader18 coolreader18 mentioned this pull request Sep 26, 2024
@coolreader18
Copy link
Collaborator Author

gruhhh.... the tests are failing because the rust module now declares snake_case table names but the csharp module doesn't

@coolreader18
Copy link
Collaborator Author

theoretically this is all tests passing, besides that

@coolreader18
Copy link
Collaborator Author

maybe

@coolreader18
Copy link
Collaborator Author

yeah, rust tests are all passing for me locally

@coolreader18
Copy link
Collaborator Author

Reviewer's discretion whether this is an acceptable solution, I guess.

@gefjon
Copy link
Contributor

gefjon commented Sep 26, 2024

Can you alter the C# module to define snake_case table names by changing class MyTable to class my_table?

@gefjon gefjon self-requested a review September 26, 2024 22:24
@gefjon
Copy link
Contributor

gefjon commented Sep 27, 2024

Don't merge until we have a companion PR for private.

@coolreader18 coolreader18 added this pull request to the merge queue Sep 27, 2024
Merged via the queue into master with commit 5375842 Sep 27, 2024
7 of 8 checks passed
@coolreader18 coolreader18 deleted the noa/rust-module-api branch October 8, 2024 20:53
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.

2 participants