SteNicholas commented on code in PR #461: URL: https://github.com/apache/paimon-cpp/pull/461#discussion_r4417267787
########## src/paimon/core/table/source/index_full_text_search_split_impl.h: ########## @@ -0,0 +1,107 @@ +/* + * 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. + */ + +#pragma once + +#include <cstdint> +#include <memory> +#include <string> +#include <utility> +#include <vector> + +#include "paimon/core/index/index_file_meta.h" +#include "paimon/result.h" +#include "paimon/table/source/full_text_search_split.h" +#include "paimon/utils/range.h" + +namespace paimon { +/// The implementation of `IndexFullTextSearchSplit`, which also holds the full-text index files of +/// the shard. +class IndexFullTextSearchSplitImpl : public IndexFullTextSearchSplit { + public: + /// "PFTSPLT1" in ASCII. + static constexpr int64_t MAGIC = 0x50465453504C5431L; + static constexpr int32_t VERSION = 1; Review Comment: Thanks. There is no cross-language format to stay compatible with here. Java `IndexFullTextSearchSplit` is a `java.io.Serializable` written by `ObjectOutputStream` (`defaultWriteObject()`, then a version int and the `IndexFileMetaSerializer` lists), so its bytes are a Java object stream with class descriptors, and the pypaimon `IndexFullTextSearchSplit` is a plain dataclass with no serialized form. As #404 notes, C++ therefore defines its own versioned encoding, and `full_text_search_split.h` and the user guide state that it is specific to Paimon C++ and cannot be exchanged with Java. What is shared is the index file layout: the entries use the same `IndexFileMetaSerializer` row layout as Java. In 83a29ba6, `TestSerializeRoundTrip` pins the complete version 1 bytes instead of building the index file part with the current serializer, and those index file bytes were cross-checked against the output of Java `IndexFileMetaSerializer#serializeList`, so a change of the format can no longer pass the test silently. A plan exchanged across languages would first need a language-neutral split format on the Java side. ########## src/paimon/core/table/source/index_full_text_search_split_impl.h: ########## @@ -0,0 +1,107 @@ +/* + * 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. + */ + +#pragma once + +#include <cstdint> +#include <memory> +#include <string> +#include <utility> +#include <vector> + +#include "paimon/core/index/index_file_meta.h" +#include "paimon/result.h" +#include "paimon/table/source/full_text_search_split.h" +#include "paimon/utils/range.h" + +namespace paimon { +/// The implementation of `IndexFullTextSearchSplit`, which also holds the full-text index files of +/// the shard. +class IndexFullTextSearchSplitImpl : public IndexFullTextSearchSplit { + public: + /// "PFTSPLT1" in ASCII. + static constexpr int64_t MAGIC = 0x50465453504C5431L; + static constexpr int32_t VERSION = 1; Review Comment: Yes, that is intentionally out of scope. Version 2 of Java `IndexFullTextSearchSplit` appends the scalar index files that pre-filter rows for `withFilter`, and row filters are tracked separately in #405. Since the C++ encoding has its own version numbers, there is nothing to mirror before then. In 83a29ba6 there is a TODO at `IndexFullTextSearchSplitImpl::VERSION` to add those files in a new version once row filters are supported, and the user guide states that row filters are not supported yet. ########## src/paimon/common/global_index/scored_global_index_result_utils.h: ########## @@ -0,0 +1,46 @@ +/* + * 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. + */ + +#pragma once + +#include <cstdint> +#include <memory> +#include <vector> + +#include "paimon/global_index/global_index_result.h" +#include "paimon/result.h" + +namespace paimon { +/// Helpers that combine `ScoredGlobalIndexResult`s. +class ScoredGlobalIndexResultUtils { Review Comment: Good catch, fixed in 83a29ba6: `scored_global_index_result_utils.cpp` is now also listed in `PAIMON_GLOBAL_INDEX_SRC`, so `paimon_global_index` compiles its own copy of `MergeTopK`, as it already does for `union_global_index_reader.cpp` and `offset_global_index_reader.cpp`. ########## src/paimon/core/table/source/data_evolution_full_text_scan.cpp: ########## @@ -0,0 +1,328 @@ +/* + * 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. + */ + +#include "paimon/core/table/source/data_evolution_full_text_scan.h" + +#include <algorithm> +#include <cassert> +#include <functional> +#include <iterator> +#include <limits> +#include <map> +#include <optional> +#include <set> +#include <string> +#include <tuple> +#include <unordered_map> +#include <utility> + +#include "fmt/format.h" +#include "fmt/ranges.h" +#include "paimon/core/index/index_file_handler.h" +#include "paimon/core/manifest/index_manifest_entry.h" +#include "paimon/core/manifest/index_manifest_file.h" +#include "paimon/core/snapshot.h" +#include "paimon/core/table/source/index_full_text_search_split_impl.h" +#include "paimon/core/tag/tag.h" +#include "paimon/core/utils/branch_manager.h" +#include "paimon/core/utils/index_file_path_factories.h" +#include "paimon/core/utils/snapshot_manager.h" +#include "paimon/core/utils/tag_manager.h" +#include "paimon/defs.h" +#include "paimon/global_index/full_text/full_text_defs.h" +#include "paimon/global_index/global_indexer.h" +#include "paimon/global_index/global_indexer_factory.h" +#include "paimon/global_index/lucene/lucene_defs.h" + +namespace paimon { +namespace { +/// The files of one index shard that may rank rows of the searched field. +struct IndexRangeCandidate { + Range file_range; + std::string identity; + /// Whether the searched field is the indexed field of the index, not an extra field. + bool primary; + std::vector<std::shared_ptr<IndexFileMeta>> files; + /// The rows assigned to this shard, merged as they are assigned in ascending order. + std::vector<Range> assigned; +}; + +/// Resolves the snapshot to search as Java `TimeTravelUtil#tryTravelOrLatest` does: the one that +/// the time-travel option selects, whatever `scan.mode` is, or else the latest one. Review Comment: Added in 83a29ba6. The TODO on `ResolveSnapshot` in `data_evolution_full_text_scan.cpp` states that the column, the partition keys and the options still come from the latest schema instead of the schema of the selected snapshot, as Java time travel uses, so a column dropped and added again with the same name since that snapshot finds no index files. The user guide also says that the search uses the latest schema of the table, not the schema of the snapshot. -- 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]
