JingsongLi commented on code in PR #9050:
URL: https://github.com/apache/paimon/pull/9050#discussion_r3721773332
##########
paimon-python/pypaimon/globalindex/data_evolution_global_index_scanner.py:
##########
@@ -103,24 +106,33 @@ def _create_evaluator(self, fields, file_io, index_path,
index_files):
options = self._options
def readers_function(field: DataField) ->
Collection[GlobalIndexReader]:
+ groups = []
group = index_metas.get(field.id)
if group is not None:
- return _create_readers(
- file_io, index_path, group.metas, field, executor, options)
+ groups.append(group)
extra_groups = extra_index_metas.get(field.id)
- if not extra_groups:
+ if extra_groups:
+ groups.extend(
+ extra_group
+ for extra_group in extra_groups
+ if extra_group not in groups
+ )
+ if not groups:
return []
+ if len(groups) == 1:
+ return _create_readers(
+ file_io, index_path, groups[0].metas, field, executor,
options)
union_coverage = Range.sort_and_merge_overlap(
[
range_key
- for group in extra_groups
+ for group in groups
for range_key in group.coverage_ranges
],
True,
)
readers = []
- for group in extra_groups:
+ for group in groups:
pad_ranges = _exclude_ranges(union_coverage,
group.coverage_ranges)
readers.extend(
_create_readers(
Review Comment:
[P1] This merge can let an unsupported alternate index poison an otherwise
usable primary reader. For example, if `c` has a dedicated BTree index and is
also a companion/extra field of a Java-built multi-column `es-index(a, c)`, the
base branch returns the BTree result, but this loop also instantiates the
`es-index` group and `_create_inner_readers` raises `ValueError` because
PyPaimon does not support that index type. `FileScanner` silently loses
pruning, while the indexed and raw vector pre-filter paths propagate the
exception and fail the query. Please make alternate selection capability-aware
while keeping coverage conservative: either exclude unreadable coverage or
represent those ranges as all-hit/fallback padding, and add a regression test
with a supported primary plus a real unsupported extra-field index.
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/GlobalIndexEvaluator.java:
##########
@@ -165,40 +175,64 @@ private CompletableFuture<Optional<GlobalIndexResult>>
visitCompoundAsync(
return CompletableFuture.allOf(childFutures.toArray(new
CompletableFuture[0]))
.thenApply(
v -> {
- List<Optional<GlobalIndexResult>> results = new
ArrayList<>();
- for
(CompletableFuture<Optional<GlobalIndexResult>> f : childFutures) {
+ List<Optional<Evaluation>> results = new
ArrayList<>();
+ for (CompletableFuture<Optional<Evaluation>> f :
childFutures) {
results.add(f.join());
}
return combineResults(results, predicate);
});
}
- private Optional<GlobalIndexResult> combineResults(
- List<Optional<GlobalIndexResult>> results, CompoundPredicate
predicate) {
+ private Optional<Evaluation> combineResults(
+ List<Optional<Evaluation>> results, CompoundPredicate predicate) {
+ Set<Integer> fieldIds = new HashSet<>();
if (predicate.function() instanceof Or) {
GlobalIndexResult compoundResult = GlobalIndexResult.createEmpty();
- for (Optional<GlobalIndexResult> childResult : results) {
- if (!childResult.isPresent()) {
+ for (Optional<Evaluation> child : results) {
+ if (!child.isPresent()) {
return Optional.empty();
}
- compoundResult = compoundResult.or(childResult.get());
+ compoundResult = compoundResult.or(child.get().result());
+ fieldIds.addAll(child.get().fieldIds());
}
- return Optional.of(compoundResult);
+ return Optional.of(new Evaluation(compoundResult, fieldIds));
} else {
Optional<GlobalIndexResult> compoundResult = Optional.empty();
- for (Optional<GlobalIndexResult> childResult : results) {
- if (childResult.isPresent()) {
+ for (Optional<Evaluation> child : results) {
+ if (child.isPresent()) {
if (compoundResult.isPresent()) {
- compoundResult =
Optional.of(compoundResult.get().and(childResult.get()));
+ compoundResult =
+
Optional.of(compoundResult.get().and(child.get().result()));
} else {
- compoundResult = childResult;
+ compoundResult = Optional.of(child.get().result());
}
+ fieldIds.addAll(child.get().fieldIds());
}
if (compoundResult.isPresent() &&
compoundResult.get().results().isEmpty()) {
- return compoundResult;
+ break;
}
}
- return compoundResult;
+ return compoundResult.map(result -> new Evaluation(result,
fieldIds));
+ }
+ }
+
+ /** Global-index matches and the fields which produced them. */
+ public static final class Evaluation {
+
+ private final GlobalIndexResult result;
+ private final Set<Integer> fieldIds;
Review Comment:
Non-blocking: could we rename `fieldIds` to `contributingFieldIds` (and keep
the Python field/method names in sync)? This set intentionally excludes
unsupported or discarded branches; it is not the full predicate field set or
every field that was evaluated. The provenance distinction is the core contract
of this fix, and these APIs are introduced in this PR, so naming it explicitly
now would make future coverage call sites much harder to misuse. Suggested
contract: “Field IDs whose supported index results were combined into the
returned candidate; unsupported or discarded branches are excluded.”
--
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]
