From 2dea44994922dacec38f81cc68c42360bdc449d7 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 3 May 2016 09:12:28 -0400 Subject: [PATCH] Remove Strings#splitStringToArray This commit removes the method Strings#splitStringToArray and replaces the call sites with invocations to String#split. There are only two explanations for the existence of this method. The first is that String#split is slightly tricky in that it accepts a regular expression rather than a character to split on. This means that if s is a string, s.split(".") does not split on the character '.', but rather splits on the regular expression '.' which splits on every character (of course, this is easily fixed by invoking s.split("\\.") instead). The second possible explanation is that (again) String#split accepts a regular expression. This means that there could be a performance concern compared to just splitting on a single character. However, it turns out that String#split has a fast path for the case of splitting on a single character and microbenchmarks show that String#split has 1.5x--2x the throughput of Strings#splitStringToArray. There is a slight behavior difference between Strings#splitStringToArray and String#split: namely, the former would return an empty array in cases when the input string was null or empty but String#split will just NPE at the call site on null and return a one-element array containing the empty string when the input string is empty. There was only one place relying on this behavior and the call site has been modified accordingly. --- .../action/search/TransportSearchHelper.java | 3 +- .../org/elasticsearch/common/Strings.java | 40 +------------------ .../java/org/elasticsearch/common/Table.java | 2 +- .../elasticsearch/common/path/PathTrie.java | 13 +++--- .../common/settings/Settings.java | 13 ------ .../common/util/BloomFilter.java | 3 +- .../xcontent/support/XContentMapValues.java | 4 +- .../index/mapper/DocumentParser.java | 17 ++++---- .../index/query/GeoShapeQueryBuilder.java | 3 +- .../ingest/core/IngestDocument.java | 4 +- .../admin/cluster/node/tasks/TasksIT.java | 2 +- .../common/settings/SettingsTests.java | 18 --------- .../cloud/gce/network/GceNameResolver.java | 2 +- .../repositories/azure/AzureRepository.java | 2 +- .../repositories/s3/S3Repository.java | 2 +- .../ingest/RandomDocumentPicks.java | 4 +- 16 files changed, 29 insertions(+), 103 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/search/TransportSearchHelper.java b/core/src/main/java/org/elasticsearch/action/search/TransportSearchHelper.java index 49d4c65add1..8da195b739b 100644 --- a/core/src/main/java/org/elasticsearch/action/search/TransportSearchHelper.java +++ b/core/src/main/java/org/elasticsearch/action/search/TransportSearchHelper.java @@ -24,7 +24,6 @@ import org.apache.lucene.util.CharsRefBuilder; import org.elasticsearch.cluster.routing.ShardRouting; import org.elasticsearch.common.Base64; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.util.concurrent.AtomicArray; import org.elasticsearch.search.SearchPhaseResult; import org.elasticsearch.search.internal.InternalScrollSearchRequest; @@ -89,7 +88,7 @@ final class TransportSearchHelper { } catch (Exception e) { throw new IllegalArgumentException("Failed to decode scrollId", e); } - String[] elements = Strings.splitStringToArray(spare.get(), ';'); + String[] elements = spare.get().toString().split(";"); if (elements.length < 2) { throw new IllegalArgumentException("Malformed scrollId [" + scrollId + "]"); } diff --git a/core/src/main/java/org/elasticsearch/common/Strings.java b/core/src/main/java/org/elasticsearch/common/Strings.java index 151c53e2007..6b6c31c1522 100644 --- a/core/src/main/java/org/elasticsearch/common/Strings.java +++ b/core/src/main/java/org/elasticsearch/common/Strings.java @@ -38,7 +38,6 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Properties; -import java.util.Random; import java.util.Set; import java.util.StringTokenizer; import java.util.TreeSet; @@ -557,7 +556,8 @@ public class Strings { } public static String[] splitStringByCommaToArray(final String s) { - return splitStringToArray(s, ','); + if (s == null || s.isEmpty()) return Strings.EMPTY_ARRAY; + else return s.split(","); } public static Set splitStringToSet(final String s, final char c) { @@ -588,42 +588,6 @@ public class Strings { return result; } - public static String[] splitStringToArray(final CharSequence s, final char c) { - if (s == null || s.length() == 0) { - return Strings.EMPTY_ARRAY; - } - int count = 1; - for (int i = 0; i < s.length(); i++) { - if (s.charAt(i) == c) { - count++; - } - } - final String[] result = new String[count]; - final StringBuilder builder = new StringBuilder(); - int res = 0; - for (int i = 0; i < s.length(); i++) { - if (s.charAt(i) == c) { - if (builder.length() > 0) { - result[res++] = builder.toString(); - builder.setLength(0); - } - - } else { - builder.append(s.charAt(i)); - } - } - if (builder.length() > 0) { - result[res++] = builder.toString(); - } - if (res != count) { - // we have empty strings, copy over to a new array - String[] result1 = new String[res]; - System.arraycopy(result, 0, result1, 0, res); - return result1; - } - return result; - } - /** * Split a String at the first occurrence of the delimiter. * Does not include the delimiter in the result. diff --git a/core/src/main/java/org/elasticsearch/common/Table.java b/core/src/main/java/org/elasticsearch/common/Table.java index 0d4a827202d..ab0252b11dc 100644 --- a/core/src/main/java/org/elasticsearch/common/Table.java +++ b/core/src/main/java/org/elasticsearch/common/Table.java @@ -149,7 +149,7 @@ public class Table { // get the attributes of the header cell we are going to add mAttr.putAll(headers.get(currentCells.size()).attr); } - String[] sAttrs = Strings.splitStringToArray(attributes, ';'); + String[] sAttrs = attributes.split(";"); for (String sAttr : sAttrs) { if (sAttr.length() == 0) { continue; diff --git a/core/src/main/java/org/elasticsearch/common/path/PathTrie.java b/core/src/main/java/org/elasticsearch/common/path/PathTrie.java index 704468f7533..3c25a9caa52 100644 --- a/core/src/main/java/org/elasticsearch/common/path/PathTrie.java +++ b/core/src/main/java/org/elasticsearch/common/path/PathTrie.java @@ -19,8 +19,6 @@ package org.elasticsearch.common.path; -import org.elasticsearch.common.Strings; - import java.util.HashMap; import java.util.Map; @@ -38,7 +36,7 @@ public class PathTrie { private final Decoder decoder; private final TrieNode root; - private final char separator; + private final String separator; private T rootValue; public PathTrie(Decoder decoder) { @@ -47,8 +45,9 @@ public class PathTrie { public PathTrie(char separator, String wildcard, Decoder decoder) { this.decoder = decoder; - this.separator = separator; - root = new TrieNode(new String(new char[]{separator}), null, wildcard); + final String separatorAsString = new String(new char[]{separator}); + this.separator = separatorAsString; + root = new TrieNode(separatorAsString, null, wildcard); } public class TrieNode { @@ -196,7 +195,7 @@ public class PathTrie { } public void insert(String path, T value) { - String[] strings = Strings.splitStringToArray(path, separator); + String[] strings = path.split(separator); if (strings.length == 0) { rootValue = value; return; @@ -217,7 +216,7 @@ public class PathTrie { if (path.length() == 0) { return rootValue; } - String[] strings = Strings.splitStringToArray(path, separator); + String[] strings = path.split(separator); if (strings.length == 0) { return rootValue; } diff --git a/core/src/main/java/org/elasticsearch/common/settings/Settings.java b/core/src/main/java/org/elasticsearch/common/settings/Settings.java index 1c243255454..8488ca75c73 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -899,19 +899,6 @@ public final class Settings implements ToXContent { return this; } - public Builder loadFromDelimitedString(String value, char delimiter) { - String[] values = Strings.splitStringToArray(value, delimiter); - for (String s : values) { - int index = s.indexOf('='); - if (index == -1) { - throw new IllegalArgumentException( - "value [" + s + "] for settings loaded with delimiter [" + delimiter + "] is malformed, missing ="); - } - map.put(s.substring(0, index), s.substring(index + 1)); - } - return this; - } - /** * Loads settings from the actual string content that represents them using the * {@link SettingsLoaderFactory#loaderFromSource(String)}. diff --git a/core/src/main/java/org/elasticsearch/common/util/BloomFilter.java b/core/src/main/java/org/elasticsearch/common/util/BloomFilter.java index b9dd6859ce0..6c471cddb55 100644 --- a/core/src/main/java/org/elasticsearch/common/util/BloomFilter.java +++ b/core/src/main/java/org/elasticsearch/common/util/BloomFilter.java @@ -24,7 +24,6 @@ import org.apache.lucene.store.IndexInput; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.RamUsageEstimator; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.hash.MurmurHash3; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -75,7 +74,7 @@ public class BloomFilter { if (config == null) { return buildDefault(); } - String[] sEntries = Strings.splitStringToArray(config, ','); + String[] sEntries = config.split(","); if (sEntries.length == 0) { if (config.length() > 0) { return new Factory(new Entry[]{new Entry(0, Double.parseDouble(config))}); diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java b/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java index 4612d3f05d0..a8c120f424b 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java @@ -40,7 +40,7 @@ public class XContentMapValues { */ public static List extractRawValues(String path, Map map) { List values = new ArrayList<>(); - String[] pathElements = Strings.splitStringToArray(path, '.'); + String[] pathElements = path.split("\\."); if (pathElements.length == 0) { return values; } @@ -93,7 +93,7 @@ public class XContentMapValues { } public static Object extractValue(String path, Map map) { - String[] pathElements = Strings.splitStringToArray(path, '.'); + String[] pathElements = path.split("\\."); if (pathElements.length == 0) { return null; } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 3d191e4d80a..a666ffc0ed5 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -19,13 +19,6 @@ package org.elasticsearch.index.mapper; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.Iterator; -import java.util.List; - import org.apache.lucene.document.Field; import org.apache.lucene.index.IndexableField; import org.elasticsearch.Version; @@ -51,6 +44,13 @@ import org.elasticsearch.index.mapper.internal.UidFieldMapper; import org.elasticsearch.index.mapper.object.ArrayValueMapperParser; import org.elasticsearch.index.mapper.object.ObjectMapper; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; + /** A parser for documents, given mappings from a DocumentMapper */ final class DocumentParser { @@ -829,8 +829,7 @@ final class DocumentParser { // The path of the dest field might be completely different from the current one so we need to reset it context = context.overridePath(new ContentPath(0)); - // TODO: why Strings.splitStringToArray instead of String.split? - final String[] paths = Strings.splitStringToArray(field, '.'); + final String[] paths = field.split("\\."); final String fieldName = paths[paths.length-1]; ObjectMapper mapper = context.root(); ObjectMapper[] mappers = new ObjectMapper[paths.length-1]; diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java index 8ff2f697b45..d3c47d5bc70 100644 --- a/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java @@ -33,7 +33,6 @@ import org.elasticsearch.action.get.GetResponse; import org.elasticsearch.client.Client; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.geo.ShapeRelation; import org.elasticsearch.common.geo.ShapesAvailability; import org.elasticsearch.common.geo.SpatialStrategy; @@ -379,7 +378,7 @@ public class GeoShapeQueryBuilder extends AbstractQueryBuilder(node.getName(), actionMasks), listener); assertNull(oldListener); diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index ced961a70b8..3539e54d943 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -38,24 +38,6 @@ import static org.hamcrest.Matchers.nullValue; public class SettingsTests extends ESTestCase { - public void testLoadFromDelimitedString() { - Settings settings = Settings.builder() - .loadFromDelimitedString("key1=value1;key2=value2", ';') - .build(); - assertThat(settings.get("key1"), equalTo("value1")); - assertThat(settings.get("key2"), equalTo("value2")); - assertThat(settings.getAsMap().size(), equalTo(2)); - assertThat(settings.toDelimitedString(';'), equalTo("key1=value1;key2=value2;")); - - settings = Settings.builder() - .loadFromDelimitedString("key1=value1;key2=value2;", ';') - .build(); - assertThat(settings.get("key1"), equalTo("value1")); - assertThat(settings.get("key2"), equalTo("value2")); - assertThat(settings.getAsMap().size(), equalTo(2)); - assertThat(settings.toDelimitedString(';'), equalTo("key1=value1;key2=value2;")); - } - public void testReplacePropertiesPlaceholderSystemProperty() { String value = System.getProperty("java.home"); assertFalse(value.isEmpty()); diff --git a/plugins/discovery-gce/src/main/java/org/elasticsearch/cloud/gce/network/GceNameResolver.java b/plugins/discovery-gce/src/main/java/org/elasticsearch/cloud/gce/network/GceNameResolver.java index 22d79fb1614..0bd5e07da91 100644 --- a/plugins/discovery-gce/src/main/java/org/elasticsearch/cloud/gce/network/GceNameResolver.java +++ b/plugins/discovery-gce/src/main/java/org/elasticsearch/cloud/gce/network/GceNameResolver.java @@ -92,7 +92,7 @@ public class GceNameResolver extends AbstractComponent implements CustomNameReso } else if (value.startsWith(GceAddressResolverType.PRIVATE_IP.configName)) { // We extract the network interface from gce:privateIp:XX String network = "0"; - String[] privateIpConfig = Strings.splitStringToArray(value, ':'); + String[] privateIpConfig = value.split(":"); if (privateIpConfig != null && privateIpConfig.length == 3) { network = privateIpConfig[2]; } diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java b/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java index 66db57fdd92..8af614df605 100644 --- a/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java +++ b/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java @@ -116,7 +116,7 @@ public class AzureRepository extends BlobStoreRepository { // Remove starting / if any basePath = Strings.trimLeadingCharacter(basePath, '/'); BlobPath path = new BlobPath(); - for(String elem : Strings.splitStringToArray(basePath, '/')) { + for(String elem : basePath.split("/")) { path = path.add(elem); } this.basePath = path; diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java index a09d57ebc93..5861893d6b2 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java @@ -287,7 +287,7 @@ public class S3Repository extends BlobStoreRepository { String basePath = getValue(repositorySettings, Repository.BASE_PATH_SETTING, Repositories.BASE_PATH_SETTING); if (Strings.hasLength(basePath)) { BlobPath path = new BlobPath(); - for(String elem : Strings.splitStringToArray(basePath, '/')) { + for(String elem : basePath.split("/")) { path = path.add(elem); } this.basePath = path; diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/RandomDocumentPicks.java b/test/framework/src/main/java/org/elasticsearch/ingest/RandomDocumentPicks.java index c42d51b7abd..6dc920c6ff8 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/RandomDocumentPicks.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/RandomDocumentPicks.java @@ -22,8 +22,6 @@ package org.elasticsearch.ingest; import com.carrotsearch.randomizedtesting.generators.RandomInts; import com.carrotsearch.randomizedtesting.generators.RandomPicks; import com.carrotsearch.randomizedtesting.generators.RandomStrings; -import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.ingest.core.IngestDocument; import java.util.ArrayList; @@ -105,7 +103,7 @@ public final class RandomDocumentPicks { * that each node of the tree either doesn't exist or is a map, otherwise new fields cannot be added. */ public static boolean canAddField(String path, IngestDocument ingestDocument) { - String[] pathElements = Strings.splitStringToArray(path, '.'); + String[] pathElements = path.split("\\."); Map innerMap = ingestDocument.getSourceAndMetadata(); if (pathElements.length > 1) { for (int i = 0; i < pathElements.length - 1; i++) {