SteNicholas commented on code in PR #431:
URL: https://github.com/apache/paimon-cpp/pull/431#discussion_r4310656040
##########
src/paimon/common/global_index/offset_global_index_reader.h:
##########
@@ -31,11 +31,22 @@ namespace paimon {
/// results. This is used to convert local row IDs into global row IDs.
class OffsetGlobalIndexReader : public GlobalIndexReader {
public:
- /// Constructs an OffsetGlobalIndexReader.
+ /// Constructs an OffsetGlobalIndexReader whose full-text searches rank
all rows it covers.
/// @param wrapped The inner reader to delegate queries to. Its results
are expected to
/// contain local row ids.
- /// @param offset The offset to add to each row id in the results.
- OffsetGlobalIndexReader(std::shared_ptr<GlobalIndexReader>&& wrapped,
int64_t offset);
Review Comment:
Agreed, it is not specific to full-text search. The constructor comment no
longer mentions full-text search: it documents `offset` and `to` as the first
and last global row ids covered by the wrapped reader, which every visit uses.
How `include_row_ids` is clipped to `[offset, to]` is commented in
`VisitFullTextSearch`.
##########
src/paimon/common/global_index/offset_global_index_reader.h:
##########
@@ -31,11 +31,22 @@ namespace paimon {
/// results. This is used to convert local row IDs into global row IDs.
class OffsetGlobalIndexReader : public GlobalIndexReader {
public:
- /// Constructs an OffsetGlobalIndexReader.
+ /// Constructs an OffsetGlobalIndexReader whose full-text searches rank
all rows it covers.
/// @param wrapped The inner reader to delegate queries to. Its results
are expected to
/// contain local row ids.
- /// @param offset The offset to add to each row id in the results.
- OffsetGlobalIndexReader(std::shared_ptr<GlobalIndexReader>&& wrapped,
int64_t offset);
+ /// @param offset The first global row id covered by the wrapped reader,
added to each row id
+ /// in the results.
+ /// @param to The last global row id (inclusive) covered by the
wrapped reader.
+ OffsetGlobalIndexReader(std::shared_ptr<GlobalIndexReader>&& wrapped,
int64_t offset,
+ int64_t to);
+
Review Comment:
Removed. `OffsetGlobalIndexReader` now has one constructor, `(wrapped,
offset, to)`, the same as Java `OffsetGlobalIndexReader(wrapped, offset, to)`,
and `GlobalIndexScanImpl` uses it for every shard.
On `chooseIndexRanges`: as you pointed out, Java passes the rows a shard
ranks as `includeRowIds` (`DataEvolutionFullTextRead#includeRowIds`), so
`OffsetGlobalIndexReader` no longer keeps them. `GlobalIndexScanImpl` now gives
`UnionGlobalIndexReader` the row range of each shard and the rows it ranks; the
union reader restricts the shard through `include_row_ids` and skips a shard
with no included row left.
The assignment itself stays simpler than Java's. `chooseIndexRanges` splits
the coverage of a column at every range boundary and gives each interval to the
best active candidate: an index whose primary field is the column first, then
by index identity, `from` and `to`. A C++ `UnionGlobalIndexReader` serves one
field and one index type, so its candidates share one identity and the choice
reduces to the first range covering a row in `(from, to)` order. Each shard
then ranks one contiguous tail of its range, `[max(from, covered_to + 1), to]`,
where `covered_to` is the largest `to` of earlier ranges, so the multi-segment
search ranges that Java merges with `mergeAdjacent` cannot occur. The PR
description now says the core semantics align with Java rather than calling
this a full port.
--
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]
