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

Proposed fix for Optional RestartSequenceOperation.StartValue #26560 #29346

Merged
merged 6 commits into from
Mar 30, 2023

Conversation

dmihaita
Copy link
Contributor

@dmihaita dmihaita commented Oct 13, 2022

Modified the RestartSequenceOperation.StartValue's type from long to Nullable and adapted the dependencies.

Closes #26560

@dnfadmin
Copy link

dnfadmin commented Oct 13, 2022

CLA assistant check
All CLA requirements met.

roji
roji previously requested changes Oct 13, 2022
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Can you please add two tests in MigrationsTestBase for restarting a sequence with and without a value (seems like we're missing value there)? You can look at Alter_sequence_increment_by as a basis for that. These would need to be overridden in MigrationsSqlServerTest to assert on the actual SQL generated, and also in MigrationsSqliteTest, which should simply assert not supported (since SQLite doesn't support sequences).

If you're having trouble figuring it out, let me know.

If this is

@dmihaita
Copy link
Contributor Author

Suggestions applied.
Adding the two unit tests revealed that the functionality was not complete ... In order to manage to pass the RESTART without WITH clause, I had to add a new property to ISequence interface (InternalStartValue), and use it only for RestartSequenceOperation, on Diff. Not sure it's the most proper way to establish if the startValue was specified or not.

dmihaita and others added 3 commits October 31, 2022 20:07
Added unit tests for Restart operation (with and without startsAt parameter)
@dmihaita
Copy link
Contributor Author

dmihaita commented Nov 8, 2022

I think the scope of this issue was covered with the latest commit.

Reverted all changes to Sequence and related.
Adjusted the unit tests (removed the unit test that checked the operation creation through diff, added two unit-tests that cover the query generation based on restart operation definition)

@ajcvickers ajcvickers assigned bricelam and unassigned roji Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional RestartSequenceOperation.StartValue
4 participants