[CALCITE-3823] Do not use String.replaceAll

String.replaceAll uses regex, which is inefficient, and may not be what we want. For strings, use either String.replace; for regex use Pattern.compile().matcher().replaceAll(), being sure to store the pattern for future use. Add entries to forbidden-apis/signatures.txt to prevent people using String.replaceAll in future.
hoperunChen · Mar 9, 2020 · 4208d0b · 4208d0b
1 parent 91f5bb5
commit 4208d0b
Show file tree

Hide file tree

Showing 31 changed files with 129 additions and 95 deletions.
diff --git a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
@@ -141,6 +141,8 @@ public Enumerator<Object[]> enumerator() {
   private static final ThreadLocal<Map<String, AtomicLong>> THREAD_SEQUENCES =
       ThreadLocal.withInitial(HashMap::new);
 
+  private static final Pattern PATTERN_0_STAR_E = Pattern.compile("0*E");
+
   private SqlFunctions() {
   }
 
@@ -599,7 +601,7 @@ public static boolean posixRegex(String s, String regex, Boolean caseSensitive)
     String[] existingExpressions = Arrays.stream(POSIX_CHARACTER_CLASSES)
         .filter(v -> originalRegex.contains(v.toLowerCase(Locale.ROOT))).toArray(String[]::new);
     for (String v : existingExpressions) {
-      regex = regex.replaceAll(v.toLowerCase(Locale.ROOT), "\\\\p{" + v + "}");
+      regex = regex.replace(v.toLowerCase(Locale.ROOT), "\\p{" + v + "}");
     }
 
     int flags = caseSensitive ? 0 : Pattern.CASE_INSENSITIVE;
@@ -1683,7 +1685,7 @@ public static String toString(float x) {
     BigDecimal bigDecimal =
         new BigDecimal(x, MathContext.DECIMAL32).stripTrailingZeros();
     final String s = bigDecimal.toString();
-    return s.replaceAll("0*E", "E").replace("E+", "E");
+    return PATTERN_0_STAR_E.matcher(s).replaceAll("E").replace("E+", "E");
   }
 
   /** CAST(DOUBLE AS VARCHAR). */
@@ -1694,16 +1696,18 @@ public static String toString(double x) {
     BigDecimal bigDecimal =
         new BigDecimal(x, MathContext.DECIMAL64).stripTrailingZeros();
     final String s = bigDecimal.toString();
-    return s.replaceAll("0*E", "E").replace("E+", "E");
+    return PATTERN_0_STAR_E.matcher(s).replaceAll("E").replace("E+", "E");
   }
 
   /** CAST(DECIMAL AS VARCHAR). */
   public static String toString(BigDecimal x) {
     final String s = x.toString();
-    if (s.startsWith("0")) {
+    if (s.equals("0")) {
+      return s;
+    } else if (s.startsWith("0.")) {
       // we want ".1" not "0.1"
       return s.substring(1);
-    } else if (s.startsWith("-0")) {
+    } else if (s.startsWith("-0.")) {
       // we want "-.1" not "-0.1"
       return "-" + s.substring(2);
     } else {

diff --git a/core/src/main/java/org/apache/calcite/sql/SqlDialect.java b/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
@@ -346,14 +346,7 @@ public RelDataTypeSystem getTypeSystem() {
    * @return Quoted identifier
    */
   public String quoteIdentifier(String val) {
-    if (identifierQuoteString == null) {
-      return val; // quoting is not supported
-    }
-    String val2 =
-        val.replaceAll(
-            identifierEndQuoteString,
-            identifierEscapedQuote);
-    return identifierQuoteString + val2 + identifierEndQuoteString;
+    return quoteIdentifier(new StringBuilder(), val).toString();
   }
 
   /**
@@ -375,12 +368,8 @@ public StringBuilder quoteIdentifier(
         || !identifierNeedsQuote(val)) {
       buf.append(val);
     } else {
-      String val2 =
-          val.replaceAll(
-              identifierEndQuoteString,
-              identifierEscapedQuote);
       buf.append(identifierQuoteString);
-      buf.append(val2);
+      buf.append(val.replace(identifierEndQuoteString, identifierEscapedQuote));
       buf.append(identifierEndQuoteString);
     }
     return buf;

diff --git a/core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java b/core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java
@@ -94,10 +94,6 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
     super(context);
   }
 
-  @Override public String quoteIdentifier(String val) {
-    return quoteIdentifier(new StringBuilder(), val).toString();
-  }
-
   @Override protected boolean identifierNeedsQuote(String val) {
     return !IDENTIFIER_REGEX.matcher(val).matches()
         || RESERVED_KEYWORDS.contains(val.toUpperCase(Locale.ROOT));

diff --git a/core/src/main/java/org/apache/calcite/sql/parser/SqlParserUtil.java b/core/src/main/java/org/apache/calcite/sql/parser/SqlParserUtil.java
@@ -315,12 +315,12 @@ public static int parsePositiveInt(String value) {
    */
   @Deprecated // to be removed before 2.0
   public static byte[] parseBinaryString(String s) {
-    s = s.replaceAll(" ", "");
-    s = s.replaceAll("\n", "");
-    s = s.replaceAll("\t", "");
-    s = s.replaceAll("\r", "");
-    s = s.replaceAll("\f", "");
-    s = s.replaceAll("'", "");
+    s = s.replace(" ", "");
+    s = s.replace("\n", "");
+    s = s.replace("\t", "");
+    s = s.replace("\r", "");
+    s = s.replace("\f", "");
+    s = s.replace("'", "");
 
     if (s.length() == 0) {
       return new byte[0];

diff --git a/core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java b/core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java
@@ -995,14 +995,14 @@ public void print(int x) {
   }
 
   public void identifier(String name, boolean quoted) {
-    String qName = name;
     // If configured globally or the original identifier is quoted,
     // then quotes the identifier.
+    maybeWhitespace(name);
     if (isQuoteAllIdentifiers() || quoted) {
-      qName = dialect.quoteIdentifier(name);
+      dialect.quoteIdentifier(buf, name);
+    } else {
+      buf.append(name);
     }
-    maybeWhitespace(qName);
-    buf.append(qName);
     setNeedWhitespace(true);
   }
 

diff --git a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
@@ -1023,7 +1023,7 @@ private static void findAllValidFunctionNames(
             || (op.getSyntax() == SqlSyntax.PREFIX)) {
           if (op.getOperandTypeChecker() != null) {
             String sig = op.getAllowedSignatures();
-            sig = sig.replaceAll("'", "");
+            sig = sig.replace("'", "");
             result.add(
                 new SqlMonikerImpl(
                     sig,

diff --git a/core/src/main/java/org/apache/calcite/util/BitString.java b/core/src/main/java/org/apache/calcite/util/BitString.java
@@ -49,7 +49,7 @@ public class BitString {
   protected BitString(
       String bits,
       int bitCount) {
-    assert bits.replaceAll("1", "").replaceAll("0", "").length() == 0
+    assert bits.replace("1", "").replace("0", "").length() == 0
         : "bit string '" + bits + "' contains digits other than {0, 1}";
     this.bits = bits;
     this.bitCount = bitCount;

diff --git a/core/src/main/java/org/apache/calcite/util/Util.java b/core/src/main/java/org/apache/calcite/util/Util.java
@@ -741,7 +741,7 @@ public static boolean isValidJavaIdentifier(String s) {
   }
 
   public static String toLinux(String s) {
-    return s.replaceAll("\r\n", "\n");
+    return s.replace("\r\n", "\n");
   }
 
   /**

diff --git a/core/src/test/java/org/apache/calcite/profile/ProfilerTest.java b/core/src/test/java/org/apache/calcite/profile/ProfilerTest.java
@@ -629,8 +629,9 @@ public String apply(Profiler.Statistic statistic) {
           map1.keySet().retainAll(Fluid.this.columns);
         }
         final String json = jb.toJsonString(map);
-        return json.replaceAll("\n", "").replaceAll(" ", "")
-            .replaceAll("\"", "");
+        return json.replace("\n", "")
+            .replace(" ", "")
+            .replace("\"", "");
       }
     }
   }

diff --git a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
@@ -1345,7 +1345,7 @@ private static String toSql(RelNode root, SqlDialect dialect) {
         + " 4 AS \"fo$ur\", 5 AS \"ignore\"\n"
         + "FROM \"foodmart\".\"days\") AS \"t\"\n"
         + "WHERE \"one\" < \"tWo\" AND \"THREE\" < \"fo$ur\"";
-    final String expectedOracle = expectedPostgresql.replaceAll(" AS ", " ");
+    final String expectedOracle = expectedPostgresql.replace(" AS ", " ");
     sql(query)
         .withBigQuery().ok(expectedBigQuery)
         .withMysql().ok(expectedMysql)

diff --git a/core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java b/core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java
@@ -7451,7 +7451,7 @@ public void subTestIntervalSecondFailsValidation() {
     sql(in).ok(out);
 
     // Verify that we can override with an explicit escape character
-    sql(in.replaceAll("\\\\", "!") + "UESCAPE '!'").ok(out);
+    sql(in.replace("\\", "!") + "UESCAPE '!'").ok(out);
   }
 
   @Test public void testIllegalUnicodeEscape() {

diff --git a/core/src/test/java/org/apache/calcite/sql/test/SqlPrettyWriterTest.java b/core/src/test/java/org/apache/calcite/sql/test/SqlPrettyWriterTest.java
@@ -134,7 +134,7 @@ Sql check() {
 
       // Now parse the result, and make sure it is structurally equivalent
       // to the original.
-      final String actual2 = formatted.replaceAll("`", "\"");
+      final String actual2 = formatted.replace("`", "\"");
       final SqlNode node2;
       if (expr) {
         final SqlCall valuesCall =

diff --git a/core/src/test/java/org/apache/calcite/test/CalciteAssert.java b/core/src/test/java/org/apache/calcite/test/CalciteAssert.java
@@ -457,8 +457,7 @@ public static Consumer<ResultSet> checkMaskedResultContains(
     return s -> {
       try {
         final String actual = Util.toLinux(toString(s));
-        final String maskedActual =
-            actual.replaceAll(", id = [0-9]+", "");
+        final String maskedActual = Matchers.trimNodeIds(actual);
         assertThat(maskedActual, containsString(expected));
       } catch (SQLException e) {
         throw TestUtil.rethrow(e);
@@ -1130,7 +1129,7 @@ public final AssertThat withMaterializations(String model, final boolean existin
             map.put("view", table + "v");
           }
           String sql = materializations[i];
-          final String sql2 = sql.replaceAll("`", "\"");
+          final String sql2 = sql.replace("`", "\"");
           map.put("sql", sql2);
           list.add(map);
         }
@@ -1737,7 +1736,7 @@ public AssertQuery planUpdateHasSql(String expected, int count) {
       } else {
         final String message =
             "Plan [" + plan + "] contains [" + expected.java + "]";
-        final String actualJava = toLinux(plan).replaceAll("\\\\r\\\\n", "\\\\n");
+        final String actualJava = toLinux(plan);
         assertTrue(actualJava.contains(expected.java), message);
       }
       return this;
@@ -2172,7 +2171,7 @@ private static String wrap(String sql) {
       return START
           + sql.replace("\\", "\\\\")
               .replace("\"", "\\\"")
-              .replaceAll("\n", "\\\\n")
+              .replace("\n", "\\\\n")
           + END;
     }
 

diff --git a/core/src/test/java/org/apache/calcite/test/DiffTestCase.java b/core/src/test/java/org/apache/calcite/test/DiffTestCase.java
@@ -74,8 +74,10 @@ public abstract class DiffTestCase {
    */
   // private List diffMasks;
   private String diffMasks;
+  Pattern compiledDiffPattern;
   Matcher compiledDiffMatcher;
   private String ignorePatterns;
+  Pattern compiledIgnorePattern;
   Matcher compiledIgnoreMatcher;
 
   /**
@@ -285,7 +287,7 @@ protected void addDiffMask(String mask) {
     } else {
       diffMasks = diffMasks + "|" + mask;
     }
-    Pattern compiledDiffPattern = Pattern.compile(diffMasks);
+    compiledDiffPattern = Pattern.compile(diffMasks);
     compiledDiffMatcher = compiledDiffPattern.matcher("");
   }
 
@@ -295,7 +297,7 @@ protected void addIgnorePattern(String javaPattern) {
     } else {
       ignorePatterns = ignorePatterns + "|" + javaPattern;
     }
-    Pattern compiledIgnorePattern = Pattern.compile(ignorePatterns);
+    compiledIgnorePattern = Pattern.compile(ignorePatterns);
     compiledIgnoreMatcher = compiledIgnorePattern.matcher("");
   }
 
@@ -306,7 +308,7 @@ private String applyDiffMask(String s) {
       // we assume most of lines do not match
       // so compiled matches will be faster than replaceAll.
       if (compiledDiffMatcher.find()) {
-        return s.replaceAll(diffMasks, "XYZZY");
+        return compiledDiffPattern.matcher(s).replaceAll("XYZZY");
       }
     }
     return s;
@@ -328,7 +330,7 @@ private void diffFail(
     if (verbose) {
       if (inIde()) {
         // If we're in IntelliJ, it's worth printing the 'expected
-        // <...> actual <...>' string, becauase IntelliJ can format
+        // <...> actual <...>' string, because IntelliJ can format
         // this intelligently. Otherwise, use the more concise
         // diff format.
         assertEquals(fileContents(refFile), fileContents(logFile), message);

diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
@@ -3694,7 +3694,7 @@ public void checkOrderBy(final boolean desc,
     String planLine =
         "a0s0w0 = org.apache.calcite.runtime.SqlFunctions.lesser(a0s0w0, org.apache.calcite.runtime.SqlFunctions.toFloat(_rows[j]));";
     if (CalciteSystemProperty.DEBUG.value()) {
-      planLine = planLine.replaceAll("a0s0w0", "MINa0s0w0");
+      planLine = planLine.replace("a0s0w0", "MINa0s0w0");
     }
     CalciteAssert.hr()
         .query("select min(\"salary\"+1) over w as m\n"
@@ -3719,7 +3719,7 @@ public void checkOrderBy(final boolean desc,
     String planLine =
         "a0s0w0 = org.apache.calcite.runtime.SqlFunctions.lesser(a0s0w0, org.apache.calcite.runtime.SqlFunctions.toFloat(_rows[j]));";
     if (CalciteSystemProperty.DEBUG.value()) {
-      planLine = planLine.replaceAll("a0s0w0", "MINa0s0w0");
+      planLine = planLine.replace("a0s0w0", "MINa0s0w0");
     }
     CalciteAssert.hr()
         .query("select 1+min(\"salary\"+1) over w as m\n"

diff --git a/core/src/test/java/org/apache/calcite/test/Matchers.java b/core/src/test/java/org/apache/calcite/test/Matchers.java
@@ -43,12 +43,16 @@
 import java.util.List;
 import java.util.Objects;
 import java.util.function.Function;
+import java.util.regex.Pattern;
 import java.util.stream.StreamSupport;
 
 /**
  * Matchers for testing SQL queries.
  */
 public class Matchers {
+
+  private static final Pattern PATTERN = Pattern.compile(", id = [0-9]+");
+
   private Matchers() {}
 
   /** Allows passing the actual result from the {@code matchesSafely} method to
@@ -248,7 +252,7 @@ public static Matcher<String> containsStringLinux(String value) {
   }
 
   public static String trimNodeIds(String s) {
-    return s.replaceAll(", id = [0-9]+", "");
+    return PATTERN.matcher(s).replaceAll("");
   }
 
   /**

diff --git a/core/src/test/java/org/apache/calcite/test/MaterializationTest.java b/core/src/test/java/org/apache/calcite/test/MaterializationTest.java
@@ -2580,7 +2580,7 @@ private void checkSatisfiable(RexNode e, String s) {
                 map.put("table", "locations");
                 String sql = "select distinct `deptno` as `empid`, '' as `name`\n"
                     + "from `emps`";
-                final String sql2 = sql.replaceAll("`", "\"");
+                final String sql2 = sql.replace("`", "\"");
                 map.put("sql", sql2);
                 return ImmutableList.of(map);
               })

diff --git a/core/src/test/java/org/apache/calcite/test/QuidemTest.java b/core/src/test/java/org/apache/calcite/test/QuidemTest.java
@@ -55,13 +55,17 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.function.Function;
+import java.util.regex.Pattern;
 
 import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * Test that runs every Quidem file as a test.
  */
 public abstract class QuidemTest {
+
+  private static final Pattern PATTERN = Pattern.compile("\\.iq$");
+
   private static Object getEnv(String varName) {
     switch (varName) {
     case "jdk18":
@@ -84,9 +88,9 @@ private static Object getEnv(String varName) {
 
   private Method findMethod(String path) {
     // E.g. path "sql/agg.iq" gives method "testSqlAgg"
-    String methodName =
-        AvaticaUtils.toCamelCase(
-            "test_" + path.replace(File.separatorChar, '_').replaceAll("\\.iq$", ""));
+    final String path1 = path.replace(File.separatorChar, '_');
+    final String path2 = PATTERN.matcher(path1).replaceAll("");
+    String methodName = AvaticaUtils.toCamelCase("test_" + path2);
     Method m;
     try {
       m = getClass().getMethod(methodName, String.class);

diff --git a/core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java b/core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java
@@ -73,6 +73,35 @@ public class SqlFunctionsTest {
     assertThat(charLength("xyz"), is(3));
   }
 
+  @Test public void testToString() {
+    assertThat(SqlFunctions.toString(0f), is("0E0"));
+    assertThat(SqlFunctions.toString(1f), is("1"));
+    assertThat(SqlFunctions.toString(1.5f), is("1.5"));
+    assertThat(SqlFunctions.toString(-1.5f), is("-1.5"));
+    assertThat(SqlFunctions.toString(1.5e8f), is("1.5E8"));
+    assertThat(SqlFunctions.toString(-0.0625f), is("-0.0625"));
+    assertThat(SqlFunctions.toString(0.0625f), is("0.0625"));
+    assertThat(SqlFunctions.toString(-5e-12f), is("-5E-12"));
+
+    assertThat(SqlFunctions.toString(0d), is("0E0"));
+    assertThat(SqlFunctions.toString(1d), is("1"));
+    assertThat(SqlFunctions.toString(1.5d), is("1.5"));
+    assertThat(SqlFunctions.toString(-1.5d), is("-1.5"));
+    assertThat(SqlFunctions.toString(1.5e8d), is("1.5E8"));
+    assertThat(SqlFunctions.toString(-0.0625d), is("-0.0625"));
+    assertThat(SqlFunctions.toString(0.0625d), is("0.0625"));
+    assertThat(SqlFunctions.toString(-5e-12d), is("-5E-12"));
+
+    assertThat(SqlFunctions.toString(new BigDecimal("0")), is("0"));
+    assertThat(SqlFunctions.toString(new BigDecimal("1")), is("1"));
+    assertThat(SqlFunctions.toString(new BigDecimal("1.5")), is("1.5"));
+    assertThat(SqlFunctions.toString(new BigDecimal("-1.5")), is("-1.5"));
+    assertThat(SqlFunctions.toString(new BigDecimal("1.5e8")), is("1.5E+8"));
+    assertThat(SqlFunctions.toString(new BigDecimal("-0.0625")), is("-.0625"));
+    assertThat(SqlFunctions.toString(new BigDecimal("0.0625")), is(".0625"));
+    assertThat(SqlFunctions.toString(new BigDecimal("-5e-12")), is("-5E-12"));
+  }
+
   @Test public void testConcat() {
     assertThat(concat("a b", "cd"), is("a bcd"));
     // The code generator will ensure that nulls are never passed in. If we
-Original file line number
+Diff line change
@@ Expand Up / @@ -741,7 +741,7 @@ public static boolean isValidJavaIdentifier(String s) { @@
       }
       public static String toLinux(String s) {
-        return s.replaceAll("\r\n", "\n");
+        return s.replace("\r\n", "\n");
       }
       /**
@@ Expand Down @@