Core, Open-API, Spark, Flink: Use String.replace for literal replacements

[uros-b]

Several places used String.replaceAll with literal (non-regex) patterns, which compiles a regex Pattern on every call. Switch these to the char/CharSequence String.replace overloads, which avoid the per-call compilation. Behavior-identical.

Covered call sites: ExpressionParser and ReportMetricsRequestParser (core), RCKUtils (open-api), CreateChangelogViewProcedure and TestRewriteDataFilesProcedure (Spark 3.5, 4.0, 4.1), and MetricsReporterFactoryForTests (Flink 1.20, 2.0, 2.1). This covers every remaining literal replaceAll call; the ones left in the repo are genuine regexes.

[uros-b]

Waiting for CI, cc @szehon-ho for review

[ebyhr]

Could you confirm other replaceAll usages? e.g. ReportMetricsRequestParser, RCKUtils

[uros-b]

Good catch, thank you @ebyhr!

[ebyhr]

We can update more:

iceberg/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java

Line 297 in 58d5c37

return "`" + columnName.replaceAll("`", "``") + "`";

iceberg/spark/v4.1/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java

Line 806 in 58d5c37

TableIdentifier.of("default", QUOTED_SPECIAL_CHARS_TABLE_NAME.replaceAll("`", ""));

iceberg/spark/v4.1/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java

Line 844 in 58d5c37

TableIdentifier.of("default", QUOTED_SPECIAL_CHARS_TABLE_NAME.replaceAll("`", ""));

iceberg/spark/v4.1/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java

Line 887 in 58d5c37

TableIdentifier.of("default", QUOTED_SPECIAL_CHARS_TABLE_NAME.replaceAll("`", ""));

[uros-b]

Thank you @ebyhr! Made additional changes, PTAL. Also cc @szehon-ho

[uros-b]

cc @huaxingao

[RussellSpitzer]

So this is one of those PR's that I know some folks are not a big fan of. We are touching a lot of files (lot of churn) and while there is a theoretical performance benefit here, in practice it is probably a noop.

Personally I don't mind this but it is worth questioning "Why" is this necessary. Is there a practical reason for this?

[nssalian]

It was initially scoped to the ExpressionParser which was ok to go in but expanded. +1 on Russell's comment.

[uros-b]

Yeah I agree with you folks, shall we scope back to original scope (ExpressionParser) and proceed with narrow change?

[uros-b]

I'm fine either way, I think it's worth fixing but also want to strike the correct scope (not too narrow, not too wide). Let me know what you think!

[szehon-ho]

Hm, replace is better for perf than replaceAll. It's not called enough to make huge difference, but I dont see why it's a bad improvement. Moreover, I think replaceAll doesnt make sense here, as replaceAll interprets regex's (which is not what is meant in these call sites.)

Maybe we can change the core files? I dont think those particular files are touched that much? The test changes probably wont matter that much to warrant church, and we can do it separately if needed

[RussellSpitzer]

Hm, replace is better for perf than replaceAll. It's not called enough to make huge difference, but I dont see why it's a bad improvement. Moreover, I think replaceAll doesnt make sense here, as replaceAll interprets regex's (which is not what is meant in these call sites.)

Maybe we can change the core files? I dont think those particular files are touched that much? The test changes probably wont matter that much to warrant church, and we can do it separately if needed

So the general call here is how much do we want to encourage PR's that are essentially a Noop on the project (not actually more secure or faster.) I'd feel better if we didn't already have such a huge burden on reviewers but this is more of a philosophical question. We want to encourage contributors to make meaningful changes to the code base. Now in this case if we think this is important and these are the only cases where it happens, rather than a one off correction we should be locking this out of the codebase entirely.

<module name="RegexpSinglelineJava">
    <property name="id" value="LiteralStringReplaceAll"/>
    <property name="ignoreComments" value="true"/>
    <property name="format" value="\.replaceAll\(\s*&quot;([^&quot;\\\\.*+?^$()\[\]{}|]*)&quot;"/>
    <property name="message" value="Use String.replace for literal replacements; replaceAll compiles a regex on every call."/>
</module>

Again this is only a rule I would add if we already have a tiny surface area of places to fix since again there really isn't a benefit to doing this but if we can lock out all future mistakes that feels valid to me.

In case folks aren't aware, what i'm trying to avoid is a situation like #16881 where we did a rather large PR which was a also essentially a Noop but it ended up being reverted for the same rational I noted above. That's also being replicated again in #17534 .

[szehon-ho]

So iiuc, the concern from switch discussion is touching too many files to force pr rebase.

It seems nice to change replaceAll and lock it via the rule as you suggest. I had actually thought about it yesterday, but if we want to only allow replaceAll for regex literals, your rule is too strong. Or we could just ban replaceAll for all literal and just force explicit Pattern.compile(...).replaceAll, (which sounds ok to me too, more explicit)

Yea agree its not the most important thing in the world, but the surface area does not seem as big here as the switch pr (84 files there..), but I may have missed additional context from missing that conversation