You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2023/03/21 21:01:32 UTC
[GitHub] [solr] dsmiley commented on a diff in pull request #1466: SOLR-16713: Replace Guava usages with pure Java
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org