dsmiley commented on code in PR #1466:
URL: https://github.com/apache/solr/pull/1466#discussion_r1143669893
##########
solr/core/src/java/org/apache/solr/core/backup/BackupManager.java:
##########
@@ -254,7 +254,10 @@ public void uploadConfigDir(
String sourceConfigName, String targetConfigName, ConfigSetService
configSetService)
throws IOException {
URI source = repository.resolveDirectory(getZkStateDir(),
CONFIG_STATE_DIR, sourceConfigName);
- Preconditions.checkState(repository.exists(source), "Path %s does not
exist", source);
+ if (!repository.exists(source)) {
+ throw new IllegalArgumentException(
+ String.format(Locale.ROOT, "Path %s does not exist", source));
Review Comment:
Fine, but just want to say I think String.format is over-used, such as here.
Good 'ol String concatenation is simple.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -2937,7 +2939,12 @@ public static void preDecorateResponse(SolrQueryRequest
req, SolrQueryResponse r
@Override
public Iterator<String> getParameterNamesIterator() {
- return Iterators.filter(params.getParameterNamesIterator(),
lpSet::contains);
+ return StreamSupport.stream(
+ Spliterators.spliteratorUnknownSize(
+ params.getParameterNamesIterator(),
Spliterator.ORDERED),
+ false)
Review Comment:
It [was a deliberate decision](https://stackoverflow.com/a/23177907/92186)
of Java architects to make it hard to go from an Iterator to a Stream. In
light of this, maybe we should avoid returning Iterators from our APIs, I
suppose. Just thinking out loud; your change here is fine.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1265,15 +1261,13 @@ protected Elevation mergeWith(Elevation elevation) {
return this;
}
Set<BytesRef> elevatedIds =
- ImmutableSet.<BytesRef>builder()
- .addAll(this.elevatedIds)
- .addAll(elevation.elevatedIds)
- .build();
+ Stream.concat(this.elevatedIds.stream(),
elevation.elevatedIds.stream())
+ .collect(Collectors.toCollection(LinkedHashSet::new));
boolean overlappingElevatedIds =
elevatedIds.size() != (this.elevatedIds.size() +
elevation.elevatedIds.size());
BooleanQuery.Builder includeQueryBuilder = new BooleanQuery.Builder();
Set<BooleanClause> clauseSet =
- (overlappingElevatedIds ?
Sets.newHashSetWithExpectedSize(elevatedIds.size()) : null);
+ (overlappingElevatedIds ? new HashSet<>(elevatedIds.size()) : null);
Review Comment:
This is a Java gotcha! [Java 19 has an
substitute](https://bugs.openjdk.org/browse/JDK-8186958) but obviously we're
not there yet. Maybe we continue to use Guava for this or have our own
utility? Or just use 2x the size?
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1299,14 +1291,16 @@ protected Elevation mergeWith(Elevation elevation) {
excludedIds.size() != (this.excludedIds.size() +
elevation.excludedIds.size());
if (overlappingExcludedIds) {
excludeQueries =
- ImmutableSet.<TermQuery>builder()
- .add(this.excludeQueries)
- .add(elevation.excludeQueries)
- .build()
- .toArray(new TermQuery[0]);
+ Stream.concat(
+ Arrays.stream(this.excludeQueries),
Arrays.stream(elevation.excludeQueries))
+ .collect(Collectors.toUnmodifiableSet())
Review Comment:
Use Stream.distinct().toArray(...)
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/LegacyAbstractAnalyticsTest.java:
##########
@@ -227,18 +229,25 @@ public <T extends Comparable<T>> Long
calculateMissing(ArrayList<T> list, String
}
public static SolrQueryRequest request(String... args) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class));
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new));
Review Comment:
don't collect then toArray; just go straight to toArray
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/LegacyAbstractAnalyticsTest.java:
##########
@@ -227,18 +229,25 @@ public <T extends Comparable<T>> Long
calculateMissing(ArrayList<T> list, String
}
public static SolrQueryRequest request(String... args) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class));
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new));
}
public static SolrQueryRequest request(String[] args, String... additional) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class), additional);
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new),
Review Comment:
don't collect then toArray; just go straight to toArray
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1234,12 +1230,12 @@ public Elevation(Set<BytesRef> elevatedIds,
Set<BytesRef> excludedIds, String qu
this.excludedIds = Collections.emptySet();
excludeQueries = null;
} else {
- this.excludedIds = ImmutableSet.copyOf(excludedIds);
- List<TermQuery> excludeQueriesBuilder = new
ArrayList<>(excludedIds.size());
- for (BytesRef excludedId : excludedIds) {
- excludeQueriesBuilder.add(new TermQuery(new Term(queryFieldName,
excludedId)));
- }
- excludeQueries = excludeQueriesBuilder.toArray(new TermQuery[0]);
+ this.excludedIds = Collections.unmodifiableSet(new
LinkedHashSet<>(excludedIds));
+ this.excludeQueries =
+ this.excludedIds.stream()
+ .map(excludedId -> new TermQuery(new Term(queryFieldName,
excludedId)))
+ .collect(Collectors.toUnmodifiableList())
Review Comment:
Stream.toArray exists; remove the intermediate List
##########
solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java:
##########
@@ -885,7 +884,7 @@ private static SolrInputDocument
toSolrInputDocument(SolrDocument doc, IndexSche
if (!fieldArrayListCreated && doc.getFieldValue(fname) instanceof
Collection) {
// previous value was array so we must return as an array even if
was a single value
// array
- out.setField(fname, Lists.newArrayList(val));
+ out.setField(fname, new ArrayList<>(List.of(val)));
Review Comment:
Why the extra wrapping using List.of ?
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1299,14 +1291,16 @@ protected Elevation mergeWith(Elevation elevation) {
excludedIds.size() != (this.excludedIds.size() +
elevation.excludedIds.size());
if (overlappingExcludedIds) {
excludeQueries =
- ImmutableSet.<TermQuery>builder()
- .add(this.excludeQueries)
- .add(elevation.excludeQueries)
- .build()
- .toArray(new TermQuery[0]);
+ Stream.concat(
+ Arrays.stream(this.excludeQueries),
Arrays.stream(elevation.excludeQueries))
+ .collect(Collectors.toUnmodifiableSet())
+ .toArray(TermQuery[]::new);
} else {
excludeQueries =
- ObjectArrays.concat(this.excludeQueries,
elevation.excludeQueries, TermQuery.class);
+ Stream.concat(
+ Arrays.stream(this.excludeQueries),
Arrays.stream(elevation.excludeQueries))
+ .collect(Collectors.toUnmodifiableSet())
Review Comment:
Use Stream.distinct().toArray(...)
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2192,6 +2191,9 @@ public SolrCore getCore(String name) {
* @see SolrCore#close()
*/
public SolrCore getCore(String name, UUID id) {
+ if (name == null) {
+ return null;
+ }
Review Comment:
Unrelated?
##########
solr/core/src/test/org/apache/solr/cloud/NestedShardedAtomicUpdateTest.java:
##########
@@ -64,7 +72,10 @@ public static void beforeClass() throws Exception {
@AfterClass
public static void afterClass() throws Exception {
- IOUtils.close(clients);
+ if (clients != null) {
+ IOUtils.close(clients);
+ clients = null;
+ }
Review Comment:
Did this change prove to be necessary or impactful?
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1558,7 +1556,7 @@ Solution is q(k) = 1/2 (k^2+k+2)
public Builder<E, M> addSubset(Collection<E> subset, M matchValue) {
if (!subset.isEmpty()) {
TrieSubsetMatcher.Node<E, M> node = root;
- for (E e : ImmutableSortedSet.copyOf(subset)) {
+ for (E e : Collections.unmodifiableSortedSet(new TreeSet<>(subset)))
{
Review Comment:
Why wrap with unmodifiable?
Come to think of it, can't you do subset.sort().forEach(...) ? No copying
is needed either; I looked.
CC @bruno-roustant
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/facet/LegacyAbstractAnalyticsFacetTest.java:
##########
@@ -306,7 +307,10 @@ public <T extends Comparable<T>> Long
calculateMissing(ArrayList<T> list, String
}
public static SolrQueryRequest request(String... args) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class));
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new));
Review Comment:
don't collect then toArray; just go straight to toArray
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
