jnioche commented on issue #158: URL: https://github.com/apache/stormcrawler/issues/158#issuecomment-6337600001
I had a look at which ParseFilters actually use the DOM (i.e. return `true` for `needsDOM()`): | DOM ParseFilter (`o.a.s.parse.filter`) | JSoupFilter equivalent (`o.a.s.jsoup`) | |---|---| | `XPathFilter` | `XPathFilter` | | `LinkParseFilter` (inherits from `XPathFilter`) | `LinkParseFilter` (same logic) | | `LDJsonParseFilter` | `LDJsonParseFilter` (uses a CSS selector, no DOM) | | `DebugParseFilter` | none, but it is only there to debug the DOM | `JsRenderingDetector` (playwright) already returns `false`. All the other ParseFilters (`CollectionTagger`, `DomainParseFilter`, `MD5SignatureParseFilter`, `MimeTypeNormalization`, `CommaSeparatedToMultivaluedMetadata`, `LanguageID`...) only have `DocumentFragment` in their signature and never use it. The archetypes already ship the JSoup versions. Since the next release is a major one, we should **remove** the DOM-based `XPathFilter`, `LinkParseFilter` and `LDJsonParseFilter` rather than deprecate them. A few things to take into account when doing so: 1. **`jsoup.XPathFilter` is not DOM-free.** jsoup's `Element.selectXpath()` converts the document to a W3C DOM on *every call* (see `NodeUtils.selectXpath` → `W3CDom.fromJsoup`). `jsoup.XPathFilter` evaluates at least one expression per key and `jsoup.LinkParseFilter` evaluates every pattern, so the document gets converted once per expression. With several expressions this is likely more expensive than the DOM version, which builds it once. Before removing the DOM filters, the JSoup ones should build the W3C document once per page and reuse it across expressions (or favour CSS selectors where possible). 2. **The expression syntax differs.** The DOM `XPathFilter` supports `string(...)` and `serialize(...)`; the JSoup one supports `/tidyText()`, `/allText()`, `/html()` and `/@attr`. Users will have to rewrite their configs, so this needs to go in the migration notes (compare `test.parsefilters.json` with `test.jsoupfilters.json`). 3. **Tika.** JSoupFilters only run in `JSoupParserBolt`, so users of the Tika `ParserBolt` would no longer have an XPath-based extraction. We should either accept that and document it, or leave the DOM building in Tika behind a config switch. 4. **`needsDOM()`**: once these filters are gone, only `DebugParseFilter` (and custom user filters) would need the DOM. We could then drop `needsDOM()` and replace it with a config option telling the parser bolts whether to build a DOM, as suggested above. Filters using it must handle a `null` doc anyway, since `SiteMapParserBolt` and `FeedParserBolt` always pass `null`. Related clean-ups: - `RobotsTags.extractMetaTags(DocumentFragment)` is never called (`JSoupParserBolt` uses the `String` variant) and can be removed. - `JSONResourceWrapper` (opensearch and opensearch-java) does not delegate `needsDOM()`, so wrapping a filter that needs the DOM currently gets a `null` doc. This goes away if `needsDOM()` is removed. @AsifQureshi04 does that sound good to you? Happy for you to start with the removal of the three filters and the change to `jsoup.XPathFilter` so that it builds the W3C DOM only once. -- 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]
