-
Couldn't load subscription status.
- Fork 3.4k
Use faster alternatives to inline patterns #26928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR replaces inline regular expression usage with precompiled or more efficient alternatives: it introduces a Guava CharMatcher for identifier sanitization and a precompiled Pattern for metadata path stripping, improving performance and code clarity. Class diagram for updated BytecodeUtils identifier sanitizationclassDiagram
class BytecodeUtils {
<<final>>
+static CharMatcher ALLOWED_IDENTIFIER_CHARS
+static String sanitizeName(String name)
}
class CharMatcher {
}
BytecodeUtils --> CharMatcher : uses
Class diagram for updated TrinoGlueCatalog metadata path handlingclassDiagram
class TrinoGlueCatalog {
+static Pattern METADATA_PATTERN
+void dropCorruptedTable(ConnectorSession session, SchemaTableName schemaTableName, Table table)
}
class Pattern {
}
TrinoGlueCatalog --> Pattern : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java:462` </location>
<code_context>
public static String sanitizeName(String name)
{
- return name.replaceAll("[^A-Za-z0-9_$]", "_");
+ return ALLOWED_IDENTIFIER_CHARS.replaceFrom(name, '_');
}
</code_context>
<issue_to_address>
**issue:** replaceFrom may not match the original replaceAll behavior for consecutive invalid characters.
The previous code collapsed sequences of invalid characters into a single underscore, but the new approach may produce multiple consecutive underscores. To match the original behavior, use CharMatcher.negate().collapseFrom(name, '_').
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java
Outdated
Show resolved
Hide resolved
bece66c to
dd5b884
Compare
Description
Additional context and related issues
Release notes
(x) 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:
Summary by Sourcery
Replace runtime regex operations with faster alternatives: use Guava CharMatcher for identifier sanitization and a precompiled Pattern for metadata path removal in the Iceberg Glue catalog.
Enhancements: