Repository navigation
Conversation
Akash3121
left a comment
There was a problem hiding this comment.
Could we track completeness at predicate/leaf level before enabling this grouped path? Evaluation.contributingFieldIds() cannot distinguish a fully evaluated filter from a partially evaluated conjunction whose leaves reference the same field. For example, with grouped ES index (embedding, category, price) and filter price > 0 AND isNaN(price), ES evaluates the range but declines isNaN; the range still contributes the price field ID, so FilteredRowIdReader.isExact() returns true and the vector/full-text prefilter skips data refinement. The search can therefore return rows that fail the complete filter (and in vector search they may also displace valid top-K rows). I reproduced this with real ES archive tests for both vector and full-text searches: each returned a non-empty result although the complete filter matches no rows. Please carry an explicit fully-evaluated/leaf-level completeness signal and add same-field mixed-supported/unsupported regression coverage for both search paths.
JingsongLi
left a comment
There was a problem hiding this comment.
The grouped scalar-index use case has end-to-end value. I ran the targeted JDK 11 reactor with normal Maven checks: 380 unique tests passed (38 common, 252 core, 90 ESLib).
Additional real-archive/table probes confirm the existing same-field partial-conjunction concern: price > 0 AND isNaN(price) has no matching data rows but returns a vector hit and three full-text hits. I have not duplicated that inline comment.
The distinct finding below needs only a single keyword range predicate. It also fails with scalar-index.search-mode=full and global-index.filter.refine-from-data=true. Reusing the exact parent ES factory (without the new grouped scalar opt-in) restores the expected results for both probes.
| if (group.extraFields.isEmpty()) { | ||
| // Composite-key indexes require their own planner, not independent field lookups. | ||
| if (group.extraFields.isEmpty() | ||
| || GlobalIndexerFactoryUtils.supportsGroupedIndexScalarQueries(group.type)) { |
There was a problem hiding this comment.
[P1] Refine conservative ES candidates before search ranking
Enabling grouped ES here also admits predicates for which the existing reader returns an unlabeled superset. For category > 'c1' on a keyword field, canEvaluateFilter returns false and dispatchFilter returns conservativeAllRows(). That nonempty Optional is credited with full evaluated coverage and the category field ID, so FilteredRowIdReader.isExact treats the single GreaterThan as exact and skips refinement before Top-K.
I reproduced this with this PR's 12-row ES table fixture and real (embedding,category,price) / (note,category,price) archives, using FULL mode and refine-from-data=true. The data oracle matches IDs {2,8,11}; a vector(5), limit=1 query returns ID 5 (null category) instead of ID 2. Full-text keep should return {2,8}, but returns {0,2,4,6,8,10}. Using the parent ES factory, which excludes grouped scalar lookups, restores both expected results.
Please make unsupported reader operations return unsupported, or propagate candidate/exact status through evaluation and force refinement/exclusion before ranking. Field IDs and covered row ranges alone do not prove that the complete predicate was evaluated exactly.
There was a problem hiding this comment.
Thanks, confirmed and fixed in 8e12fe3cb6.
Unsupported ES scalar operations now return Optional.empty() instead of conservativeAllRows(), including the guarded capability paths and runtime unsupported-operation fallbacks. A supported query with zero hits still returns an empty bitmap inside a present Optional. Negation propagates unsupported rather than complementing an unlabeled superset. Such results no longer claim evaluated coverage or exactness.
The real-archive regressions cover ordinary scalar reads, coverage, AND/OR, and vector/full-text prefilters. With the FULL/refine-enabled fixture described here, the scalar oracle is {2,8,11}, vector top-1 is 2, and full-text keep returns {2,8}. Same-field partial conjunctions are covered as well.
Exercising the unsupported-filter fallback also exposed a metric issue: raw vector scoring could use current table options instead of the persisted index metric. The fallback now retains the persisted metric while preserving each backend's existing shard-conflict policy; cosine-vs-L2 and backend-policy regressions are included.
This is a correctness-only fix: ESLib stays at 1.0.7, and keyword-range capability expansion is deferred to a separate change. The targeted clean reactor passed 458 tests, 0 failures/errors/skips, with normal Maven checks enabled. This is not a claim that full GitHub CI or cloud E2E has passed.
Propagate explicit predicate completeness through evaluation, ranges, reader wrappers and planning so partial same-field filters cannot bypass refinement. Return unsupported for unsupported ESLib scalar operations, distinguish zero hits, and preserve persisted vector metrics when falling back to data without changing backend conflict policies. Add scalar, coverage, BTree, vector and full-text regressions. Keep ESLib 1.0.7 and the existing vector-primary design.
|
@Akash3121 Thanks for the reproduction in this review. Addressed in 8e12fe3cb6. Field coverage cannot establish predicate completeness: Partial predicates therefore cannot bypass the existing pre-ranking refinement/exclusion rules. Coverage is credited only to groups that actually evaluated the query. BTree index selection is unchanged, and its fused range evaluation is retained. The serialized query format and vector-primary-column design are also unchanged. Added same-field supported/unsupported regressions with real ES archives for both vector and full-text queries, plus framework, wrapper, coverage and BTree regressions. The separate unsupported-keyword-range finding is addressed in the same correctness update and replied to in its thread. This does not add new ESLib query capabilities or upgrade SDK 1.0.7. Validation of the exact committed source: 458 targeted tests passed, 0 failures/errors/skips (common 56, core 274, ESLib 122, Lumina 6), with a JDK 11 |
JingsongLi
left a comment
There was a problem hiding this comment.
The previous unsupported-keyword-range and same-field partial-conjunction findings are addressed by the completeness changes and real-archive regressions. The targeted normal-check reactor passes. An additional real-archive regression found a distinct filtered-vector failure when row ranges use different grouped definitions; a controlled parent-factory comparison passes.
| .filter( | ||
| file -> | ||
| !isMultiFieldIndex(file) | ||
| || GlobalIndexerFactoryUtils |
There was a problem hiding this comment.
[P2] Keep vector readers separate for different grouped definitions
Admitting grouped scalar filters here makes a previously working filtered vector query take the ANN path, which still creates one indexer from the first split and reuses it for every definition. I reproduced this with the real ESIndexScalarTableTest fixture: build rows [0,6) as (embedding, category, price) and [6,12) as (embedding, category), then search vector(5) with category = 'c1' and limit 1. Instead of returning row 4, the second archive fails with Invalid es-index metadata, caused by ES index field count does not match current schema: indexed=2, current=3. Replacing only the ES factory with its exact parent version restores row 4 for the same test, since the grouped scalar path then uses raw fallback.
Please create/reuse the normal ANN indexer by index type and ordered field definition, as the new fallback-metric path and existing full-text path already do, or fall back when these definitions differ. Add this supported-equality case alongside the new unsupported-filter test across different definitions; that test currently exercises only the raw path.
Purpose
Allow ordinary scalar predicates to use grouped indexes, where the indexed fields can be queried independently, without treating them as ordered composite keys.
A grouped ES index such as
(embedding, category, price)can answer a predicate oncategoryorpriceindependently. Currently the single-field scalar paths exclude this definition altogether, so a normal filtered scan can silently fall back to reading data without opening the ES index. Metadata-only coverage also cannot establish that every shard accepted the predicate.This change:
GlobalIndexerFactory.supportsGroupedIndexScalarQueries()as an opt-in capability, defaulting tofalse. Only the ES factory opts in; there is no ES type-name special case in the core planner.es_search()SQL API.The additional ESLib module dependencies are test-scoped only.
Tests
Added 23 test invocations:
Local validation on upstream base
b21f36cc56ee6bd11f43b7a2d7f43695445cd446uses JDK 11 with Checkstyle, Spotless and Enforcer enabled. The targeted reactor command is:mvn -B -ntp -Ppaimon-eslib -pl paimon-eslib -am \ -DwildcardSuites=none -DfailIfNoTests=false \ '-Dtest=ESIndex*Test,GlobalIndexQueryTest,GlobalIndexEvaluatorTest,CompositeBTreeTableTest,IndexQuerySplitTest,DataEvolutionBatchScanTest,FullTextSearchBuilderTest,VectorSearchBuilderTest,VectorSearchRowFilterExactnessTest' \ packagepackageis used for a fresh worktree so the codegen-loader embedded resources are prepared before the dependent tests run.Result on upstream
b21f36cc56ee6bd11f43b7a2d7f43695445cd446: 380 tests passed, 0 failures, 0 errors, 0 skipped (38 common + 252 core + 90 ESLib). This is a targeted regression run, not the entire repository test suite.CI status
Python / Native CIfailed. The same job on the exact upstream parent, without this change, reported 67 failed, 19154 passed, 131 skipped. The Python source tree and Python CI workflow are byte-identical between this commit and its parent.paimon-python/conftest.pydoes not accept the newbatchkeyword passed by truncate operations. Other baseline failures include native reader/test-fixture incompatibilities and update-order assertions.paimon-rustmain branch and resolved different Rust commits. The fork Native job ended before a final pytest summary; its early termination is not yet explained. This PR does not change Python/Rust code or suppress failing tests.