Copilot commented on code in PR #6985: URL: https://github.com/apache/paimon/pull/6985#discussion_r2674706440
########## paimon-faiss/paimon-faiss-jni/src/main/native/CMakeLists.txt: ########## @@ -0,0 +1,457 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +cmake_minimum_required(VERSION 3.30.1) Review Comment: Requiring exactly CMake version 3.30.1 is overly restrictive. This should use a minimum version requirement (e.g., `cmake_minimum_required(VERSION 3.17)`) rather than an exact version to allow compatibility with newer CMake versions and avoid forcing users to downgrade. ```suggestion cmake_minimum_required(VERSION 3.17) ``` ########## paimon-faiss/paimon-faiss-jni/src/main/java/org/apache/paimon/faiss/Faiss.java: ########## @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.faiss; + +/** Global Faiss configuration and utilities. */ +public final class Faiss { + + static { + try { + NativeLibraryLoader.load(); + } catch (FaissException e) { + throw new ExceptionInInitializerError(e); + } + } + Review Comment: Missing JavaDoc for the public `getVersion()` method. All public API methods should have documentation explaining their purpose and return values. ```suggestion /** * Get the version of the underlying Faiss library. * * @return the Faiss library version string */ ``` ########## paimon-faiss/paimon-faiss-jni/src/test/java/org/apache/paimon/faiss/IndexTest.java: ########## @@ -0,0 +1,456 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.faiss; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.File; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.FloatBuffer; +import java.nio.file.Path; +import java.util.Random; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests for the Faiss Index class. + * + * <p>Note: These tests require the native library to be built and available. They will be skipped + * if the native library is not found. + */ +class IndexTest { + + private static final int DIMENSION = 128; + private static final int NUM_VECTORS = 1000; + private static final int K = 10; + + @Test + void testFlatIndexBasicOperations() { + try (Index index = createFlatIndexWithMetric(MetricType.L2)) { + assertEquals(DIMENSION, index.getDimension()); + assertEquals(0, index.getCount()); + assertTrue(index.isTrained()); + assertEquals(MetricType.L2, index.getMetricType()); + + // Add vectors using zero-copy API + ByteBuffer vectorBuffer = createVectorBuffer(NUM_VECTORS, DIMENSION); + index.add(NUM_VECTORS, vectorBuffer); + assertEquals(NUM_VECTORS, index.getCount()); + + // Search using array API + float[] queryVectors = createQueryVectors(1, DIMENSION); + float[] distances = new float[K]; + long[] labels = new long[K]; + + index.search(1, queryVectors, K, distances, labels); + + // Verify labels are in valid range + for (int i = 0; i < K; i++) { + assertTrue( + labels[i] >= 0 && labels[i] < NUM_VECTORS, + "Label " + labels[i] + " out of range"); + } + + // Verify distances are non-negative for L2 + for (int i = 0; i < K; i++) { + assertTrue(distances[i] >= 0, "Distance should be non-negative for L2"); + } + } + } + + @Test + void testFlatIndexWithIds() { + try (Index index = IndexFactory.create(DIMENSION, "IDMap,Flat", MetricType.L2)) { + ByteBuffer vectorBuffer = createVectorBuffer(NUM_VECTORS, DIMENSION); + ByteBuffer idBuffer = Index.allocateIdBuffer(NUM_VECTORS); + idBuffer.asLongBuffer(); Review Comment: The result of `idBuffer.asLongBuffer()` is not used. This statement has no effect and should be removed. ```suggestion ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
