Clone promql proper prefix #5
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the PromQL query translation within ESQL by introducing more granular pattern fragment types for prefixes and suffixes. Previously, patterns like Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/refacto-visz |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements support for PromQL's proper prefix (.+) and proper suffix (.+-) regex patterns by translating them into STARTS_WITH/ENDS_WITH combined with a NOT_EQUALS check. The changes are logical, and new tests have been added to verify the behavior. My review includes a minor suggestion to reorder conditions in the generated AND expressions for a potential performance optimization.
| return switch (fragment.type()) { | ||
| case EXACT -> new Equals(source, field, value); | ||
| case PREFIX -> new StartsWith(source, field, value); | ||
| case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value)); |
There was a problem hiding this comment.
For a potential performance improvement, it's generally better to place the more selective filter first in an AND condition. StartsWith is likely more selective than NotEquals, which could allow the query engine to short-circuit more effectively.
| case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value)); | |
| case PROPER_PREFIX -> new And(source, new StartsWith(source, field, value), new NotEquals(source, field, value)); |
| case PREFIX -> new StartsWith(source, field, value); | ||
| case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value)); | ||
| case SUFFIX -> new EndsWith(source, field, value); | ||
| case PROPER_SUFFIX -> new And(source, new NotEquals(source, field, value), new EndsWith(source, field, value)); |
There was a problem hiding this comment.
For a potential performance improvement, it's generally better to place the more selective filter first in an AND condition. EndsWith is likely more selective than NotEquals, which could allow the query engine to short-circuit more effectively.
| case PROPER_SUFFIX -> new And(source, new NotEquals(source, field, value), new EndsWith(source, field, value)); | |
| case PROPER_SUFFIX -> new And(source, new EndsWith(source, field, value), new NotEquals(source, field, value)); |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value)); | ||
| case SUFFIX -> new EndsWith(source, field, value); | ||
| case PROPER_SUFFIX -> new And(source, new NotEquals(source, field, value), new EndsWith(source, field, value)); |
There was a problem hiding this comment.
Suggestion: Use the existing Predicates.combineAnd helper to compose the proper-prefix/proper-suffix condition instead of instantiating the And node directly, keeping combination logic consistent and avoiding a direct dependency on the And implementation. [best practice]
| case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value)); | |
| case SUFFIX -> new EndsWith(source, field, value); | |
| case PROPER_SUFFIX -> new And(source, new NotEquals(source, field, value), new EndsWith(source, field, value)); | |
| case PROPER_PREFIX -> Predicates.combineAnd(List.of(new NotEquals(source, field, value), new StartsWith(source, field, value))); | |
| case PROPER_SUFFIX -> Predicates.combineAnd(List.of(new NotEquals(source, field, value), new EndsWith(source, field, value))); |
Why Change? ⭐
The improved code replaces direct instantiation of the And node with the existing helper Predicates.combineAnd, which is already used elsewhere in the file (e.g., combineAnd(selectorConditions)). Predicates.combineAnd accepts a List, and List.of(...) is valid in the project's Java target (Java 9+ / 11). All referenced classes (NotEquals, StartsWith, EndsWith, Predicates) are present in the current file imports/context. The switch expression still returns an Expression, and Predicates.combineAnd returns an Expression as well, so the types match and the switch remains valid.
Assumptions:
- Predicates.combineAnd(List) exists and returns an Expression (consistent with other usages in this file).
- The project uses a Java version that supports List.of.
- The NotEquals import (already present in the PR) is available.
This change is purely a refactor to use the helper for predicate composition, does not alter semantics, and avoids a direct dependency on the And implementation. No new runtime errors or compilation issues are introduced given the stated assumptions.
|
CodeAnt AI finished reviewing your PR. |
Code Review: PromQL Pattern Matching Logic👍 Well Done
📁 Selected files for review (4)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| if (isLiteral(suffix)) { | ||
| return new PatternFragment(PatternFragment.Type.SUFFIX, suffix); | ||
| return new PatternFragment( | ||
| trimmed.startsWith(".*") ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX, |
There was a problem hiding this comment.
Incorrect Suffix Pattern Detection Logic
Logic error in suffix pattern detection: condition checks startsWith(".*") but should validate if the suffix equals the entire literal after removing the prefix wildcard. This causes incorrect classification where all suffix patterns are misidentified, resulting in wrong query optimization behavior.
suffix.equals(trimmed.substring(2)) ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX,
Commitable Suggestion
| trimmed.startsWith(".*") ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX, | |
| suffix.equals(trimmed.substring(2)) ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX, |
Standards
- Algorithm-Correctness-Boolean-Logic
- Business-Rule-Pattern-Classification
| if (isLiteral(prefix)) { | ||
| return new PatternFragment(PatternFragment.Type.PREFIX, prefix); | ||
| return new PatternFragment( | ||
| trimmed.endsWith(".*") ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX, |
There was a problem hiding this comment.
Incorrect Prefix Pattern Detection Logic
Logic error in prefix pattern detection: condition checks endsWith(".*") but should validate if the prefix equals the entire literal after removing the suffix wildcard. This causes incorrect classification where all prefix patterns are misidentified, affecting regex optimization logic.
prefix.equals(trimmed.substring(0, trimmed.length() - 2)) ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX,
Commitable Suggestion
| trimmed.endsWith(".*") ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX, | |
| prefix.equals(trimmed.substring(0, trimmed.length() - 2)) ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX, |
Standards
- Algorithm-Correctness-Boolean-Logic
- Business-Rule-Pattern-Classification
| if (isLiteral(suffix)) { | ||
| return new PatternFragment(PatternFragment.Type.SUFFIX, suffix); | ||
| return new PatternFragment( | ||
| trimmed.startsWith(".*") ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX, |
There was a problem hiding this comment.
Redundant Pattern Validation
Conditional check for trimmed.startsWith(".*") is redundant since the containing if condition already validates this pattern. The ternary operator will always evaluate to SUFFIX type, making PROPER_SUFFIX unreachable and creating unnecessary computational overhead.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Algorithmic-Complexity-Conditional-Optimization
| if (isLiteral(prefix)) { | ||
| return new PatternFragment(PatternFragment.Type.PREFIX, prefix); | ||
| return new PatternFragment( | ||
| trimmed.endsWith(".*") ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX, |
There was a problem hiding this comment.
Redundant Pattern Validation
Conditional check for trimmed.endsWith(".*") is redundant since the containing if condition already validates this pattern. The ternary operator will always evaluate to PREFIX type, making PROPER_PREFIX unreachable and creating unnecessary computational overhead.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Algorithmic-Complexity-Conditional-Optimization
|
PR already reviewed at the latest commit: 5609698. |
2 participants
User description
gradle check?CodeAnt-AI Description
Translate PromQL proper-prefix/suffix label selectors into explicit starts-with + not-equal filters
What Changed
Impact
✅ More accurate label matches for queries like pod=~"host-.+"✅ Fewer full-regex evaluations for common patterns like prefix.* or .*suffix✅ Planner emits explicit starts-with + not-equal filters for proper-prefix/suffix patterns💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.