TIKA-4766: typed Document parse contract for tika-grpc - #2921
Conversation
c1de042 to
1d2d5bd
Compare
1d2d5bd to
da51be9
Compare
|
@nddipiazza @tballison what do you think of this design instead? far less fields - ability to roll your own protobuf model in the future. Best of both worlds. Document structure is very markdown-friendly. I'll make the output of this be able to be the input of the grpc OpenNLP grpc server. |
nddipiazza
left a comment
nddipiazza
left a comment
There was a problem hiding this comment.
Review: e2e test coverage for the new typed Document contract
Reviewed the design overall — the shift from mirroring Tika's metadata taxonomy in protobuf to a small, stable document.proto (208 lines) plus per-parser mapper code is a solid answer to the #2916 feedback. A few things worth addressing before merge:
Testing gap: e2e coverage does not exercise the new contract
The only change in tika-e2e-tests/tika-grpc is a 2-line fix in HandlerTypeTest.java:
- String htmlContent = htmlReply.getFieldsMap().get("X-TIKA:content");
+ String htmlContent = htmlReply.getDocument().getMarkdown();(same for the TEXT handler case). This is a compile-fix to keep the pre-existing assertion working against the new API — it only proves document.markdown is non-empty over a real gRPC round-trip for two handler types.
It does not exercise, end-to-end, through the live server:
DocumentMetadatatyped fields (title/authors/dates/counts) — no e2e assertion anywhereextratagged-tail typing (int/bool/date/string) — no e2e assertionblocksstructured content tree (headings/tables/lists/code) — untouched by any e2e testembeddedrecursion for container formats (e.g. an Office doc with an embedded image) — no e2e coverage at allformat_categoryrouting hint — untouched
The other e2e tests (FileSystemFetcherTest, IgniteConfigStoreTest, ExternalTestBase) only ever inspect getFetchKey()/getStatus() on FetchAndParseReply — none of them look at getDocument(), so the bulk-corpus/streaming/ignite e2e paths give zero signal on the new typed contract.
The real proof of correctness for the mapping logic lives entirely in tika-grpc-mapper's unit tests (DocumentBuilderTest, MarkdownBlockTreeBuilderTest, one test class per transformer), which feed fixture-derived Metadata/markdown into DocumentBuilder in-process. That's good for the mapping logic itself, but it bypasses the actual gRPC wire serialization, the live server (TikaGrpcServerImpl), the pipes client, and fetcher plumbing entirely.
Ask: add (or extend HandlerTypeTest) at least one e2e case per format that fetches a real file through the live gRPC server and asserts on document.metadata (a couple of typed fields), document.extra (at least one tagged key), and one document.embedded case (e.g. an Office/PDF file with an embedded image) — not just that markdown is non-empty. Right now this new, larger surface area has no live-server coverage at all.
Other findings from code inspection
- Likely-dead error path:
DocumentBuilder.build()'sprimary == nullbranch (returnsFAILEDwith "No metadata returned from parse") looks unreachable in production — the only caller,TikaGrpcServerImpl, always passes a non-nullMetadata(tikaMetadata = new Metadata()as fallback whenemitData()/metadata list is empty). Worth double-checking this is intentional, since a real fetch failure with no emit data currently produces an "empty-but-success-shaped"Documentrather than hitting this explicit error branch. - Duplication: 7 of 8 format transformers (
Generic,Pdf,Html,Image,Office,Rtf,Epub) repeat an identical block mapping TITLE/DESCRIPTION/CREATOR/SUBJECT/LANGUAGE/CREATED/MODIFIED. Consider factoring this into a shared helper inTransformSupportcalled by each transformer, keeping only the format-specific additions per-transformer. - Minor:
parseTimeMsinTikaGrpcServerImplmeasures fetch+parse combined (timer starts beforepipesClient.process()), though the field's doc/intent reads as parse-only time.
Nice work on the overall shape of the contract — the main blocker from my read is the e2e coverage gap above.
nddipiazza
left a comment
nddipiazza
left a comment
There was a problem hiding this comment.
Overall this is a solid design and a much better-scoped answer to the #2916 feedback: a small stable 208-line proto, format logic pushed into per-parser Java transformers, and a lossless typed+tagged tail. Requesting changes mainly on test coverage for the new contract, plus a couple of correctness/maintainability issues found while reading the mapper code (see inline comments).
|
@nddipiazza great feedback..
My bad; the e2e suite really was only asserting "markdown is non-empty". Added three live-server cases to
Two things fell out of writing these:
Note the CI e2e job doesn't currently execute these (the |
|
Thanks for the review @nddipiazza, all four points addressed:
Also merged latest main (the lombok removal and split-packages changes). Full build plus mapper, grpc, and e2e suites pass locally. |
234247b to
8b79b26
Compare
One Document shape for parsed output instead of a message per source format. Content is a block tree anchored to the markdown block model; the tree is canonical and a flat markdown rendering is returned only when the request asks, so a reply never carries the content twice. Format specifics come from per-parser transformer code, not new wire messages: adding a parser never touches the proto. The typed metadata follows the Dublin Core element set (title, authors, description, keywords, languages, publishers, identifiers, dates, rights), with a tagged tail for the rest: typed where Tika declares a type, string otherwise, never guessed. Status lives in one place, the typed ParseStatus for branching plus the raw pipes result name for diagnostics. SourceOrigin records the SHA-256 of the parsed bytes and ParseStatus the producing Tika version. The shape stays small. What it adds is metadata consumers end up parsing or inferring on their own anyway: provenance, typed fields, and structure that does not need a second parse.
8b79b26 to
a0ff002
Compare
DocumentTextFlattener walks Document.blocks depth-first and emits plain text, recording an exact (block path, char range) anchor for every text-bearing block as it goes, so a consumer that annotates the flat text can project spans back onto blocks without re-parsing anything. Tables linearize row-wise with an anchor per cell; code blocks and raw html are skipped for analysis and reported as skipped paths; blocks are separated by a blank line so sentence boundaries cannot cross blocks. FlattenedText carries the document id, the source digest, and a flattening version, since offsets are only meaningful against the exact flattening that produced them.
Parses bytes the caller already has: no fetcher registration, no client-visible server state, and it works with component management locked down. The parse runs in the same isolated forked worker as FetchAndParse. Runtime config-store writes do not reach the forked worker, so the server bakes an internal file-system fetcher, rooted at a server temp directory, into an augmented copy of its config at construction. The payload is written there under a unique name, parsed, and deleted, and the reply carries the caller's request_id as the fetch key and Document id.
|
I figured I'd explain why and what I'm trying to do. I view tika is a primary input plane and think the gRPC server is where I'd want it to integrate. So a Tika The OpenNLP gRPC server is deliberately following OpenNLP 3.0 as it develops. As features are merged to OpenNLP If Tika adopts this I want to avoid defining and implementing a second document input format before we settle this boundary. If Tika later chooses a different shape, OpenNLP would have an unused input contract and both projects would carry unnecessary compatibility surface. So I'm curious: Is this If not, which parts need to change before it can serve that role? I'm in no rush to get the answer - but I've never had an opportunity explicit about the vision and goal. By agreeing to a model, it can allow both projects to launch with an amazing open source story to tell. The current OpenNLP gRPC work is here: https://github.com/apache/opennlp-sandbox/tree/OPENNLP-1833-grpc-expansion Once we agree on a shape, I can make the OpenNLP server conform to it as the rest of the 3.0 features land. This is not going to happen right away - we're still going to tease more releases so I'd expect both servers to go through many iterations. What I'm trying to do on our side is to have a generic enough output shape so we can survive a lot of new features for point releases. With Tika - I want to help integrate and transform a lot of parsing surfaces and connectors (as you saw with the markdown parser). But it all starts with a document shape - then there's no rush to add on more features. Thoughts? |
|
@nddipiazza up for another look? If you land this shape, I can demo how this would work w/opennlp 3's grpc server + make an enhancement to tika to integrate it directly. Let me know what you think - Another suggestion is to make this a separate build under the tika umbrella since you'll need to keep up with CVEs and stuff for creating a docker image. I'd help regardless. Up to you.. |
|
Follow-up after digging further into the Two scope concerns in the current diff1.
The implementation in
Given the whole point of this PR (and #2916) is keeping tika-grpc small and reviewable, I'd pull 2.
Aside from those two, the actual mapper transformers ( Proposal: stage this as a sequence of small PRsGiven @tballison's core objection was scale/maintenance risk of a big bang change, I think this whole effort would merge faster broken into independently reviewable, independently testable PRs — mostly a split of what already exists here, not a rewrite:
Would drop or defer indefinitely:
Each stage above is independently testable (unit + e2e) and independently mergeable, which should make review a lot less daunting than reviewing ~3.9k lines at once. |
|
This sounds great and goes exactly in the direction I'm hoping. Multiple tickets = real collaboration and proper pacing I'll close this and post the first suggested change but keep the overall thread alive here unless you can do something in jira after a couple tickets land. I like the feedback's instinct because shaping the API is by far the most important part. Honestly I understand this is a big feature and I appreciate the time you're taking to look into it. seriously. |
First slice of the typed Document contract, per review on apache#2921: the Document envelope, Dublin Core DocumentMetadata, the lossless tagged metadata tail, and ParseStatus. FetchAndParseReply.document is additive: the legacy fields/status stay untouched, so no client breaks. Tail values mirror Tika's String[]-backed metadata model: every entry is a typed array, tagged by the declared Property element type, so declared integer sequences (pdf:charsPerPage, tiff:BitsPerSample) arrive as int64s per element and dates as Timestamps. Untyped keys stay strings, never guessed; if any element refuses its declared type the whole entry falls back to strings. Deferred to follow-up PRs: per-format transformers, the structured content tree, embedded-document recursion. Dropped per review: FormatCategory, the ParseBytes RPC.
|
Closing in favor of #2961 |
2 participants
Summary
Follow-up to #2916, reshaped per the review there. Instead of mirroring Tika's open metadata taxonomy in protobuf (~5k lines of proto, per-format messages), this PR types the thing that is actually stable: the parsed document. One small contract —
document.protois 208 lines — and format specifics live in per-parser mapping code, never in the wire.FetchAndParseReply.fields(map<string,string>, field 2, now reserved) is replaced byFetchAndParseReply.document.How this answers the #2916 review
extratail), not schema — add/rename/retype a TikaPropertyand no client regenerates anythingDocument.extracarries every Tika key, multivalue-preserving; a test asserts nothing is droppedDocumentMetadatatypes only the bounded cross-format fields (title, authors, dates asTimestamp, counts, dimensions, rights)The shape
markdown(the same renderToMarkdownContentHandleralready produces since TIKA-4730) plusblocks— that markdown parsed once, format-agnostically, into a structured tree of headings/paragraphs/lists/tables/code blocks/inline runs (CommonMark + GFM, a spec that does not churn). This is what a downstream NLP/RAG/embeddings consumer actually wants: typed tables and sections, not a string to re-parse.DocumentMetadata, grouped by concern, not by source format. Dates areTimestamps, counts are ints — not strings that 12 language clients each re-parse.extra— every remaining key, typed only where Tika's ownPropertydeclares a type (integer/real/boolean/date), string otherwise, never guessed.embeddedrecurses: a PDF with an embedded image is a parentDocumentwith a fully typed child — no forcing two formats into one bucket (this was the oneof problem from TIKA-4766: Typed parse response grpc #2916).DocumentTransformer(seetika-grpc-mapper/docs/EXTENSIONS.md);PdfDocumentTransformeris 65 lines and the wire contract does not move.Deliberately not in this PR (follow-ups, each its own PR)
Documentas agoogle.protobuf.Any— so wildly different result shapes (e.g. a document-layout model's tree) never require Tika to model them. Built and tested on a branch; kept out to keep this reviewable..mdinput files (separate JIRA).optionalfields, compatible both directions.Open decisions where reviewer preference wins
repeated MetadataFieldwith a typed value oneof (as implemented) vs themap<string, StringList>suggested in TIKA-4766: Typed parse response grpc #2916. The typed-where-declared tail preserves types without guessing; the map is maximally churn-proof. Swapping is a one-message change — happy to go either way.markdown+blocksboth: today both ship (string render + structured tree). If payload size matters, a per-request flag choosing one is easy.fieldsis hard-removed (4.0, nothing consumes it yet); can switch to deprecate-then-remove if preferred.Client migration
fields["X-TIKA:content"]document.markdown(or walkdocument.blocks)fields["Content-Type"]document.content_typedocument.metadata.title/.authors/.created(Timestamp)document.extra(typed by declaredPropertytype, string otherwise)Test plan
./mvnw -pl tika-grpc-api,tika-grpc-mapper,tika-grpc test— green (transformer tests against real parse fixtures per format, block-tree tests,DocumentBuilderenvelope/status/embedded tests, server tests readingFetchAndParseReply.document)tika-grpc-apijar bundlesMETA-INF/org.apache.tika.grpc.v1.descriptors(verified: containsdocument.proto)tika-grpc-e2e-testcompiles against the new APIDownstream context: this contract is what the OpenNLP gRPC work (OPENNLP-1833) will consume as input — Tika parse → typed document → NLP/embeddings without re-parsing strings.