opensearch-project / k-NN

🆕 Find the k-nearest neighbors (k-NN) for your vector data
https://opensearch.org/docs/latest/search-plugins/knn/index/
Apache License 2.0
156 stars 123 forks source link

[Enhancement] Upgrade C++ version to C++ 17 in JNI #2251

Closed 0ctopus13prime closed 1 week ago

0ctopus13prime commented 2 weeks ago

Description

We are currently using C++11 for building the JNI, while Faiss is built with C++17 and NMSLIB with C++11. C++17 introduces a richer set of language features compared to C++11, making code more expressive and efficient. I won't walk through the new features but for details on the differences, please refer to this guide. One particularly useful addition is std::make_unique, which enhances memory management in a clean and concise way. We don't need to do this anymore - std::unique_ptr<knn_jni::faiss_wrapper::FaissMethods> faissMethods(new knn_jni::faiss_wrapper::FaissMethods());

Overall, using C++17 in our JNI code would make it more succinct. Upgrading is straightforward—it requires only a single line change—and it has passed all tests in my local development environment.

-set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_STANDARD 17)
vamshin commented 2 weeks ago

Does this upgrade bring any compatibility issue with Amazon Linux version we use for building the artifacts for release?

jmazanec15 commented 2 weeks ago

@0ctopus13prime I guess if we are already building faiss with C++17 it will be very safe. This looks good to me.

0ctopus13prime commented 2 weeks ago

Does this upgrade bring any compatibility issue with Amazon Linux version we use for building the artifacts for release?

I don't think so, we are already using C++17 when building FAISS. I'm sure 99% will be fine, but will test in each platform thoroughly before applying it in production.

0ctopus13prime commented 1 week ago

merged in both main and 2.x branch