GGraziadei commented on code in PR #2133:
URL: https://github.com/apache/stormcrawler/pull/2133#discussion_r3945475328
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ if (compiled.prefixes.length > 0) {
+ for (Map.Entry<String, String[]> entry : source.entrySet()) {
+ final String key = entry.getKey();
+ if (entry.getValue().length == 0 || target.containsKey(key)) {
+ continue;
+ }
+ for (String prefix : compiled.prefixes) {
+ if (key.startsWith(prefix)) {
+ target.put(key, entry.getValue());
+ break;
+ }
}
- } else {
- metadata.copy(filteredMetadata, key);
}
}
- return filteredMetadata;
+ return new Metadata(target);
+ }
+
+ /**
+ * Pre-computed, normalised form of a set of keys to transfer: exact keys
and wildcard prefixes.
+ * Cached per set and rebuilt if the set has been modified since (e.g. by
a subclass).
+ */
+ private static final class CompiledFilter {
+ private final Set<String> sourceSet;
+ private final int sourceSize;
+ private final Set<String> exactKeys;
+ private final String[] prefixes;
+
+ private CompiledFilter(Set<String> filter) {
+ this.sourceSet = filter;
+ this.sourceSize = filter.size();
+ final Set<String> exact = new HashSet<>();
+ final List<String> prefixList = new ArrayList<>();
+ for (String key : filter) {
+ final String normalised = key.toLowerCase(Locale.ROOT);
+ if (normalised.endsWith("*")) {
+ prefixList.add(normalised.substring(0, normalised.length()
- 1));
+ } else {
+ exact.add(normalised);
+ }
+ }
+ this.exactKeys = exact;
+ this.prefixes = prefixList.toArray(new String[0]);
+ }
+
+ private boolean isFor(Set<String> filter) {
Review Comment:
Fixed in a9578fd. `CompiledFilter` now keeps a `HashSet` snapshot of the
keys it was built from and `isFor` is `snapshot.equals(filter)`, so a same-size
content change (`remove("depth"); add("mycustom")`) rebuilds the compiled form.
Javadoc adjusted to describe what the code does. Test
`testSameSizeMutationOfTransferSetIsHonoured` reproduces the exact scenario and
failed on the previous revision.
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ if (compiled.prefixes.length > 0) {
+ for (Map.Entry<String, String[]> entry : source.entrySet()) {
+ final String key = entry.getKey();
+ if (entry.getValue().length == 0 || target.containsKey(key)) {
+ continue;
+ }
+ for (String prefix : compiled.prefixes) {
+ if (key.startsWith(prefix)) {
+ target.put(key, entry.getValue());
+ break;
+ }
}
- } else {
- metadata.copy(filteredMetadata, key);
}
}
- return filteredMetadata;
+ return new Metadata(target);
+ }
+
+ /**
+ * Pre-computed, normalised form of a set of keys to transfer: exact keys
and wildcard prefixes.
+ * Cached per set and rebuilt if the set has been modified since (e.g. by
a subclass).
+ */
+ private static final class CompiledFilter {
+ private final Set<String> sourceSet;
+ private final int sourceSize;
+ private final Set<String> exactKeys;
+ private final String[] prefixes;
+
+ private CompiledFilter(Set<String> filter) {
+ this.sourceSet = filter;
+ this.sourceSize = filter.size();
+ final Set<String> exact = new HashSet<>();
+ final List<String> prefixList = new ArrayList<>();
+ for (String key : filter) {
+ final String normalised = key.toLowerCase(Locale.ROOT);
+ if (normalised.endsWith("*")) {
+ prefixList.add(normalised.substring(0, normalised.length()
- 1));
+ } else {
+ exact.add(normalised);
+ }
+ }
+ this.exactKeys = exact;
+ this.prefixes = prefixList.toArray(new String[0]);
+ }
+
+ private boolean isFor(Set<String> filter) {
+ return sourceSet == filter && sourceSize == filter.size();
+ }
+ }
+
+ private volatile CompiledFilter compiledTransfer;
+ private volatile CompiledFilter compiledPersistOnly;
+
+ private CompiledFilter compile(Set<String> filter) {
Review Comment:
I went with the second option (sound check on a copy of the contents) rather
than compiling at the end of `configure()`, for one reason: `configure()` is
protected and a subclass that does `super.configure(conf);
mdToTransfer.add("added.*");` is the natural extension point given the two
`protected` sets. Compiling inside the base `configure()` would leave that
subclass with a stale filter. `testSubclassCanExtendTransferSetInConfigure`
pins that case.
The cache is now built lazily on first use and `isFor` is a `Set.equals`
against the snapshot: a size check plus one hash lookup per key with cached
`String` hashes, no allocation. The per-outlink win (the stream + intermediate
`Set`) is unchanged.
Dropped: the two `volatile`s (all `CompiledFilter` fields are final, and the
cache is idempotent), the identity dispatch and the `null` branch.
`filter(Metadata, Set)` is now `filter(Metadata, CompiledFilter)` fed by
`transferFilter()` / `persistOnlyFilter()`.
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ if (compiled.prefixes.length > 0) {
+ for (Map.Entry<String, String[]> entry : source.entrySet()) {
+ final String key = entry.getKey();
+ if (entry.getValue().length == 0 || target.containsKey(key)) {
+ continue;
+ }
+ for (String prefix : compiled.prefixes) {
+ if (key.startsWith(prefix)) {
+ target.put(key, entry.getValue());
+ break;
+ }
}
- } else {
- metadata.copy(filteredMetadata, key);
}
}
- return filteredMetadata;
+ return new Metadata(target);
+ }
+
+ /**
+ * Pre-computed, normalised form of a set of keys to transfer: exact keys
and wildcard prefixes.
+ * Cached per set and rebuilt if the set has been modified since (e.g. by
a subclass).
+ */
+ private static final class CompiledFilter {
+ private final Set<String> sourceSet;
+ private final int sourceSize;
+ private final Set<String> exactKeys;
+ private final String[] prefixes;
+
+ private CompiledFilter(Set<String> filter) {
+ this.sourceSet = filter;
+ this.sourceSize = filter.size();
+ final Set<String> exact = new HashSet<>();
+ final List<String> prefixList = new ArrayList<>();
+ for (String key : filter) {
+ final String normalised = key.toLowerCase(Locale.ROOT);
+ if (normalised.endsWith("*")) {
+ prefixList.add(normalised.substring(0, normalised.length()
- 1));
+ } else {
+ exact.add(normalised);
+ }
+ }
+ this.exactKeys = exact;
+ this.prefixes = prefixList.toArray(new String[0]);
+ }
+
+ private boolean isFor(Set<String> filter) {
+ return sourceSet == filter && sourceSize == filter.size();
+ }
+ }
+
+ private volatile CompiledFilter compiledTransfer;
+ private volatile CompiledFilter compiledPersistOnly;
+
+ private CompiledFilter compile(Set<String> filter) {
+ CompiledFilter compiled =
+ filter == mdToTransfer
+ ? compiledTransfer
+ : filter == mdToPersistOnly ? compiledPersistOnly :
null;
Review Comment:
Gone. The private filter now takes a `CompiledFilter` directly, obtained
from `transferFilter()` / `persistOnlyFilter()`, so there is no dispatch on set
identity anymore.
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ if (compiled.prefixes.length > 0) {
+ for (Map.Entry<String, String[]> entry : source.entrySet()) {
+ final String key = entry.getKey();
+ if (entry.getValue().length == 0 || target.containsKey(key)) {
Review Comment:
Fixed. The wildcard loop reads `entry.getValue()` once and skips `null` as
well as empty arrays, matching the old `getValues()` behaviour.
`testNullValueArrayIsSkipped` builds a `Metadata` over a map with `null` arrays
for both an exact key and a wildcard match; it threw an NPE on the previous
revision.
##########
core/src/test/java/org/apache/stormcrawler/util/MetadataTransferTest.java:
##########
@@ -152,4 +152,26 @@ void testFilterWithAsterisk() {
}
static class MyCustomTransferClass extends MetadataTransfer {}
+
+ @Test
+ void testWildcardPrefixIsCaseInsensitiveAndSelective() throws
MalformedURLException {
Review Comment:
Added three tests: `testSameSizeMutationOfTransferSetIsHonoured` (remove/add
keeping the size, second call must see the new keys; failed before),
`testSubclassCanExtendTransferSetInConfigure` (subclass adds a wildcard after
`super.configure()`), and `testNullValueArrayIsSkipped`. Full `core` verify:
447 tests, 0 failures.
--
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]
