-
Notifications
You must be signed in to change notification settings - Fork 418
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
Ensure that batchParamValues is cleared in all cases when executing a batch #1869
Ensure that batchParamValues is cleared in all cases when executing a batch #1869
Conversation
It looks like a lot of changes but it's mostly whitespace as I've had to indent the whole method bodies by an extra 4 spaces to wrap them in a try block. |
Hi @ianroberts, Just a comment. We haven't forgotten about this, but for the time being priority is being given to preparing for our upcoming release. Expect to see this addressed shortly thereafter. |
…atch, whether via bulk update or via traditional batch insert. Closes microsoft#1767 Signed-off-by: Ian Roberts <i.roberts@dcs.shef.ac.uk>
Signed-off-by: Ian Roberts <i.roberts@dcs.shef.ac.uk>
7206950
to
094c584
Compare
I've cleaned up the tests slightly, but otherwise this perfectly reproduces, resolves, and confirms resolution of, the issue. Thank you so much! |
Almost ready, can we ask that you run the files through your ide, formatting using the format files included at the root. We think there may be some minor spacing issues. |
I don't have Eclipse (I'm an IntelliJ user) but I was able to apply the |
9c2b8a7
to
9ce74b3
Compare
Thank you so much. With that I can ask the rest of the team to review, and we can finally get this merged. |
We'll need to run the PR through some of our internal pipelines. |
This is an alternative implementation to #1768 which moves the
batchParamValues = null
to a new top-level try/finally block.Closes #1767