-
Notifications
You must be signed in to change notification settings - Fork 90
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
Support SQL Server binary data types #687
Conversation
@javornikolov I can't see any Travis builds running for this branch/push or PR. Do you know what the problem might be? |
I contacted the Travis CO support. At first glimpse seems the same issue - running out of credits. I didn't expect 25k credits to finish that quickly... |
@MMatten, builds should work now. I am not sure whether it might need manual triggering for an existing pr or branch. |
Thanks @javornikolov I've had a (brief) search around and I'm still not quite sure if manually triggering builds for existing PRs/pushes is supported. I've got some tweaks to add to this PR anyway so I'll see what happens then. |
I'll ensure that we close #386 as part of this too. |
Maybe pushing something to the branch or force-pusshing the same... |
a593d07
to
6b6504a
Compare
@javornikolov thanks. I've pushed more changes and we're building again. Will be interesting to see how fast we're going to be burning build credits. I'll check shortly. |
@javornikolov I'm seeing some new JUnit deprecation warnings. I'll look to deal with these on a different PR (maybe bump up to JUnit 5 too on that one). I have to at least test this change across all of the adapters as there's that change to the core before marking this as ready for a review. |
@javornikolov I've now tested this across all of the adapters, apart from Snowflake as I have no usable environment (I have reached out to Snowflake to see if they'll offer us any long-term support though). |
@javornikolov one observation on the Snowflake situation...it seems that we didn't inherit any tests for Snowflake anyway. |
@javornikolov any review comments or do you think we can merge this (after any rebasing)? |
@MMatten - looks reasonably good to me. Just see if we can reuse hex parsing, etc. from a library which we already use - as per my comment. You can push to the PR to trigger a build (sqlserver tests are already available in our CI). |
4701f6a
to
9eb343f
Compare
@javornikolov so apart from squashing commits, etc, any other things you've spotted? We could always do with more unit tests of course! |
8aec4d5
to
1602c1b
Compare
1602c1b
to
6e317e9
Compare
@javornikolov updates applied. I think we could be ready to merge following a final review (please). |
Thank you @MMatten!. Yes, it looks good to me to merge it. 👍 |
Provide base class for binary data type support.
Provide support for the following SQL Server data types: -
Resolves #386.