feat(artifacts): search a document's structure pages - #2319
Conversation
2ef70a3 to
e5160f1
Compare
caro3801
left a comment
There was a problem hiding this comment.
Great feature and solid implementation easy to follow through
There are the two blocking correctness issues before merge:
- the page-count cap currently affects all artifact routes,
- and narrowed read-error handling may turn some 404 cases into 500s.
I also put a question about storing the OCR strategy used at index time to avoid future content/rendering mismatches. The rest is mostly cleanup, clarification, and small structure-search efficiency improvements.
| /** Beyond this a total is a corrupt manifest, not a long document. It matters because {@link | ||
| * StructureSearch} turns a total written by another producer into a loop bound, and unbounded, | ||
| * {@code Integer.MAX_VALUE} never terminates at all: the counter wraps to MIN_VALUE, still in range. */ | ||
| public static final int MAX_SERVABLE_PAGES = 100_000; |
There was a problem hiding this comment.
important: MAX_SERVABLE_PAGES is enforced here in servableTotal(), which every artifact route
goes through (manifest, single-page fetch, and structure search) - but the doc comment on the
constant says its only purpose is bounding StructureSearch's scan loop from running away on a
corrupt total.
Could we move the cap into StructureSearch.scan() instead (clamp its own loop
bound to min(total, MAX_SERVABLE_PAGES)) and let servableTotal() accept any positive total? As
written, a legitimately huge document (a merged archive, a bulk-exported log) with more than
100,000 pages now 404s on every artifact route, not just search.
There was a problem hiding this comment.
Moved in 414807e: servableTotal() now takes any positive total, and StructureSearch clamps its own loop to min(total, MAX_SCANNED_PAGES). pages still reports the manifest total, so past the cap scanned < pages marks the count as a floor.
| public Hits search(String query) throws IOException { | ||
| ManifestEntry entry = reader.servableEntry(docArtifactDir, TYPE); | ||
| Integer total = entry == null ? null : reader.servableTotal(docArtifactDir, TYPE, entry); | ||
| // Without the formats probe, a document whose page-N.md files are gone would answer "no |
There was a problem hiding this comment.
suggestion(perf): search()'s formats() probe stats page 1 to confirm the format exists, and then
scan()'s first loop iteration reads that same page 1 file again a few lines later.
This is the exact stat-then-read round trip the sibling ArtifactReader.page() change in this PR was written to eliminate, just reintroduced once per search request.
Could we have the probe read page 1's bytes directly (instead of just checking readability) and hand them to scan() so the first iteration reuses them instead of re-reading?
There was a problem hiding this comment.
Fixed in aba5c50, one step further: the probe is gone. search() counts page 1 through ArtifactReader.page() and seeds the scan, which starts at page 2. One consequence: under byte-range pagination a malformed range 1 now 404s the search, a scheme no structure artifact uses.
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
| private PDFParserConfig.OCR_STRATEGY pdfOcrStrategy() { | ||
| try { | ||
| return PDFParserConfig.OCR_STRATEGY.valueOf( | ||
| propertiesProvider.get(OCR_STRATEGY_OPT).orElse("NO_OCR").toUpperCase(Locale.ROOT)); | ||
| } catch (IllegalArgumentException unknown) { | ||
| return PDFParserConfig.OCR_STRATEGY.NO_OCR; | ||
| } | ||
| } |
There was a problem hiding this comment.
hint: do you want to reuse the same enum parsing/mapping as in datashare cli and map it into Tika's so the two don't drift independently if a member is ever renamed?
private PDFParserConfig.OCR_STRATEGY pdfOcrStrategy() {
try {
OcrStrategy strategy = OcrStrategy.valueOf(
propertiesProvider.get(OCR_STRATEGY_OPT).orElse("NO_OCR").toUpperCase(Locale.ROOT));
return PDFParserConfig.OCR_STRATEGY.valueOf(strategy.name());
} catch (IllegalArgumentException unknown) {
return PDFParserConfig.OCR_STRATEGY.NO_OCR;
}
}| // Half-open [start, end). A range outside the file means manifest and payload disagree, which | ||
| // is a 404 for that page rather than a truncated body. | ||
| private byte[] slice(Path content, List<long[]> ranges, int page, ArtifactType type, int total) throws IOException { | ||
| if (!Files.isReadable(content)) { |
There was a problem hiding this comment.
suggestion: slice() does a Files.isReadable() presence check (stat) before opening the file, the exact stat-then-read pattern this same PR removed from the sibling filesystem-page branch a few lines up, with a comment calling it out as a doubled round trip on a shared/network artifactDir.
Could we drop the pre-check here too and read-then-catch instead, the way page()'s filesystem branch now does?
remark(TOCTOU): this pattern can be sometimes racy, especially on network/shared filesystems. It’s often better to just try the read/open directly and handle IOException, because the file could change between the check and the read anyway.
Adds an in-document search over a document's persisted structure pages, counting a query's occurrences per page the way
/documents/searchContentcounts them over the indexed content.GET /:project/artifacts/structure/search/:id, answering{count, pages, scanned, hits}searchOccurrences.painless.javafolding rules to Java, with a test pinning the script so the two counters cannot drift apartocrandocrStrategy, so every existing structure artifact is stale and the next ARTIFACT run re-extractsHow it works, and what it does not do
The scan reads one
page-N.mdat a time and folds it with the rules ported fromsearchOccurrences.painless.java, so the endpoint and/documents/searchContentfold text identically. Occurrences are counted per page withindexOfstepping by the raw query length, and summed. Work is linear in pages, capped by a 10s budget:scannedbelowpagesmeans the count is a floor, not a total.Known limits, all deliberate:
contentfield.targetLanguage.--ocrStrategy, which is the same condition under which INDEX extracted nothing from it.