use stock khash from attractivechaos/klib

[ARF1]

fixes MSVC C++ compile errors & allows easier updates of khash in future

[CarstVaartjes]

@ARF1 Thanks! @FrancescElies can you check it against the test scripts?

[FrancescElies]

Travis says is fine https://travis-ci.org/visualfabriq/bquery/builds/54074222, but I'll try it manually in one of our servers too.

[FrancescElies]

@ARF1 would it be possible to change the end of line CRLF to LF (current github version)?
To see the diff properly one needs to do it locally (maybe is there a trick for github?) this way we should be able to see in github too which lines really changed.

[ARF1]

@FrancescElies I was wondering why the comparison was not working. Fixed now.

Where did the current khash come from? Somebody seems to have spent quite a bit of time cherry picking modifications pandas seems to have made to khash v0.2.6 (and earlier) and applied them to v0.2.8.

It might be worth understanding why pandas changed the __ac_isempty etc. functions. I looked into it but could not make heads or tails of it.

[FrancescElies]

Thanks! it looks much better now.

First we took these changes from pandas and applied them to the new version of klib https://github.com/CarstVaartjes/khash (atm we are not using this any more), as you correctly noticed we just replicated what pandas team did, we are no klib experts.

If you want to have a look how somebody with much more deep understanding of this library is updating klib you could have a look at pandas-dev/pandas#8547

I completely agree it's worth understanding why did they change that but is going to take a while though, I guess they did it to reduce memory-footprint removing unused flags, but honestly I don't really know.

[ARF1]

Thanks for the explanation and the reference. I am not up to understanding the pandas modifications. Those code sections look like chinese to me. I was mainly bothered that the bquery version would not compile on MSVC while the pandas version did.

If you prefer to keep the cherry-picked version, I believe a single line addition will fix the MSVC compile errors:

Adding #endif after the following block does the trick if I remember correctly:

#ifdef _MSC_VER
#define kh_inline __inline
#else
#define kh_inline inline

[FrancescElies]

Honestly I am not able to follow those changes neither, I don't know what what should be done, as far as I can see tests went well and I had no problems runing some tests in our servers, hopefully that's good enough.
If the rest of the people involved agree, I am in in for merging these changes as they are at the moment in this PR, if we face any unexpected problems we could always revert, would this be ok?

[ARF1]

Whatever you choose is ok, with me. I have no preference either way provided we fix the MSVC issue.

[FrancescElies]

Let's merge 👍 and keep an eye on it if we see any strange stuff

[CarstVaartjes]

I'm greatly in favour of stock khash by the way