JingsongLi commented on code in PR #8371: URL: https://github.com/apache/paimon/pull/8371#discussion_r3489455611
########## paimon-common/src/test/java/org/apache/paimon/globalindex/btree/ReverseBTreeEndsWithTest.java: ########## @@ -0,0 +1,190 @@ +/* + * 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.globalindex.btree; + +import org.apache.paimon.data.BinaryString; +import org.apache.paimon.fs.FileIO; +import org.apache.paimon.fs.Path; +import org.apache.paimon.fs.PositionOutputStream; +import org.apache.paimon.fs.local.LocalFileIO; +import org.apache.paimon.globalindex.GlobalIndexIOMeta; +import org.apache.paimon.globalindex.GlobalIndexReader; +import org.apache.paimon.globalindex.GlobalIndexResult; +import org.apache.paimon.globalindex.GlobalIndexSingleColumnWriter; +import org.apache.paimon.globalindex.KeySerializer; +import org.apache.paimon.globalindex.ResultEntry; +import org.apache.paimon.globalindex.ReversedKeySerializer; +import org.apache.paimon.globalindex.io.GlobalIndexFileReader; +import org.apache.paimon.globalindex.io.GlobalIndexFileWriter; +import org.apache.paimon.options.MemorySize; +import org.apache.paimon.options.Options; +import org.apache.paimon.predicate.FieldRef; +import org.apache.paimon.types.DataField; +import org.apache.paimon.types.DataType; +import org.apache.paimon.types.VarCharType; +import org.apache.paimon.utils.Pair; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.Iterator; +import java.util.List; +import java.util.Optional; +import java.util.Random; +import java.util.stream.Collectors; + +import static org.apache.paimon.shade.guava30.com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService; +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests for {@link ReverseBTreeGlobalIndexer}. */ +public class ReverseBTreeEndsWithTest { + + private static final DataType TYPE = new VarCharType(VarCharType.MAX_LENGTH); + private static final FieldRef REF = new FieldRef(1, "f", TYPE); + + @TempDir java.nio.file.Path tempPath; + + private List<Pair<Object, Long>> data; + + @Test + public void testEndsWithAndLikeAreAccelerated() throws Exception { + String[] suffixes = {"red", "green", "blue", "gold"}; + ReversedKeySerializer reversed = new ReversedKeySerializer(KeySerializer.create(TYPE)); + + try (GlobalIndexReader reader = buildIndex(reversed, suffixes)) { + for (String suffix : suffixes) { + List<Long> expected = idsEndingWith(suffix); + + GlobalIndexResult endsWith = + reader.visitEndsWith(REF, BinaryString.fromString(suffix)).join().get(); + assertThat(rowIds(endsWith)).containsExactlyInAnyOrderElementsOf(expected); + + GlobalIndexResult like = + reader.visitLike(REF, BinaryString.fromString("%" + suffix)).join().get(); + assertThat(rowIds(like)).containsExactlyInAnyOrderElementsOf(expected); + } + + Optional<GlobalIndexResult> none = + reader.visitEndsWith(REF, BinaryString.fromString("nosuchsuffix")).join(); + assertThat(none).isPresent(); + assertThat(none.get().results().isEmpty()).isTrue(); + } + } + + @Test + public void testOrderDependentPredicatesDecline() throws Exception { + String[] suffixes = {"red", "green", "blue", "gold"}; + ReversedKeySerializer reversed = new ReversedKeySerializer(KeySerializer.create(TYPE)); + + try (GlobalIndexReader reader = buildIndex(reversed, suffixes)) { + BinaryString probe = BinaryString.fromString("row5"); + assertThat(reader.visitStartsWith(REF, probe).join()).isEmpty(); + assertThat(reader.visitLessThan(REF, probe).join()).isEmpty(); + assertThat(reader.visitGreaterThan(REF, probe).join()).isEmpty(); + assertThat(reader.visitLessOrEqual(REF, probe).join()).isEmpty(); + assertThat(reader.visitGreaterOrEqual(REF, probe).join()).isEmpty(); + assertThat(reader.visitBetween(REF, probe, BinaryString.fromString("row9")).join()) + .isEmpty(); + } + } + + @Test + public void testEqualityStillWorksOnReversedIndex() throws Exception { + String[] suffixes = {"red", "green", "blue", "gold"}; + ReversedKeySerializer reversed = new ReversedKeySerializer(KeySerializer.create(TYPE)); + + try (GlobalIndexReader reader = buildIndex(reversed, suffixes)) { + BinaryString exact = (BinaryString) data.get(0).getKey(); + List<Long> expected = + data.stream() + .filter(p -> p.getKey().equals(exact)) + .map(Pair::getValue) + .collect(Collectors.toList()); + GlobalIndexResult equal = reader.visitEqual(REF, exact).join().get(); + assertThat(rowIds(equal)).containsExactlyInAnyOrderElementsOf(expected); + } + } + + private GlobalIndexReader buildIndex(ReversedKeySerializer reversed, String[] suffixes) + throws IOException { + FileIO fileIO = LocalFileIO.create(); + GlobalIndexFileWriter fileWriter = + new GlobalIndexFileWriter() { + @Override + public String newFileName(String prefix) { + return "reverse-btree-" + prefix; + } + + @Override + public PositionOutputStream newOutputStream(String fileName) + throws IOException { + return fileIO.newOutputStream( + new Path(new Path(tempPath.toUri()), fileName), true); + } + }; + GlobalIndexFileReader fileReader = + meta -> + fileIO.newInputStream( + new Path(new Path(tempPath.toUri()), meta.filePath())); + + Options options = new Options(); + options.set(BTreeIndexOptions.BTREE_INDEX_CACHE_SIZE, MemorySize.ofMebiBytes(8)); + ReverseBTreeGlobalIndexer indexer = + new ReverseBTreeGlobalIndexer(new DataField(1, "f", TYPE), options); + Comparator<Object> cmp = reversed.createComparator(); + + Random rnd = new Random(7); + data = new ArrayList<>(); + for (int i = 0; i < 20000; i++) { + String v = "row" + rnd.nextInt(1_000_000) + suffixes[rnd.nextInt(suffixes.length)]; + data.add(Pair.of(BinaryString.fromString(v), (long) i)); + } + data.sort((a, b) -> cmp.compare(a.getKey(), b.getKey())); Review Comment: [P1] Please cover the production build path before registering this index type. This test writes a valid reverse-btree only because it sorts with the reversed-key comparator here. The real create_global_index paths do not do that: reverse-btree is not in the Spark/Flink SortedIndexTopoBuilder support lists, so it falls through to the default/generic builders, and those pass rows to BTreeIndexWriter in scan order; even SortedGlobalIndexBuilder sorts by the original index field, not the reversed bytes. Since SstFileWriter/BTreeIndexWriter require keys to be monotonically increasing in the writer comparator, an index built through the normal procedure can produce an incorrectly ordered SST and wrong suffix lookups. Please wire reverse-btree into a builder that sorts by ReversedKeySerializer ordering and add an integration test for the procedure path. -- 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]
