Random improvements to random things in random places

[wendigo]

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

[Copilot]

Pull Request Overview

This PR introduces several cross-cutting refactorings and enhancements, including a unified numeric literal parser, caching for type signature translations, and cache-backed rule indexing, along with corresponding test and assertion updates.

• Consolidated numeric parsing logic into a new NumericParser and updated NumericParameter and LongLiteral to use it.
• Added caches in TypeSignatureTranslator and RuleIndex for performance; refactored parsing and lookup loops accordingly.
• Updated test code to use more expressive AssertJ assertions and added a new parser test for underscored numeric parameters.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
core/trino-spi/src/main/java/io/trino/spi/block/BlockUtil.java	Removed unused checkValidPositions validation method
core/trino-parser/src/test/java/io/trino/sql/parser/TestTypeParser.java	Added a new assertion for foo(1_000, 2_000) ARRAY parsing
core/trino-parser/src/main/java/io/trino/sql/tree/NumericParser.java	Introduced NumericParser with underscore and base-prefix support
core/trino-parser/src/main/java/io/trino/sql/tree/NumericParameter.java	Stored and exposed parsed numeric value on parameters
core/trino-parser/src/main/java/io/trino/sql/tree/LongLiteral.java	Delegated numeric parsing to NumericParser
core/trino-parser/src/main/java/io/trino/sql/ReservedIdentifiers.java	Cached reserved identifiers set; switched to Locale.ENGLISH
core/trino-main/src/test/java/io/trino/sql/planner/iterative/TestRuleIndex.java	Switched from collect(toSet()) to AssertJ contains... assertions
core/trino-main/src/main/java/io/trino/sql/planner/iterative/ExpressionRewriteRuleSet.java	Replaced stream-based rewrite with loop and builder for clarity
core/trino-main/src/main/java/io/trino/sql/planner/iterative/RuleIndex.java	Reworked getCandidates to use an EvictableCache
core/trino-main/src/main/java/io/trino/sql/planner/iterative/IterativeOptimizer.java	Simplified rule-iteration loop
core/trino-main/src/main/java/io/trino/sql/analyzer/TypeSignatureTranslator.java	Added a cache for data-type parsing; improved identifier checks and caching

Comments suppressed due to low confidence (2)

core/trino-parser/src/main/java/io/trino/sql/ReservedIdentifiers.java:127

• [nitpick] The method name isNotIdentifier is confusing since it returns true for reserved keywords. Consider renaming it to something like isReservedKeyword or isNonIdentifierKeyword for clarity.

private static boolean isNotIdentifier(String name)

core/trino-spi/src/main/java/io/trino/spi/block/BlockUtil.java:60

• [nitpick] The removal of checkValidPositions may break callers if this method was in use. If it’s no longer needed, consider cleaning up references or marking it as deprecated before removal.

static void checkValidPositions(boolean[] positions, int positionCount)

[Copilot]

Wrapping parseTypeSignature failures in a generic RuntimeException loses the original semanticException and TYPE_MISMATCH error code. Consider throwing a semanticException with the appropriate error code to preserve user-facing diagnostics.

Suggested change

	throw new RuntimeException(e);
	throw new TrinoException(TYPE_MISMATCH, e.getMessage(), e);

[losipiuk]

Fuzzer driven development?

[losipiuk]

original looks nicer.

[wendigo]

Yeah but resizes the list instead of preallocating it

[wendigo]

Returns a new builder, expecting the specified number of elements to be added.
If expectedSize is exactly the number of elements added to the builder before ImmutableList.Builder.build is called, the builder is likely to perform better than an unsized builder() would have.

[wendigo]

and the toImmutableList() is:

  private static final Collector<Object, ?, ImmutableList<Object>> TO_IMMUTABLE_LIST =
      Collector.of(
          ImmutableList::builder,
          ImmutableList.Builder::add,
          ImmutableList.Builder::combine,
          ImmutableList.Builder::build);

[wendigo]

Nicer != more performant :P

[losipiuk]

Meh - I doubt it matters at all, so I would keep nicer. I will not fight though.

[martint]

Have we observed a measurable performance improvement? If not, then readability is more important.

[losipiuk]

x