SteNicholas commented on code in PR #431:
URL: https://github.com/apache/paimon-cpp/pull/431#discussion_r4311502121
##########
src/paimon/common/global_index/union_global_index_reader.h:
##########
@@ -35,8 +37,15 @@ namespace paimon {
/// collected in submission order. Otherwise, sub-readers are evaluated
sequentially.
class UnionGlobalIndexReader : public GlobalIndexReader {
public:
+ /// Constructs a UnionGlobalIndexReader.
+ /// @param row_ranges The global row ids covered by each reader, in the
order of `readers`.
+ /// @param search_ranges The global row ids each reader ranks in a
full-text search, within its
+ /// row range, in the order of `readers`;
std::nullopt if it ranks none.
+ /// If empty, each reader ranks all of its rows.
UnionGlobalIndexReader(std::vector<std::shared_ptr<GlobalIndexReader>>&&
readers,
- const std::shared_ptr<Executor>& executor);
+ const std::shared_ptr<Executor>& executor,
+ std::vector<Range>&& row_ranges = {},
+ std::vector<std::optional<Range>>&& search_ranges =
{});
Review Comment:
Done. The constructor is now `UnionGlobalIndexReader(readers, row_ranges,
search_ranges, executor)`, with no default arguments. `GlobalIndexScanImpl`
passes all four, and tests that don't use full-text ranges pass
`/*row_ranges=*/{}, /*search_ranges=*/{}` explicitly.
##########
src/paimon/common/global_index/union_global_index_reader.cpp:
##########
@@ -133,9 +216,74 @@ Result<std::shared_ptr<ScoredGlobalIndexResult>>
UnionGlobalIndexReader::VisitVe
Result<std::shared_ptr<ScoredGlobalIndexResult>>
UnionGlobalIndexReader::VisitFullTextSearch(
const std::shared_ptr<FullTextSearch>& full_text_search) {
- return UnionScored([full_text_search](const
std::shared_ptr<GlobalIndexReader>& reader) {
- return reader->VisitFullTextSearch(full_text_search);
- });
+ if (!full_text_search) {
+ return Status::Invalid("VisitFullTextSearch: null FullTextSearch
pointer");
+ }
+ if (full_text_search->limit <= 0) {
+ return Status::Invalid("full-text search requires a positive limit");
+ }
+ // If the readers rank disjoint rows, the top `limit` rows of each reader
include all of its
+ // rows scoring above the `limit`-th highest score of all readers, so
merging them loses none
+ // of those rows. Rows tied at that score may already have been cut by a
reader's engine.
+ auto reader_searches =
std::make_shared<std::vector<std::shared_ptr<FullTextSearch>>>();
+ reader_searches->reserve(readers_.size());
+ for (size_t index = 0; index < readers_.size(); ++index) {
+ reader_searches->push_back(FullTextSearchForReader(index,
full_text_search));
+ }
+ auto results =
ExecuteAllReadersWithIndex<Result<std::shared_ptr<ScoredGlobalIndexResult>>>(
+ [reader_searches](size_t index, const
std::shared_ptr<GlobalIndexReader>& reader)
+ -> Result<std::shared_ptr<ScoredGlobalIndexResult>> {
+ const std::shared_ptr<FullTextSearch>& reader_search =
(*reader_searches)[index];
+ if (!reader_search) {
+ return EmptyScoredResult();
+ }
+ return reader->VisitFullTextSearch(reader_search);
+ });
+ std::vector<std::shared_ptr<ScoredGlobalIndexResult>> scored_results;
+ scored_results.reserve(results.size());
+ for (auto& result_or_status : results) {
+ PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<ScoredGlobalIndexResult> result,
+ std::move(result_or_status));
+ if (result != nullptr) {
+ scored_results.push_back(std::move(result));
+ }
+ }
+ if (scored_results.empty()) {
+ return std::shared_ptr<ScoredGlobalIndexResult>();
+ }
Review Comment:
`scored_results` is empty only when no reader returned a result: the union
has no readers, or every reader returned nullptr. The C++ full-text readers
(`FullTextGlobalIndexReader`, `LuceneGlobalIndexReader`) always return a
result, an empty one if nothing matches. Only readers without full-text support
return nullptr, such as the Lumina reader and `FileIndexReaderWrapper`. So
nullptr here means what you describe: the index wasn't used. Returning an empty
result would tell the caller that no row matched.
Java returns `createEmpty()` when a shard has no rows to search.
`NativeFullTextGlobalIndexReader` does this for an empty include set, and
`DataEvolutionFullTextRead#evalColumnQuery` merges skipped splits into it. C++
does the same: a shard that ranks none of the rows to search is not visited and
adds `EmptyScoredResult()`, so the merge returns an empty result, not nullptr.
`TestVisitFullTextSearchSparseIncludeInLargeRanges` and the `{5}` case in
`TestVisitFullTextSearchWithinSearchRanges` cover this. The nullptr case
matches Java's `UnionGlobalIndexReader#visitVectorSearch`, which returns
`Optional.empty()` when no reader returns a result.
I documented this on `VisitFullTextSearch` and commented that branch.
--
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]
