-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix sequence deletion after nextval call #26
base: master
Are you sure you want to change the base?
Conversation
agscpp
commented
Sep 24, 2024
Hi, @gmoshkin . Can you please take a look at my pull request? |
Hi, thanks for the patch! Could you explain, how are the provided SQL commands related to the test?
The code in question will not be invoked when this SQL is executed. Could you also add a test, so that we don't break the fix accidentally in the future. |
This is an abstract example that I used to compare the behavior with other DBMSs. Okay, I'll write tests for this logic. |
2c81c42
to
8cca8d2
Compare
@gmoshkin |
Also please update |
8cca8d2
to
f1187e6
Compare
tests/src/box.rs
Outdated
let mut seq = Sequence::find("test_drop_seq").unwrap().unwrap(); | ||
assert_eq!(seq.next().unwrap(), 1); | ||
|
||
tarantool::schema::sequence::drop_sequence(2).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the test! There's a problem though, this hard-coded id is volatile as it may change if another test is added (if someone adds another sequence). We could make it much more robust if we got the id from the Sequence
struct. Unfortunately there's currently no method for this, but it's very easy to add it.
Do you mind adding fn id(&self) -> u32
for the Sequence struct and using it in this test?
This will also be potentially useful for other users of tarantool-module as it will allow explicit deletion of sequences, by adding the ability to get the id of the instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I wrote this comment yesterday, but failed to submit the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have corrected the code based on your comments. Please take a look again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f1187e6
to
b45cd7c
Compare
@gmoshkin |