[ESQL] Support first/last functions. #4
Conversation
- Change `AggregatorImplementor` to use `Argument` instead of `AggregationParameter`. - Compiling esql module resulted in only 2 modified `SpatialExtent*` classes
|
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 introduces a foundational refactoring within the ESQL compute module, specifically targeting the mechanism for implementing aggregation functions. By transitioning from a specialized 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
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (10)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including 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:L
![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 refactors the aggregator implementation to use the Argument interface instead of AggregationParameter, which is a good improvement for code flexibility and extensibility, particularly for adding new functions like first/last. The changes are well-contained and focus on this refactoring. I have found one significant issue in AggregatorImplementer.java where the generated code could be incorrect for functions with multiple BlockArguments, along with a related inefficiency. My review includes a suggestion to fix this.
| for (Argument a : aggParams) { | ||
| String rawBlock = "addRawBlock(" | ||
| + aggParams.stream().map(arg -> arg.blockName()).collect(joining(", ")) | ||
| + (hasMask ? ", mask" : "") | ||
| + ")"; | ||
|
|
||
| if (isBlockArgument == false) { | ||
| a.resolveVectors(builder, rawBlock, "return"); | ||
| } else { | ||
| builder.addStatement(rawBlock); | ||
| } | ||
| } |
There was a problem hiding this comment.
There are a couple of issues in this loop.
- The
rawBlockstring is constructed inside theforloop, which is inefficient as it will be identical in every iteration. It should be defined once before this logic. - More critically, if
isBlockArgumentistrue,builder.addStatement(rawBlock)is executed for every argument inaggParams. This generates theaddRawBlock(...)call multiple times, which is incorrect. It should only be generated once.
The logic should be restructured to address both of these points.
| for (Argument a : aggParams) { | |
| String rawBlock = "addRawBlock(" | |
| + aggParams.stream().map(arg -> arg.blockName()).collect(joining(", ")) | |
| + (hasMask ? ", mask" : "") | |
| + ")"; | |
| if (isBlockArgument == false) { | |
| a.resolveVectors(builder, rawBlock, "return"); | |
| } else { | |
| builder.addStatement(rawBlock); | |
| } | |
| } | |
| String rawBlock = "addRawBlock(" | |
| + aggParams.stream().map(Argument::blockName).collect(joining(", ")) | |
| + (hasMask ? ", mask" : "") | |
| + ")"; | |
| if (isBlockArgument) { | |
| builder.addStatement(rawBlock); | |
| } else { | |
| for (Argument a : aggParams) { | |
| a.resolveVectors(builder, rawBlock, "return"); | |
| } | |
| } |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| builder.addStatement("$T $L = $L.asVector()", vectorType(p.type()), p.vectorName(), p.blockName()); | ||
| builder.beginControlFlow("if ($L == null)", p.vectorName()); | ||
|
|
||
| boolean isBlockArgument = aggParams.getFirst() instanceof BlockArgument; |
There was a problem hiding this comment.
Suggestion: Replace the invalid use of aggParams.getFirst() with a safe indexed access guarded by an emptiness check (use aggParams.get(0) when non-empty). [possible bug]
| boolean isBlockArgument = aggParams.getFirst() instanceof BlockArgument; | |
| boolean isBlockArgument = aggParams.isEmpty() == false && aggParams.get(0) instanceof BlockArgument; |
Why Change? ⭐
The original code called getFirst() on a List, which is not a method on java.util.List and would not compile.
Replacing it with a guarded indexed access (aggParams.isEmpty() == false && aggParams.get(0) instanceof BlockArgument)
uses standard List APIs (isEmpty and get). This prevents IndexOutOfBoundsException by checking emptiness before get(0).
The surrounding code previously iterated over aggParams or used its size, so using a guard here preserves semantics while fixing
the compile error. The change is syntactically valid Java and uses only symbols present in the file (aggParams, BlockArgument).
| private MethodSpec addRawVector(boolean masked) { | ||
| MethodSpec.Builder builder = initAddRaw(true, masked); | ||
| if (aggParams.getFirst().isArray()) { | ||
| if (aggParams.getFirst() instanceof BlockArgument) { |
There was a problem hiding this comment.
Suggestion: Replace the aggParams.getFirst() check with a guarded indexed access (use aggParams.get(0) only when the list is not empty) to avoid compile errors. [possible bug]
| if (aggParams.getFirst() instanceof BlockArgument) { | |
| if (aggParams.isEmpty() == false && aggParams.get(0) instanceof BlockArgument) { |
Why Change? ⭐
This replaces an invalid List.getFirst() invocation with a safe check that first ensures the list is non-empty,
then accesses the first element via get(0). The code uses standard List methods (isEmpty and get), so it will compile.
The guard avoids an IndexOutOfBoundsException in the unlikely case aggParams is empty. All referenced symbols exist
in the file and the change preserves the original intent of checking whether the first argument is a BlockArgument.
|
|
||
| default String scratchName() { | ||
| if (isBytesRef() == false) { | ||
| throw new IllegalStateException("can't build scratch for non-BytesRef"); |
There was a problem hiding this comment.
Suggestion: Include the argument name in the exception message from scratchName() to make failures easier to diagnose. [enhancement]
| throw new IllegalStateException("can't build scratch for non-BytesRef"); | |
| throw new IllegalStateException("can't build scratch for non-BytesRef for parameter: " + name()); |
Why Change? ⭐
The proposed change only enhances the exception message by concatenating the existing name() value. In an interface default method it is valid to call the abstract name() method implemented by concrete classes, so this is syntactically correct and will compile. The change does not alter control flow or state, only the text of the IllegalStateException; it therefore cannot introduce runtime bugs beyond the original exception and actually aids debugging by including the parameter name. Assumes implementations provide a reasonable name() and that calling it in the error path is acceptable.
| */ | ||
| void read(MethodSpec.Builder builder, boolean blockStyle); | ||
|
|
||
| void read(MethodSpec.Builder builder, String accessor, String firstParam); |
There was a problem hiding this comment.
Suggestion: Provide a default implementation for the newly added read(builder, accessor, firstParam) overload to avoid breaking existing implementers of this interface; make it throw an explicit UnsupportedOperationException until concrete implementations are updated. [possible bug]
| void read(MethodSpec.Builder builder, String accessor, String firstParam); | |
| default void read(MethodSpec.Builder builder, String accessor, String firstParam) { | |
| throw new UnsupportedOperationException("read(builder, accessor, firstParam) is not implemented for " + getClass().getName()); | |
| } |
Why Change? ⭐
Converting the newly added abstract overload to a default method that throws UnsupportedOperationException is syntactically valid for a Java interface and will compile. Default methods can reference instance methods such as getClass(), so the diagnostic message will include the concrete implementor's class name. This preserves binary compatibility for existing implementers (they won't be forced to implement the new overload immediately) while clearly signalling unimplemented behavior at runtime. Assumes it's acceptable in this codebase to temporarily provide a throwing default until concrete classes are updated.
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public TypeName type() { | ||
| return null; |
There was a problem hiding this comment.
Suggestion: Implement name() and type() to return concrete, non-null values (e.g., return "position" and TypeName.INT) instead of returning null. [possible bug]
| return null; | |
| } | |
| @Override | |
| public TypeName type() { | |
| return null; | |
| return "position"; | |
| } | |
| @Override | |
| public TypeName type() { | |
| return TypeName.INT; |
Why Change? ⭐
The improved code replaces two methods that previously returned null with concrete, non-null values.
- Syntax: The Java method signatures and return statements are valid Java and match the surrounding file style.
- Dependencies: TypeName is already imported in the file (com.squareup.javapoet.TypeName), so returning TypeName.INT is resolvable.
- Semantics: Returning "position" for name() and TypeName.INT for type() is consistent with the class purpose (position in a block) and with dataType(boolean) which already returns TypeName.INT.
- Safety: Eliminates potential NPEs or downstream null checks caused by returning null; no new exceptions are introduced.
Assumptions: The Argument interface permits non-null name() and type() implementations as used elsewhere. Given the existing imports and surrounding methods in the diff, this change is executable and does not introduce production-breaking behavior.
| if (invokeBlockEval.length == 1) { | ||
| builder.addStatement(invokeBlockEval[0]); | ||
| builder.endControlFlow(); | ||
| } else { | ||
| for (String statement : invokeBlockEval) { | ||
| builder.addStatement(statement); | ||
| } | ||
| builder.endControlFlow(); | ||
| } |
There was a problem hiding this comment.
Suggestion: Guard against a null varargs array for invokeBlockEval to avoid an NPE and consolidate the single endControlFlow call. [possible bug]
| if (invokeBlockEval.length == 1) { | |
| builder.addStatement(invokeBlockEval[0]); | |
| builder.endControlFlow(); | |
| } else { | |
| for (String statement : invokeBlockEval) { | |
| builder.addStatement(statement); | |
| } | |
| builder.endControlFlow(); | |
| } | |
| if (invokeBlockEval == null || invokeBlockEval.length == 0) { | |
| // nothing to invoke; just fall through to end the if block | |
| } else if (invokeBlockEval.length == 1) { | |
| builder.addStatement(invokeBlockEval[0]); | |
| } else { | |
| for (String statement : invokeBlockEval) { | |
| builder.addStatement(statement); | |
| } | |
| } | |
| builder.endControlFlow(); |
Why Change? ⭐
The improved code is valid Java and uses only symbols visible in the surrounding PR diff (vectorType, type, name, and MethodSpec.Builder). It guards against invokeBlockEval being null (defensive but correct: varargs can be null if explicitly passed) and against an empty array, avoiding a potential NullPointerException from checking length. The control-flow balance is preserved: beginControlFlow(...) is matched by a single endControlFlow() unconditionally after handling the possible branches, which is equivalent to the previous behavior but avoids duplicate endControlFlow calls and ensures the method remains syntactically and logically correct. The builder.addStatement calls are unchanged in semantics. No new types or methods are introduced, so this change is executable and safe in production.
Pull Request Feedback 🔍
|
|
CodeAnt AI finished reviewing your PR. |
|
/refacto-visz |
|
Your subscription expired on 10/15/2025. To continue using Refacto code reviews, please renew or upgrade your subscription. |
|
/refacto-visz |
1 similar comment
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Aggregation Parameter Processing👍 Well Done
📁 Selected files for review (10)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| @Override | ||
| public String name() { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public TypeName type() { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Null Pointer Risk
Methods name() and type() explicitly return null which can cause NullPointerException when accessed by code expecting valid values. Runtime failures occur when null values propagate through argument processing chains.
@Override
public String name() {
throw new UnsupportedOperationException("PositionArgument does not have a name");
}
@Override
public TypeName type() {
throw new UnsupportedOperationException("PositionArgument does not have a type");
}
Commitable Suggestion
| @Override | |
| public String name() { | |
| return null; | |
| } | |
| @Override | |
| public TypeName type() { | |
| return null; | |
| } | |
| @Override | |
| public String name() { | |
| throw new UnsupportedOperationException("PositionArgument does not have a name"); | |
| } | |
| @Override | |
| public TypeName type() { | |
| throw new UnsupportedOperationException("PositionArgument does not have a type"); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- DbC-Postcondition-Violation
| boolean isBlockArgument = aggParams.getFirst() instanceof BlockArgument; |
There was a problem hiding this comment.
Array Bounds Risk
Direct getFirst() call without size validation can throw NoSuchElementException if aggParams list is empty. Runtime exception causes aggregation function generation failure preventing system operation.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- DbC-Precondition-Violation
| for (Argument a : aggParams) { | ||
| String rawBlock = "addRawBlock(" | ||
| + aggParams.stream().map(arg -> arg.blockName()).collect(joining(", ")) | ||
| + (hasMask ? ", mask" : "") | ||
| + ")"; | ||
|
|
||
| if (isBlockArgument == false) { | ||
| a.resolveVectors(builder, rawBlock, "return"); | ||
| } else { | ||
| builder.addStatement(rawBlock); | ||
| } |
There was a problem hiding this comment.
Redundant Block Processing
String concatenation for rawBlock statement occurs inside loop for each argument, creating identical strings repeatedly. This creates unnecessary string allocation overhead proportional to argument count.
Standards
- ISO-IEC-25010-Performance-Efficiency-Resource-Utilization
- Optimization-Pattern-Loop-Invariant-Code-Motion
| boolean isBlockArgument = aggParams.getFirst() instanceof BlockArgument; | ||
|
|
||
| for (Argument a : aggParams) { | ||
| String rawBlock = "addRawBlock(" | ||
| + aggParams.stream().map(arg -> arg.blockName()).collect(joining(", ")) | ||
| + (hasMask ? ", mask" : "") | ||
| + ")"; | ||
|
|
||
| if (isBlockArgument == false) { | ||
| a.resolveVectors(builder, rawBlock, "return"); | ||
| } else { | ||
| builder.addStatement(rawBlock); | ||
| } |
There was a problem hiding this comment.
Block Processing Logic
Loop determines block processing mode based only on first parameter but applies same rawBlock statement construction inside loop for each argument. Logic creates redundant addRawBlock statements when isBlockArgument is true since same statement executes multiple times within the loop.
Standards
- Algorithm-Correctness-Loop-Logic
- Logic-Verification-Control-Flow
2 participants
User description
AggregatorImplementorto useArgumentinstead ofAggregationParameter.SpatialExtent*classesCodeAnt-AI Description
Handle multi-valued aggregation inputs as blocks and enable first/last aggregations
What Changed
Impact
✅ Fewer aggregation errors on multi-valued fields✅ Correct first/last aggregation results on multi-valued inputs✅ More reliable spatial aggregation for multi-valued doc values💡 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.