Repository navigation
Repeatable - #45
Repeatable#45
Conversation
…kip setting added to skip already processed files
… that are being changed
|
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 · |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughAdds resumable checkpointing and CSV-based processed-repo tracking, a method-call index and batched signature updates, preserves original WHERE clause text extraction, updates Gemini AI request/response keys and dependencies, and adds multiple unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "Invoker"
participant Checker as "QueryOptimizationChecker"
participant Checkpoint as "CheckpointManager (file)"
participant Tracker as "QueryOptimizationTracker (CSV)"
participant Optimizer as "QueryOptimizer"
participant Fields as "Fields (method-call index)"
CLI->>Checker: start analyze()
Checker->>Checkpoint: load()
Checkpoint-->>Checker: checkpoint state or empty
Checker->>Tracker: shouldSkip(repo)?
Tracker-->>Checker: skip decision
alt not skipped
Checker->>Optimizer: analyze repository
Optimizer->>Fields: buildMethodCallIndex()
Fields-->>Optimizer: method-call index
Optimizer->>Optimizer: compute batched renames
Optimizer->>Checkpoint: update modifiedFiles / suggestions
end
Checker->>Checkpoint: save()
Checker-->>CLI: return analyzed count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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:XXL
There was a problem hiding this comment.
Pull request overview
Adds repeatability/resume support and performance improvements to the query optimization workflow by checkpointing progress, batching method-call signature updates, and improving SQL/AST handling.
Changes:
- Introduces checkpoint/resume support (including CLI “fresh start”) and persists modified files/index suggestions across runs.
- Batches repository method rename propagation using a method-call index to avoid scanning all dependent classes.
- Improves WHERE/JOIN extraction (incl. preserving original WHERE text) and adds new unit tests.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java | Adds checkpointing + target_class filtering; changes analyze() to return count; refactors redundancy cleanup. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java | Batches method rename propagation; adds argument reordering cache; adds “fresh” flag; adds profiling/logging. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java | Builds method call index to support batched updates. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java | New checkpoint persistence layer for resume capability. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationTracker.java | New tracker for “skip processed” behavior based on CSV. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/WhereClauseCollector.java | Attempts to handle set operations/parenthesized selects when collecting where/join info. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationExtractor.java | Adds extraction of original WHERE clause text from parsed statements. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryAnalysisEngine.java | Stores original WHERE clause text onto QueryAnalysisResult. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryAnalysisResult.java | Prefers original WHERE clause text when present. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java | Hardens optimized-query parsing with fallbacks. |
| src/main/java/sa/com/cloudsolutions/antikythera/examples/OptimizationStatsLogger.java | Removes CSV-based processed repository lookup. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManagerTest.java | Adds unit tests for checkpointing behavior. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationTrackerTest.java | Adds unit tests for processed-repo lookup behavior. |
| src/test/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizerArgumentReorderTest.java | Adds unit tests for argument reordering on method rename. |
| pom.xml | Adds property for antikythera version; updates hql-parser; changes Mockito scope. |
| docs/query-optimization-review-prompt.md | Tweaks documentation text for the review prompt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public <S> Void visit(Select select, S context) { | ||
| // Process PlainSelect statements | ||
| if (select.getPlainSelect() != null) { | ||
| processPlainSelect(select.getPlainSelect()); | ||
| // Handle different select types safely | ||
| if (select instanceof PlainSelect plainSelect) { | ||
| processPlainSelect(plainSelect); | ||
| } else if (select instanceof SetOperationList setOpList) { | ||
| processSetOperationList(setOpList); | ||
| } else if (select instanceof ParenthesedSelect parenthesedSelect) { | ||
| processFromItem(parenthesedSelect); | ||
| } else { | ||
| // Fallback: try getPlainSelect() for simple cases, but catch ClassCastException | ||
| try { | ||
| PlainSelect plainSelect = select.getPlainSelect(); | ||
| if (plainSelect != null) { | ||
| processPlainSelect(plainSelect); | ||
| } | ||
| } catch (ClassCastException e) { | ||
| // This can happen with SetOperationList - already handled above | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The instanceof PlainSelect/SetOperationList/ParenthesedSelect checks on Select are inconvertible in JSqlParser (those types are typically SelectBody/subcomponents, not Select), which will fail compilation. Switch this logic to inspect the select body (e.g., select.getSelectBody() / equivalent for your JSqlParser version) and branch on PlainSelect / SetOperationList / ParenthesedSelect there.
| if (statement instanceof Select select) { | ||
| if (select instanceof PlainSelect plainSelect) { | ||
| whereExpr = plainSelect.getWhere(); | ||
| } else { | ||
| try { | ||
| PlainSelect plainSelect = select.getPlainSelect(); | ||
| if (plainSelect != null) { | ||
| whereExpr = plainSelect.getWhere(); | ||
| } | ||
| } catch (ClassCastException e) { | ||
| // SetOperationList or other complex select - can't extract simple WHERE | ||
| return ""; | ||
| } | ||
| } | ||
| } else if (statement instanceof Update update) { | ||
| whereExpr = update.getWhere(); | ||
| } else if (statement instanceof Delete delete) { | ||
| whereExpr = delete.getWhere(); | ||
| } |
There was a problem hiding this comment.
Same inconvertible type issue as in WhereClauseCollector: Select is not a PlainSelect, so select instanceof PlainSelect won’t compile under typical JSqlParser APIs. Extract the select body first, then read the WHERE from PlainSelect (and consider SetOperationList by iterating sub-select bodies if you want WHERE text support for unions).
| String cacheKey = oldMethod.getNameAsString() + "->" + newMethod.getNameAsString(); | ||
| Map<Integer, Integer> map = parameterMappingCache.computeIfAbsent(cacheKey, k -> { | ||
| Map<Integer, Integer> newMap = new HashMap<>(); | ||
| // Build parameter mapping: newIndex -> oldIndex | ||
| for (int i = 0; i < oldMethod.getParameters().size(); i++) { | ||
| String oldParam = oldMethod.getParameter(i).getNameAsString() + ":" + | ||
| oldMethod.getParameter(i).getType().asString(); | ||
|
|
||
| for (int j = 0; j < newMethod.getParameters().size(); j++) { | ||
| String newParam = newMethod.getParameter(j).getNameAsString() + ":" + | ||
| newMethod.getParameter(j).getType().asString(); | ||
|
|
||
| if (oldParam.equals(newParam)) { | ||
| newMap.put(j, i); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return newMap; | ||
| }); | ||
|
|
||
|
|
||
| // Reorder arguments in-place using array to avoid expensive NodeList operations | ||
| Expression[] argsArray = new Expression[args.size()]; | ||
| for (int i = 0; i < args.size(); i++) { | ||
| argsArray[i] = args.get(i); | ||
| } | ||
|
|
||
| args.clear(); | ||
|
|
||
| for (int i = 0; i < argsArray.length; i++) { | ||
| Integer oldIdx = map.get(i); | ||
| if (oldIdx != null && oldIdx < argsArray.length) { | ||
| args.add(argsArray[oldIdx]); | ||
| } else { | ||
| // Fallback if mapping is missing or invalid | ||
| for (int j = 0; j < argsArray.length; j++) { | ||
| if (j < args.size()) continue; | ||
| args.add(argsArray[j]); | ||
| } | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Two correctness issues here: (1) cacheKey collides for overloaded methods (same name, different params), so a mapping built for one overload can be reused for another and reorder arguments incorrectly; include old/new signatures (name + param types) in the key. (2) The fallback logic can duplicate/misorder args because it appends based on j < args.size() rather than “which indices were already used”; if mapping is incomplete/invalid, the safest fallback is to restore the original argument order (e.g., re-add argsArray as-is) and return.
| throw new IllegalStateException( | ||
| "Class " + className + " found in method call index but not in resolved types. " + | ||
| "This may indicate incomplete preprocessing or an external dependency that should be excluded."); |
There was a problem hiding this comment.
Throwing here turns a previously-skipped condition (null resolved type) into a hard crash, which can break runs when the index includes external/non-resolved callers. Prefer logging a warning and skipping this class (or filtering such classes out when building the index) to keep processing robust.
| throw new IllegalStateException( | |
| "Class " + className + " found in method call index but not in resolved types. " + | |
| "This may indicate incomplete preprocessing or an external dependency that should be excluded."); | |
| logger.warn("Skipping class {}: found in method call index but not in resolved types. " | |
| + "This may indicate incomplete preprocessing or an external dependency that should be excluded.", | |
| className); | |
| continue; |
| } catch (AntikytheraException ae) { | ||
| logger.error("Error analyzing repository {}: {}", fullyQualifiedName, ae.getMessage()); | ||
| // Save checkpoint even on error so we don't lose progress | ||
| checkpointManager.setIndexSuggestions(suggestedNewIndexes, suggestedMultiColumnIndexes); | ||
| checkpointManager.save(); | ||
| } |
There was a problem hiding this comment.
The checkpoint is cleared unconditionally at the end, even if errors occurred (caught AntikytheraException) or the run was partial (e.g., via target_class). This defeats resume behavior. Track whether any repository failed (or whether the run completed the intended scope) and only call checkpointManager.clear() when the run completes successfully for that scope.
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Set; |
There was a problem hiding this comment.
This test file won’t compile as-is: Java treats unused imports as compilation errors (e.g., NodeList, Expression, ArrayList, List, Set aren’t used in the file). Also, using new NameExpr("\"valA\"") creates an invalid identifier expression for a string literal; use the proper literal expression nodes (e.g., StringLiteralExpr, IntegerLiteralExpr) so the AST is syntactically correct and matches production behavior.
| call.addArgument(new NameExpr("\"valA\"")); | ||
| call.addArgument(new NameExpr("123")); |
There was a problem hiding this comment.
This test file won’t compile as-is: Java treats unused imports as compilation errors (e.g., NodeList, Expression, ArrayList, List, Set aren’t used in the file). Also, using new NameExpr("\"valA\"") creates an invalid identifier expression for a string literal; use the proper literal expression nodes (e.g., StringLiteralExpr, IntegerLiteralExpr) so the AST is syntactically correct and matches production behavior.
| } | ||
|
|
||
| long astStart = System.currentTimeMillis(); | ||
| logger.info(" Visiting class: {} (fields: {})", className, fieldNames); |
There was a problem hiding this comment.
Logging each visited class at info (including fieldNames) will be very noisy on large codebases and can materially slow the run. Consider downgrading these per-class logs to debug (and/or guarding with logger.isDebugEnabled()), keeping info for batch summaries and periodic progress.
| logger.info(" Visiting class: {} (fields: {})", className, fieldNames); | |
| if (logger.isDebugEnabled()) { | |
| logger.debug(" Visiting class: {} (fields: {})", className, fieldNames); | |
| } |
| /** | ||
| * Processes a SetOperationList (UNION, UNION ALL, INTERSECT, EXCEPT). | ||
| * Recursively processes each select in the set operation. | ||
| */ | ||
| private void processSetOperationList(SetOperationList setOpList) { | ||
| if (setOpList.getSelects() != null) { | ||
| for (Select select : setOpList.getSelects()) { | ||
| if (select instanceof PlainSelect plainSelect) { | ||
| processPlainSelect(plainSelect); | ||
| } else if (select instanceof ParenthesedSelect parenthesedSelect) { | ||
| processFromItem(parenthesedSelect); | ||
| } else if (select instanceof SetOperationList nestedSetOp) { | ||
| // Handle nested set operations | ||
| processSetOperationList(nestedSetOp); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new set-operation handling is non-trivial and currently untested in this PR. Add unit tests covering UNION/UNION ALL (and nested parenthesized selects) to ensure WHERE/JOIN conditions are collected correctly and to prevent regressions around SetOperationList traversal.
|
|
||
| ## 🚀 AI Review Prompt | ||
| Carry out a review of the diff between the current branch and the main branch, focusing on the optimizations made by the `QueryOptimizer` tool. | ||
| Carry out a review of the diff of the current, focusing on the optimizations made by the `QueryOptimizer` tool. |
There was a problem hiding this comment.
The sentence is grammatically incomplete (“diff of the current”); clarify what “current” refers to (e.g., “current branch” / “current PR”) and what it’s diffed against (e.g., “main”) so the prompt is unambiguous.
| Carry out a review of the diff of the current, focusing on the optimizations made by the `QueryOptimizer` tool. | |
| Carry out a review of the current Git diff, focusing on the optimizations made by the `QueryOptimizer` tool. |
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| ClassOrInterfaceDeclaration cdecl = cloneClassSignature( | ||
| originalCompilationUnit.findFirst(ClassOrInterfaceDeclaration.class).orElseThrow()); |
There was a problem hiding this comment.
Suggestion: The class/interface that is cloned to host the optimized method is obtained via originalCompilationUnit.findFirst(ClassOrInterfaceDeclaration.class), which will return the first class/interface in the file, not necessarily the one that actually contains the original repository method; in files with multiple top-level or nested types this can cause the optimized method to be attached to the wrong type, leading BaseRepositoryParser to build a query for the wrong repository or fail to build one, so the extracted column order will not correspond to the original method. [logic error]
Severity Level: Major ⚠️
- ❌ LLM-based index suggestions can analyze the wrong repository query.
- ⚠️ Column-order extraction may ignore AI-optimized method signature.
- ⚠️ Quiet-mode reports can show mismatched original/optimized queries.
- ⚠️ CLI query optimizer may emit misleading optimization guidance.| ClassOrInterfaceDeclaration cdecl = cloneClassSignature( | |
| originalCompilationUnit.findFirst(ClassOrInterfaceDeclaration.class).orElseThrow()); | |
| ClassOrInterfaceDeclaration originalClass = originalQuery.getMethodDeclaration() | |
| .getCallableDeclaration() | |
| .findAncestor(ClassOrInterfaceDeclaration.class) | |
| .orElseThrow(); | |
| ClassOrInterfaceDeclaration cdecl = cloneClassSignature(originalClass); |
Steps of Reproduction ✅
1. Run the query-optimization CLI entry point `QueryOptimizationChecker.main` in
`src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java:1322-1351`,
pointing Antikythera at a codebase where at least one JPA repository interface is *not*
the first `ClassOrInterfaceDeclaration` in its `.java` file (for example, another
top-level class or interface is declared before the repository).
2. Inside `QueryOptimizationChecker.analyze()` (`QueryOptimizationChecker.java:103-183`),
the tool discovers this repository via `AntikytheraRunTime.getResolvedTypes()` and, in
`analyzeRepository` (`QueryOptimizationChecker.java:194-222`), compiles the repository
source with `RepositoryParser`, calls `processTypes()` and `buildQueries()`, and obtains a
`RepositoryQuery` for a method that the LLM will be asked to optimize.
3. `sendRawQueriesToLLM` (`QueryOptimizationChecker.java:228-257`) builds a `QueryBatch`
containing that `RepositoryQuery` via `createQueryBatch`
(`QueryOptimizationChecker.java:266-281`) and passes it to
`GeminiAIService.analyzeQueryBatch` (`GeminiAIService.java:93-97`), which sends the batch
to Gemini and then calls `parseResponse` (`GeminiAIService.java:352-368`) and
`parseRecommendations` (`GeminiAIService.java:374-404`).
4. In `parseRecommendations`, for each `RepositoryQuery` / AI recommendation pair,
`parseOptimizationRecommendation` (`GeminiAIService.java:451-495`) is invoked. When the AI
`notes` indicate an optimization (no "N/A"/"unchanged"/"already optimized"), it calls
`extractRecommendedColumnOrder(optimizedCodeElement, originalQuery)` at
`GeminiAIService.java:469-472`.
5. Inside `extractRecommendedColumnOrder` (`GeminiAIService.java:535-572`), the code
obtains `originalCompilationUnit` from
`originalQuery.getMethodDeclaration().getCallableDeclaration().findCompilationUnit().orElseThrow()`
(`GeminiAIService.java:538-539`), but then selects the host type using
`originalCompilationUnit.findFirst(ClassOrInterfaceDeclaration.class).orElseThrow()`
(`GeminiAIService.java:541-542`). Because this returns the *first* class/interface in the
file, not necessarily the one that actually declares the repository method, the optimized
method parsed from `optimizedCodeElement` is attached to the wrong type.
`BaseRepositoryParser.create(cu)` / `processTypes()` / `buildQueries()`
(`GeminiAIService.java:554-556`) then either (a) produce queries for this wrong type,
causing `queries.stream().findFirst()` (`GeminiAIService.java:566`) to return a
`RepositoryQuery` unrelated to the original method, or (b) produce no queries, triggering
the empty-queries fallback at `GeminiAIService.java:559-563`. In case (a),
`optimizedResult.optimizedQuery()` refers to the wrong query and is later analyzed by
`QueryOptimizationChecker.createResultWithIndexAnalysis`
(`QueryOptimizationChecker.java:335-374`), leading to mismatched column-order and index
recommendations for the original method; in case (b), the tool silently falls back to the
original column order and ignores the AI's optimized signature.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java
**Line:** 541:542
**Comment:**
*Logic Error: The class/interface that is cloned to host the optimized method is obtained via `originalCompilationUnit.findFirst(ClassOrInterfaceDeclaration.class)`, which will return the first class/interface in the file, not necessarily the one that actually contains the original repository method; in files with multiple top-level or nested types this can cause the optimized method to be attached to the wrong type, leading BaseRepositoryParser to build a query for the wrong repository or fail to build one, so the extracted column order will not correspond to the original method.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| whereExpr = plainSelect.getWhere(); | ||
| } else { | ||
| try { | ||
| PlainSelect plainSelect = select.getPlainSelect(); | ||
| if (plainSelect != null) { | ||
| whereExpr = plainSelect.getWhere(); | ||
| } | ||
| } catch (ClassCastException e) { | ||
| // SetOperationList or other complex select - can't extract simple WHERE |
There was a problem hiding this comment.
Suggestion: The WHERE-clause text extractor only handles simple PlainSelect statements and falls back to select.getPlainSelect(), which fails for UNION or other SetOperationList/ParenthesedSelect queries, causing it to return an empty string even when a WHERE clause exists; this breaks the documented behavior of returning the original WHERE clause and leads to missing filter text in analysis results for such queries. [logic error]
Severity Level: Major ⚠️
- ⚠️ originalWhereClauseText empty for UNION or set-operation queries.
- ⚠️ Query optimization analysis misrepresents filters on UNION queries.| whereExpr = plainSelect.getWhere(); | |
| } else { | |
| try { | |
| PlainSelect plainSelect = select.getPlainSelect(); | |
| if (plainSelect != null) { | |
| whereExpr = plainSelect.getWhere(); | |
| } | |
| } catch (ClassCastException e) { | |
| // SetOperationList or other complex select - can't extract simple WHERE | |
| // Simple SELECT with a PlainSelect body | |
| whereExpr = plainSelect.getWhere(); | |
| } else if (select instanceof net.sf.jsqlparser.statement.select.SetOperationList setOpList) { | |
| // UNION/INTERSECT/EXCEPT style SELECT; try to find a WHERE in one of the component selects | |
| for (Select innerSelect : setOpList.getSelects()) { | |
| if (innerSelect instanceof PlainSelect innerPlain) { | |
| whereExpr = innerPlain.getWhere(); | |
| if (whereExpr != null) { | |
| break; | |
| } | |
| } else if (innerSelect instanceof net.sf.jsqlparser.statement.select.ParenthesedSelect parenthesed) { | |
| Select nestedSelect = parenthesed.getSelect(); | |
| if (nestedSelect instanceof PlainSelect nestedPlain) { | |
| whereExpr = nestedPlain.getWhere(); | |
| if (whereExpr != null) { | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| } else if (select instanceof net.sf.jsqlparser.statement.select.ParenthesedSelect parenthesedSelect) { | |
| // Parenthesized SELECT; unwrap and inspect the inner SELECT | |
| Select innerSelect = parenthesedSelect.getSelect(); | |
| if (innerSelect instanceof PlainSelect innerPlain) { | |
| whereExpr = innerPlain.getWhere(); | |
| } else if (innerSelect instanceof net.sf.jsqlparser.statement.select.SetOperationList innerSetOp) { | |
| for (Select nestedSelect : innerSetOp.getSelects()) { | |
| if (nestedSelect instanceof PlainSelect nestedPlain) { | |
| whereExpr = nestedPlain.getWhere(); | |
| if (whereExpr != null) { | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| } else { | |
| // Fallback: try getPlainSelect() for simple cases, but guard against complex select types | |
| try { | |
| PlainSelect plainSelect = select.getPlainSelect(); | |
| if (plainSelect != null) { | |
| whereExpr = plainSelect.getWhere(); | |
| } | |
| } catch (ClassCastException e) { | |
| // Complex select type where we can't easily extract a single WHERE clause |
Steps of Reproduction ✅
1. Create a RepositoryQuery whose ConversionResult.getNativeSql() is a UNION query with
WHERE clauses, for example `"SELECT * FROM t1 WHERE status = ? UNION SELECT * FROM t2
WHERE status = ?"`, following the pattern used in `QueryOptimizationExtractorTest`
(`src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationExtractorTest.java:37-60`)
where SQL strings are parsed with `CCJSqlParserUtil.parse(...)` into a `Statement`.
2. In a test or application code, parse that UNION SQL using
`CCJSqlParserUtil.parse(nativeSql)` and set the resulting `Statement` into a
`RepositoryQuery`'s ConversionResult so that `QueryAnalysisEngine.analyzeQuery(...)` will
take the native SQL path (see `QueryAnalysisEngine.java:36-45`, where
`conversionResult.getNativeSql()` is parsed and assigned to `statementToAnalyze`).
3. Call `new QueryAnalysisEngine().analyzeQuery(repositoryQuery)`; inside `analyzeQuery`,
after extracting WHERE and JOIN conditions, it invokes
`QueryOptimizationExtractor.extractWhereClauseText(statementToAnalyze)` to obtain the
original WHERE text for reporting (`QueryAnalysisEngine.java:59-64`).
4. When `extractWhereClauseText` executes for this UNION statement
(`QueryOptimizationExtractor.java:89-116`), the `statement` is a `Select` whose body is
represented as a `SetOperationList`; the current implementation falls into the `else`
branch, `select.getPlainSelect()` throws `ClassCastException`, the catch block returns
`""` immediately (`QueryOptimizationExtractor.java:96-103`), and as a result
`originalWhereClauseText` passed into `QueryAnalysisResult`
(`QueryAnalysisEngine.java:62-64`) is an empty string even though the UNION SQL contains
valid WHERE clauses.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationExtractor.java
**Line:** 94:102
**Comment:**
*Logic Error: The WHERE-clause text extractor only handles simple PlainSelect statements and falls back to select.getPlainSelect(), which fails for UNION or other SetOperationList/ParenthesedSelect queries, causing it to return an empty string even when a WHERE clause exists; this breaks the documented behavior of returning the original WHERE clause and leads to missing filter text in analysis results for such queries.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| // Use method names as cache key - they're unique enough and much faster than getSignature() | ||
| String cacheKey = oldMethod.getNameAsString() + "->" + newMethod.getNameAsString(); | ||
| Map<Integer, Integer> map = parameterMappingCache.computeIfAbsent(cacheKey, k -> { |
There was a problem hiding this comment.
Suggestion: The parameter mapping cache in reorderMethodArguments uses only the old and new method names as the cache key, so overloaded methods (or different signatures across classes) that share the same old→new names will incorrectly share a single mapping; this can cause arguments to be reordered according to the wrong signature and lead to semantically incorrect calls when overloads exist. [possible bug]
Severity Level: Major ⚠️
- ❌ Wrong argument order for overloaded repository methods when triggered.
- ⚠️ Affects method-call rewriting in QueryOptimizer's analyze() execution.
- ⚠️ Currently latent; requires specific overloaded-method rename scenarios.| // Use method names as cache key - they're unique enough and much faster than getSignature() | |
| String cacheKey = oldMethod.getNameAsString() + "->" + newMethod.getNameAsString(); | |
| Map<Integer, Integer> map = parameterMappingCache.computeIfAbsent(cacheKey, k -> { | |
| // Use full method signatures as cache key to avoid collisions between overloaded methods | |
| String cacheKey = oldMethod.getSignature().asString() + "->" + newMethod.getSignature().asString(); |
Steps of Reproduction ✅
1. Run the optimizer on a repository where two overloaded methods share the same original
and optimized names (e.g., `findByAAndB(String,int)` and `findByAAndB(int,String)` both
being renamed), producing two distinct `OptimizationIssue` instances that flow into
`QueryOptimizer.analyzeRepository(...)` at `QueryOptimizer.java:76-133`.
2. For each optimization issue, `NameChangeVisitor.visit(MethodCallExpr,
QueryAnalysisResult)` at lines 735-773 (or
`BatchedNameChangeProcessor.processMethodCall(...)` at lines 895-931) is invoked while
rewriting dependent classes, and for each matching call it calls
`reorderMethodArguments(mce, issue)` at lines 765 and 926 respectively.
3. On the first such issue, `reorderMethodArguments(...)` at lines 666-721 builds a
parameter mapping from `oldMethod` to `newMethod` and stores it in the shared
`parameterMappingCache` under a key composed only of `oldMethod.getNameAsString() + "->" +
newMethod.getNameAsString()` at lines 675-680.
4. When the second overloaded method with the same old→new name pair is processed,
`reorderMethodArguments(...)` reuses the cached mapping (because the name-based key
collides) even though `oldMethod` and `newMethod` now have a different signature, causing
arguments of this overload to be reordered according to the first overload's mapping; this
leads to semantically incorrect argument order and potentially wrong query behavior. The
current codebase does not include a test for this case, but a concrete reproduction can be
created by extending `QueryOptimizerArgumentReorderTest` at
`src/test/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizerArgumentReorderTest.java:81-121`
with two different mocked `OptimizationIssue` instances sharing the same method names but
different parameter signatures, and asserting that the second call's arguments are
misordered after invoking `reorderMethodArguments`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java
**Line:** 678:680
**Comment:**
*Possible Bug: The parameter mapping cache in `reorderMethodArguments` uses only the old and new method names as the cache key, so overloaded methods (or different signatures across classes) that share the same old→new names will incorrectly share a single mapping; this can cause arguments to be reordered according to the wrong signature and lead to semantically incorrect calls when overloads exist.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| // Handle different select types safely | ||
| if (select instanceof PlainSelect plainSelect) { | ||
| processPlainSelect(plainSelect); | ||
| } else if (select instanceof SetOperationList setOpList) { | ||
| processSetOperationList(setOpList); | ||
| } else if (select instanceof ParenthesedSelect parenthesedSelect) { | ||
| processFromItem(parenthesedSelect); | ||
| } else { | ||
| // Fallback: try getPlainSelect() for simple cases, but catch ClassCastException | ||
| try { | ||
| PlainSelect plainSelect = select.getPlainSelect(); | ||
| if (plainSelect != null) { | ||
| processPlainSelect(plainSelect); | ||
| } | ||
| } catch (ClassCastException e) { | ||
| // This can happen with SetOperationList - already handled above |
There was a problem hiding this comment.
Suggestion: The visit(Select select, S context) method is trying to distinguish between PlainSelect, SetOperationList, and ParenthesedSelect by doing instanceof checks on the Select statement itself, but in JSqlParser these types represent the select body returned by select.getSelectBody(), not the Select statement; as a result, the first three branches never execute and for set-operations like UNION the call to getPlainSelect() throws ClassCastException which is caught and ignored, so WHERE/JOIN conditions for those queries are never collected. To fix this, the dispatch should be based on select.getSelectBody() and only fall back to getPlainSelect() for older/simple cases. [logic error]
Severity Level: Major ⚠️
- ⚠️ UNION queries analyzed via QueryAnalysisEngine yield empty conditions.
- ⚠️ WHERE-based optimization hints missing for UNION-style repository queries.
- ⚠️ JOIN condition extraction skipped for UNION and similar set operations.
- ⚠️ Current test suite defines no UNION SQL, so bug untested.| // Handle different select types safely | |
| if (select instanceof PlainSelect plainSelect) { | |
| processPlainSelect(plainSelect); | |
| } else if (select instanceof SetOperationList setOpList) { | |
| processSetOperationList(setOpList); | |
| } else if (select instanceof ParenthesedSelect parenthesedSelect) { | |
| processFromItem(parenthesedSelect); | |
| } else { | |
| // Fallback: try getPlainSelect() for simple cases, but catch ClassCastException | |
| try { | |
| PlainSelect plainSelect = select.getPlainSelect(); | |
| if (plainSelect != null) { | |
| processPlainSelect(plainSelect); | |
| } | |
| } catch (ClassCastException e) { | |
| // This can happen with SetOperationList - already handled above | |
| // Handle different select types safely, based on the select body | |
| Object selectBody = select.getSelectBody(); | |
| if (selectBody instanceof PlainSelect plainSelect) { | |
| processPlainSelect(plainSelect); | |
| } else if (selectBody instanceof SetOperationList setOpList) { | |
| processSetOperationList(setOpList); | |
| } else if (selectBody instanceof ParenthesedSelect parenthesedSelect) { | |
| processFromItem(parenthesedSelect); | |
| } else { | |
| // Fallback: try getPlainSelect() for simple cases, but catch ClassCastException | |
| try { | |
| PlainSelect plainSelect = select.getPlainSelect(); | |
| if (plainSelect != null) { | |
| processPlainSelect(plainSelect); | |
| } | |
| } catch (ClassCastException e) { | |
| // Ignore unsupported select body types to avoid breaking parsing |
Steps of Reproduction ✅
1. In `QueryAnalysisEngine.analyzeQuery`
(`src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryAnalysisEngine.java:30-65`),
a `RepositoryQuery` is analyzed by first obtaining a JSqlParser `Statement` (either from
`repositoryQuery.getConversionResult().getNativeSql()` via `CCJSqlParserUtil.parse(...)`
or directly from `repositoryQuery.getStatement()`).
2. That `Statement` is passed into
`QueryOptimizationExtractor.extractWhereConditions(statement)` and
`extractJoinConditions(statement)`
(`src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationExtractor.java:35-61`),
which both construct a `WhereClauseCollector` and call `statement.accept(collector,
null)`.
3. For a SQL like `SELECT * FROM orders WHERE status = ? UNION SELECT * FROM
archived_orders WHERE status = ?`, JSqlParser will create a `Statement` whose runtime type
is `Select`, whose select body is a `SetOperationList`. When `statement.accept(collector,
null)` runs, `WhereClauseCollector.visit(Select select, S context)` at
`src/main/java/sa/com/cloudsolutions/antikythera/examples/WhereClauseCollector.java:59-80`
is invoked.
4. Inside `visit(Select select, S context)`, the current code checks `select instanceof
PlainSelect`, `SetOperationList`, and `ParenthesedSelect` (lines 61-67); all of these are
false because `Select` is a wrapper and those types represent the *select body* (returned
by `select.getSelectBody()`), not the `Select` itself. Execution falls into the `else`
block (line 68) and calls `select.getPlainSelect()` (line 71); for a `SetOperationList`
body this throws `ClassCastException`, which is caught and ignored (lines 75-77), so
neither `processPlainSelect(...)` nor `processSetOperationList(...)` is ever invoked and
no WHERE or JOIN conditions are collected for UNION queries. The suggested change to
inspect `select.getSelectBody()` would make the `SetOperationList` branch reachable and
allow UNION queries to be analyzed.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/WhereClauseCollector.java
**Line:** 61:76
**Comment:**
*Logic Error: The `visit(Select select, S context)` method is trying to distinguish between `PlainSelect`, `SetOperationList`, and `ParenthesedSelect` by doing `instanceof` checks on the `Select` statement itself, but in JSqlParser these types represent the select body returned by `select.getSelectBody()`, not the `Select` statement; as a result, the first three branches never execute and for set-operations like `UNION` the call to `getPlainSelect()` throws `ClassCastException` which is caught and ignored, so WHERE/JOIN conditions for those queries are never collected. To fix this, the dispatch should be based on `select.getSelectBody()` and only fall back to `getPlainSelect()` for older/simple cases.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@docs/query-optimization-review-prompt.md`:
- Line 8: The sentence "Carry out a review of the diff of the current, focusing
on the optimizations made by the `QueryOptimizer` tool." is grammatically
incomplete; replace "current" with a clear noun like "branch", "changes", or
"diff" so the prompt reads e.g. "Carry out a review of the diff of the current
branch, focusing on the optimizations made by the `QueryOptimizer` tool." Update
the prompt text in docs/query-optimization-review-prompt.md accordingly, keeping
the referenced `QueryOptimizer` token intact.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java`:
- Around line 94-98: After deserializing the Checkpoint in CheckpointManager
(the objectMapper.readValue call), guard against a null processedRepositories by
initializing checkpoint.processedRepositories to an empty HashSet if it is null;
do the same normalization wherever Checkpoint instances are accepted (so save(),
isProcessed(), markProcessed()) or add a private normalize(Checkpoint) helper
used after readValue—mirror the existing null-guards used in
getSuggestedNewIndexes() and getModifiedFiles() to ensure processedRepositories
is never null before calling .size(), contains(), add(), or when saving.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`:
- Around line 178-181: The comment above checkpointManager.clear() is misleading
because AntikytheraException is caught during repository processing and failures
do not prevent checkpointManager.clear() from running; either update the comment
to state that clearing the checkpoint is unconditional and intended only for
crash-recovery behavior, or add a guard (e.g., track a boolean like hadFailures
set in the AntikytheraException catch in the repository-processing loop) and
only call checkpointManager.clear() when hadFailures is false so failed repos
will be retried; update the comment accordingly near checkpointManager.clear()
and reference the AntikytheraException catch block and checkpointManager.clear()
to locate the changes.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java`:
- Around line 501-507: Replace the throwing IllegalStateException when
TypeWrapper typeWrapper = AntikytheraRunTime.getResolvedTypes().get(className)
returns null with resilient handling: log an error or warning (including
className and context) and skip processing that class (continue the loop) so
batch processing doesn't halt; ensure any cleanup or checkpoint-aware
bookkeeping for the skipped class (same loop that references TypeWrapper and
className) is preserved so the run can be resumed from checkpoints.
- Around line 709-721: In reorderMethodArguments, when the mapping lookup for an
index (oldIdx == null or invalid) triggers the fallback that sequentially fills
remaining args from argsArray, add a warning log before doing the fallback to
make this event visible; include identifying context such as the method name,
the current index i, the map contents (or size) and original argsArray length so
operators can trace why mapping was incomplete — update the code path using
symbols map, argsArray, args and the reorderMethodArguments method to emit a
clear warning via the existing logger before executing the fallback loop.
🧹 Nitpick comments (9)
pom.xml (1)
64-69: Mockito without<scope>test</scope>— intentional but worth a note.The comment explains why (
Evaluator in antikythera requires Mockito at runtime for field setup), but the missing<scope>test</scope>means Mockito will be packaged as a compile/runtime dependency. If this artifact is ever published or deployed as a library, Mockito will be pulled transitively. Consider adding a code comment clarifying that restoringtestscope would break the antikythera evaluator, so future contributors don't "fix" it.src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationTracker.java (1)
133-133: Consider usingStandardCharsets.UTF_8withFileReader.
new FileReader(csvFile)uses the platform's default charset, which can vary across environments. If the CSV is always UTF-8 (common for generated files), pass the charset explicitly to ensure portable behavior.✏️ Suggested fix
- try (BufferedReader reader = new BufferedReader(new FileReader(csvFile))) { + try (BufferedReader reader = new BufferedReader(new FileReader(csvFile, java.nio.charset.StandardCharsets.UTF_8))) {src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java (2)
85-107: RedundantsessionId/startTimeassignments — constructor already sets them.
new Checkpoint()(line 87 and 102) already initializessessionIdandstartTimein its constructor (lines 273–276). The explicit assignments on lines 88–89 and 103–104 are redundant.♻️ Remove redundant assignments
if (!checkpointFile.exists()) { logger.debug("No checkpoint file found, starting fresh"); checkpoint = new Checkpoint(); - checkpoint.sessionId = UUID.randomUUID().toString(); - checkpoint.startTime = Instant.now().toString(); loaded = true; return false; } try { checkpoint = objectMapper.readValue(checkpointFile, Checkpoint.class); logger.info("Loaded checkpoint from {} - {} repositories already processed", checkpointFile.getName(), checkpoint.processedRepositories.size()); loaded = true; return true; } catch (IOException e) { logger.warn("Failed to load checkpoint file: {}. Starting fresh.", e.getMessage()); checkpoint = new Checkpoint(); - checkpoint.sessionId = UUID.randomUUID().toString(); - checkpoint.startTime = Instant.now().toString(); loaded = true; return false; }
262-277: Checkpoint fields arepublic— consider encapsulation or a note.All fields of the
Checkpointinner class are public and mutable. This is typical for Jackson data-transfer objects, but it means any caller can mutate checkpoint state directly, bypassingCheckpointManager's methods. This is fine ifCheckpointstays internal, but it's apublic staticclass. If exposing it is intentional, this is acceptable — just noting the trade-off.src/main/java/sa/com/cloudsolutions/antikythera/examples/WhereClauseCollector.java (1)
259-292: Consider narrowing theprocessFromItemparameter type fromObjecttoFromItem.Since
ParenthesedSelectimplementsFromItem, and all callers pass either aFromItemor aParenthesedSelect, the parameter type could beFromItemfor better type safety. The only exception is line 67 where aParenthesedSelect(which is also aFromItem) is passed from thevisit(Select)method.src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java (1)
244-263:getMethodCallersandgetIndexedMethodNamesreturn mutable internal collections.
getMethodCallersreturns the internalSet<CallerInfo>directly, andgetIndexedMethodNamesreturns the livekeySet()view. If a caller mutates the returned collection, it silently corrupts the index. This matches the existing pattern ingetFieldDependencies, but could be worth wrapping withCollections.unmodifiableSet(...)for safety.Proposed defensive copies
public static Set<CallerInfo> getMethodCallers(String repositoryFqn, String methodName) { Map<String, Set<CallerInfo>> repoIndex = methodCallIndex.get(repositoryFqn); if (repoIndex == null) { return Set.of(); } Set<CallerInfo> callers = repoIndex.get(methodName); - return callers != null ? callers : Set.of(); + return callers != null ? Collections.unmodifiableSet(callers) : Set.of(); } public static Set<String> getIndexedMethodNames(String repositoryFqn) { Map<String, Set<CallerInfo>> repoIndex = methodCallIndex.get(repositoryFqn); - return repoIndex != null ? repoIndex.keySet() : Set.of(); + return repoIndex != null ? Collections.unmodifiableSet(repoIndex.keySet()) : Set.of(); }src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java (1)
55-56: Static mutabletargetClassis consistent with existingquietModepattern, but note the thread-safety implication.Both
targetClassandquietModearestaticfields mutated via static setters. This is fine for single-threaded CLI usage but would be problematic ifQueryOptimizationCheckerwere ever used concurrently. Not a blocker given the current CLI context.src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java (2)
147-157: DoublecheckpointManager.load()— once here and again insidesuper.analyze().
checkpointManager.load()is called at line 153, thensuper.analyze()calls it again at line 105 ofQueryOptimizationChecker.analyze(). This is likely idempotent but wasteful and splits checkpoint restoration logic across parent and child, making it fragile.Consider consolidating: either skip the load here and let
super.analyze()handle it (restoring modified files aftersuperloads), or override the load behavior so it's only called once.Possible consolidation
`@Override` public int analyze() throws IOException, ReflectiveOperationException, InterruptedException { - // Restore modified files from checkpoint before analysis - if (checkpointManager.hasCheckpoint()) { - // Load will be called by super.analyze(), but we need to check first - // and restore modified files if resuming - checkpointManager.load(); - restoreModifiedFilesFromCheckpoint(); - } + // super.analyze() will call checkpointManager.load(); we restore modified files + // after it loads, in analyzeRepository's preamble, or via a post-load hook. return super.analyze(); }Alternatively, hook
restoreModifiedFilesFromCheckpoint()right aftersuper.analyze()callscheckpointManager.load()— e.g., by checking once at the start ofanalyzeRepository.
655-658: StaticparameterMappingCacheis never cleared.This cache grows across all method renames for the entire run. While unlikely to cause problems in typical single-run CLI usage, it could lead to subtle bugs if method names are reused with different parameter lists across repositories (the cache key is just
oldName->newName).Consider clearing it in
clearFieldDependencies()or at the start ofanalyzeRepository().
|
|
||
| ## 🚀 AI Review Prompt | ||
| Carry out a review of the diff between the current branch and the main branch, focusing on the optimizations made by the `QueryOptimizer` tool. | ||
| Carry out a review of the diff of the current, focusing on the optimizations made by the `QueryOptimizer` tool. |
There was a problem hiding this comment.
Incomplete sentence — missing noun after "current".
The prompt reads "Carry out a review of the diff of the current, focusing on…" which is grammatically incomplete. It likely should be "of the current branch" or "of the current changes".
✏️ Suggested fix
-Carry out a review of the diff of the current, focusing on the optimizations made by the `QueryOptimizer` tool.
+Carry out a review of the diff of the current branch, focusing on the optimizations made by the `QueryOptimizer` tool.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Carry out a review of the diff of the current, focusing on the optimizations made by the `QueryOptimizer` tool. | |
| Carry out a review of the diff of the current branch, focusing on the optimizations made by the `QueryOptimizer` tool. |
🤖 Prompt for AI Agents
In `@docs/query-optimization-review-prompt.md` at line 8, The sentence "Carry out
a review of the diff of the current, focusing on the optimizations made by the
`QueryOptimizer` tool." is grammatically incomplete; replace "current" with a
clear noun like "branch", "changes", or "diff" so the prompt reads e.g. "Carry
out a review of the diff of the current branch, focusing on the optimizations
made by the `QueryOptimizer` tool." Update the prompt text in
docs/query-optimization-review-prompt.md accordingly, keeping the referenced
`QueryOptimizer` token intact.
| try { | ||
| checkpoint = objectMapper.readValue(checkpointFile, Checkpoint.class); | ||
| logger.info("Loaded checkpoint from {} - {} repositories already processed", | ||
| checkpointFile.getName(), checkpoint.processedRepositories.size()); | ||
| loaded = true; |
There was a problem hiding this comment.
Potential NPE if deserialized checkpoint contains "processedRepositories": null.
When Jackson deserializes a JSON file with an explicit null for processedRepositories, the field initializer (new HashSet<>()) is overwritten with null. Line 97 then calls .size() on it, causing a NullPointerException. The same applies to save() (line 119), isProcessed() (line 147), and markProcessed() (line 156).
Consider adding a null-guard after deserialization, similar to the null-checks already present in getSuggestedNewIndexes() and getModifiedFiles().
🛡️ Proposed fix — normalize nulls after deserialization
try {
checkpoint = objectMapper.readValue(checkpointFile, Checkpoint.class);
+ if (checkpoint.processedRepositories == null) {
+ checkpoint.processedRepositories = new HashSet<>();
+ }
logger.info("Loaded checkpoint from {} - {} repositories already processed",
checkpointFile.getName(), checkpoint.processedRepositories.size());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| checkpoint = objectMapper.readValue(checkpointFile, Checkpoint.class); | |
| logger.info("Loaded checkpoint from {} - {} repositories already processed", | |
| checkpointFile.getName(), checkpoint.processedRepositories.size()); | |
| loaded = true; | |
| try { | |
| checkpoint = objectMapper.readValue(checkpointFile, Checkpoint.class); | |
| if (checkpoint.processedRepositories == null) { | |
| checkpoint.processedRepositories = new HashSet<>(); | |
| } | |
| logger.info("Loaded checkpoint from {} - {} repositories already processed", | |
| checkpointFile.getName(), checkpoint.processedRepositories.size()); | |
| loaded = true; |
🤖 Prompt for AI Agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java`
around lines 94 - 98, After deserializing the Checkpoint in CheckpointManager
(the objectMapper.readValue call), guard against a null processedRepositories by
initializing checkpoint.processedRepositories to an empty HashSet if it is null;
do the same normalization wherever Checkpoint instances are accepted (so save(),
isProcessed(), markProcessed()) or add a private normalize(Checkpoint) helper
used after readValue—mirror the existing null-guards used in
getSuggestedNewIndexes() and getModifiedFiles() to ensure processedRepositories
is never null before calling .size(), contains(), add(), or when saving.
|
|
||
| // Clear checkpoint on successful completion (all repos processed without error) | ||
| checkpointManager.clear(); | ||
|
|
There was a problem hiding this comment.
Comment is misleading — checkpoint is cleared even when repositories fail with errors.
The comment says "Clear checkpoint on successful completion (all repos processed without error)", but AntikytheraException is caught at line 158 and the loop continues, so checkpointManager.clear() runs unconditionally after all repositories are attempted. This means failed repositories won't be retried on the next run. If this is intentional (checkpoint is only for crash recovery), update the comment. If failed repos should be retried, add a guard.
Suggested comment fix (if behavior is intentional)
- // Clear checkpoint on successful completion (all repos processed without error)
+ // Clear checkpoint after all repositories have been attempted.
+ // Failed repositories are not retried - checkpoint is only for crash/interruption recovery.
checkpointManager.clear();🤖 Prompt for AI Agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`
around lines 178 - 181, The comment above checkpointManager.clear() is
misleading because AntikytheraException is caught during repository processing
and failures do not prevent checkpointManager.clear() from running; either
update the comment to state that clearing the checkpoint is unconditional and
intended only for crash-recovery behavior, or add a guard (e.g., track a boolean
like hadFailures set in the AntikytheraException catch in the
repository-processing loop) and only call checkpointManager.clear() when
hadFailures is false so failed repos will be retried; update the comment
accordingly near checkpointManager.clear() and reference the
AntikytheraException catch block and checkpointManager.clear() to locate the
changes.
| TypeWrapper typeWrapper = AntikytheraRunTime.getResolvedTypes().get(className); | ||
|
|
||
| if (typeWrapper == null) { | ||
| throw new IllegalStateException( | ||
| "Class " + className + " found in method call index but not in resolved types. " + | ||
| "This may indicate incomplete preprocessing or an external dependency that should be excluded."); | ||
| } |
There was a problem hiding this comment.
IllegalStateException in batch processing halts the entire run.
If a class appears in the method call index but is not in resolved types (e.g., an excluded external dependency), this throw crashes the run, losing all prior progress. Consider logging an error and continue-ing to the next class, since the checkpoint should allow recovery.
Proposed resilient handling
if (typeWrapper == null) {
- throw new IllegalStateException(
- "Class " + className + " found in method call index but not in resolved types. " +
- "This may indicate incomplete preprocessing or an external dependency that should be excluded.");
+ logger.error("Class {} found in method call index but not in resolved types. " +
+ "Skipping — this may indicate incomplete preprocessing or an external dependency.", className);
+ continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TypeWrapper typeWrapper = AntikytheraRunTime.getResolvedTypes().get(className); | |
| if (typeWrapper == null) { | |
| throw new IllegalStateException( | |
| "Class " + className + " found in method call index but not in resolved types. " + | |
| "This may indicate incomplete preprocessing or an external dependency that should be excluded."); | |
| } | |
| TypeWrapper typeWrapper = AntikytheraRunTime.getResolvedTypes().get(className); | |
| if (typeWrapper == null) { | |
| logger.error("Class {} found in method call index but not in resolved types. " + | |
| "Skipping — this may indicate incomplete preprocessing or an external dependency.", className); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java`
around lines 501 - 507, Replace the throwing IllegalStateException when
TypeWrapper typeWrapper = AntikytheraRunTime.getResolvedTypes().get(className)
returns null with resilient handling: log an error or warning (including
className and context) and skip processing that class (continue the loop) so
batch processing doesn't halt; ensure any cleanup or checkpoint-aware
bookkeeping for the skipped class (same loop that references TypeWrapper and
className) is preserved so the run can be resumed from checkpoints.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java (1)
648-653:⚠️ Potential issue | 🟠 Major
PrintWriterresource leak — not closed in try-with-resources.If
writer.print(content)throws (e.g., disk full triggers an underlying I/O error), the writer is never closed and the file handle leaks. Use try-with-resources.Proposed fix
private static boolean writeFile(File f, String content) throws FileNotFoundException { - PrintWriter writer = new PrintWriter(f); - writer.print(content); // Use the content variable we already computed - writer.close(); - return true; + try (PrintWriter writer = new PrintWriter(f)) { + writer.print(content); + } + return true; }
🤖 Fix all issues with AI agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java`:
- Around line 114-123: The save() method in CheckpointManager currently swallows
IOException which makes callers like QueryOptimizer.analyzeRepository assume the
checkpoint was persisted; change save() to propagate failures or return status
so callers can react: either (a) rethrow the IOException (or a checked/unchecked
wrapper) from CheckpointManager.save() so callers (e.g.,
QueryOptimizer.analyzeRepository) can handle retry/abort, or (b) change save()
to return boolean (true on success, false on failure) and update callers to
check the result and handle failures (retry, abort, or log+stop) accordingly;
locate the method named save() in class CheckpointManager and update all call
sites (e.g., QueryOptimizer.analyzeRepository) to follow the chosen contract.
- Around line 263-277: The private fields sessionId, startTime, and lastUpdate
in the nested Checkpoint class are not visible to Jackson so they won't be
serialized/deserialized; update the Checkpoint class (or its fields) so Jackson
can read/write them: either make sessionId, startTime, and lastUpdate public (to
match processedRepositories) or add standard public getters/setters (e.g.,
getSessionId()/setSessionId(), getStartTime()/setStartTime(),
getLastUpdate()/setLastUpdate()) so deserialization restores the resumed session
ID, start time and last update; alternatively annotate the private fields with
`@JsonProperty` if you prefer to keep them private.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java`:
- Around line 45-51: The static profiling counters totalLppTime,
totalDiskWriteTime, and lppCallCount are being reset inside
updateMethodCallSignaturesBatched which wipes accumulated data before the final
updateFiles run; fix this by either making these counters instance (non-static)
fields on QueryOptimizer so they persist for the whole run, or move the reset
logic out of updateMethodCallSignaturesBatched to a single initialization point
at the start of the entire run and ensure you log the final values after
updateFiles completes (the code paths using writeFile should then contribute to
the final logged totals).
- Around line 655-698: The static parameterMappingCache key (built in
reorderMethodArguments where cacheKey = oldMethod.getNameAsString()+ "->" +
newMethod.getNameAsString()) is too weak and can collide across repository
analyses; update the cache key to include unique method signatures (e.g.,
parameter types/names or getSignature() for both oldMethod and newMethod) or
include a repository identifier (repositoryFqn) so the mapping is scoped
per-repository, or alternatively clear parameterMappingCache at the start of
each repository analysis (e.g., in analyzeRepository); change the key
construction referenced in reorderMethodArguments and keep the
parameterMappingCache field as-is.
🧹 Nitpick comments (5)
src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java (1)
58-62:STRINGis an overly generic constant name for a JSON Schema type literal.It's only used to represent the
"string"value in the JSON schema definition (line 192). A name likeJSON_TYPE_STRINGorSCHEMA_TYPE_STRINGwould be clearer and avoid confusion withjava.lang.String.♻️ Suggested rename
- public static final String STRING = "string"; + public static final String JSON_TYPE_STRING = "string";And update the usage on line 192:
- properties.putObject("originalMethod").put("type", STRING); + properties.putObject("originalMethod").put("type", JSON_TYPE_STRING);src/main/java/sa/com/cloudsolutions/antikythera/examples/Fields.java (1)
212-227:findMatchingFielddoesn't handle MockitoverifybutextractFieldNameinBatchedNameChangeProcessordoes — potential index gap.
findMatchingFieldonly matchesNameExprandFieldAccessExpr(this.x). The Mockitoverify(field)pattern is handled separately inindexMethodCall(line 180–189), which is correct. However,indexMethodReferences(line 196–206) callsfindMatchingFieldonmre.getScope()without a verify-pattern fallback. If a method reference appears asverify(repo)::method(unlikely but possible in Mockito test helpers), it would be missed from the index.This is a very minor edge case — just noting it for awareness.
src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java (3)
147-157: Doubleload()call when checkpoint exists — harmless but wasteful.
analyze()callscheckpointManager.load()on line 153, andsuper.analyze()on line 156 likely callsload()again. Theloadedflag makes this idempotent, so it's correct — but the explicit load here beforesuper.analyze()exists solely to callrestoreModifiedFilesFromCheckpoint()with loaded state. A brief comment clarifying this intent would help future readers.
182-185: Linear scan overmethodRenamesfor every result — O(n·m) where aSetlookup would be O(n).
actOnAnalysisResultdoesmethodRenames.stream().anyMatch(...)for everyQueryAnalysisResult. SincemethodRenamescan grow with repository size, consider precomputing aSet<String>of old method names before the loop at line 105.Proposed fix
+ // Pre-compute set of renamed method names for O(1) lookup + Set<String> renamedMethodNames = methodRenames.stream() + .map(MethodRename::oldMethodName) + .collect(java.util.stream.Collectors.toSet()); + // Process all results (update annotations, etc.) for (QueryAnalysisResult result : results) { - actOnAnalysisResult(result, methodRenames); + actOnAnalysisResult(result, renamedMethodNames); }Then change the method signature and line 184 to use
renamedMethodNames.contains(originalMethodName).
858-884: ExcessiveINFO-level logging in inner-loop methods.Lines 864, 575, 510, 516, and 880 all log at
INFOlevel for per-class or per-compilation-unit processing details. For a repository with hundreds of dependent classes, this will flood the log. Consider usingDEBUGfor per-class details and keepingINFOonly for batch-level summaries.
| public void save() { | ||
| checkpoint.lastUpdate = Instant.now().toString(); | ||
| try { | ||
| objectMapper.writeValue(checkpointFile, checkpoint); | ||
| logger.debug("Checkpoint saved: {} repositories processed", | ||
| checkpoint.processedRepositories.size()); | ||
| } catch (IOException e) { | ||
| logger.error("Failed to save checkpoint: {}", e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
save() silently swallows IOException — callers assume checkpoint was persisted.
analyzeRepository in QueryOptimizer calls checkpointManager.save() after each repository to ensure resume capability. If save() fails silently, the caller proceeds believing the checkpoint is durable, but the next interruption will replay already-processed work (or worse, lose modified-file tracking).
Consider either propagating the exception or returning a boolean so callers can react.
🤖 Prompt for AI Agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java`
around lines 114 - 123, The save() method in CheckpointManager currently
swallows IOException which makes callers like QueryOptimizer.analyzeRepository
assume the checkpoint was persisted; change save() to propagate failures or
return status so callers can react: either (a) rethrow the IOException (or a
checked/unchecked wrapper) from CheckpointManager.save() so callers (e.g.,
QueryOptimizer.analyzeRepository) can handle retry/abort, or (b) change save()
to return boolean (true on success, false on failure) and update callers to
check the result and handle failures (retry, abort, or log+stop) accordingly;
locate the method named save() in class CheckpointManager and update all call
sites (e.g., QueryOptimizer.analyzeRepository) to follow the chosen contract.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
| private String sessionId; | ||
| private String startTime; | ||
| private String lastUpdate; |
There was a problem hiding this comment.
Suggestion: The checkpoint metadata fields were changed from public to private without adding getters/setters or Jackson annotations, so with the default ObjectMapper configuration these fields will no longer be serialized/deserialized, causing sessionId, startTime, and lastUpdate to be lost across runs and breaking the intended checkpoint tracking behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ Checkpoint JSON omits sessionId/startTime/lastUpdate metadata fields.
- ⚠️ Session ID changes when resuming from existing checkpoint file.
- ⚠️ Docs JSON example no longer matches runtime checkpoint output.
- ⚠️ Harder to correlate optimization runs across interruptions and resumes.| private String sessionId; | |
| private String startTime; | |
| private String lastUpdate; | |
| public String sessionId; | |
| public String startTime; | |
| public String lastUpdate; |
Steps of Reproduction ✅
1. Run the unit test setup in
`src/test/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManagerTest.java:23-27`,
which creates a `CheckpointManager` pointing at a temp file and calls `manager.load()`
(CheckpointManager.load at
`src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java:79-108`).
2. In the same JVM, mark some repositories and call `manager.save()` as in
`CheckpointManagerTest.testMarkProcessedAndSave` at lines 45-55; this invokes
`CheckpointManager.save()` (`CheckpointManager.java:114-123`), which writes the checkpoint
JSON to disk.
3. Inspect the written JSON file (the `checkpointFile` used in the test). Because
`Checkpoint.Checkpoint` declares `sessionId`, `startTime`, and `lastUpdate` as private
fields at `CheckpointManager.java:264-266` and `ObjectMapper` is created with default
visibility at `CheckpointManager.java:65-69` (no custom field visibility configuration or
Jackson annotations), the JSON will contain only `processedRepositories`,
`suggestedNewIndexes`, `suggestedMultiColumnIndexes`, and `modifiedFiles`, but no
`sessionId`, `startTime`, or `lastUpdate` keys.
4. Create a new `CheckpointManager` instance with the same file path, as done in
`CheckpointManagerTest.testResumeFromCheckpoint` at lines 67-73, and call
`newManager.load()` (`CheckpointManager.java:79-107`), then call
`newManager.getSessionId()` (`CheckpointManager.java:241-247`); the returned UUID comes
solely from the `Checkpoint` default constructor (`CheckpointManager.java:272-276`) and is
not restored from the JSON file, demonstrating that session metadata is not persisted
across runs.
5. Compare this behavior with the documentation example JSON in
`docs/spring_boot_2.3_to_2.4_migration.md:1632-1635`, which shows `startTime` as part of
the checkpoint data; the runtime JSON produced by the current code no longer includes
these documented metadata fields, confirming the loss of persisted session information.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/sa/com/cloudsolutions/antikythera/examples/CheckpointManager.java
**Line:** 264:266
**Comment:**
*Logic Error: The checkpoint metadata fields were changed from public to private without adding getters/setters or Jackson annotations, so with the default ObjectMapper configuration these fields will no longer be serialized/deserialized, causing `sessionId`, `startTime`, and `lastUpdate` to be lost across runs and breaking the intended checkpoint tracking behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`:
- Around line 1388-1414: The block in configureFromSettings that reads
"target_class" and "skip_class" is over-indented making it appear nested
incorrectly; adjust the indentation so the statements under the if
(queryOptimizer != null) conditional (the retrieval of targetClassValue, the
targetClass assignment/print, the skipClassValue handling and related prints)
align with the if body, keeping the existing logic intact and ensuring
targetClass and skipClass (and the Settings.getProperty("query_optimizer") call)
remain at the correct single indentation level.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java`:
- Around line 644-649: The writeFile method currently constructs a PrintWriter
without safe cleanup and blindly returns true even if writes fail; update
writeFile to open the PrintWriter in a try-with-resources (using the existing
File f) so it is always closed, call writer.print(content) inside that block,
then check writer.checkError() after printing and return false if it indicates
failure; also catch any IO-related exceptions (or let the constructor's
FileNotFoundException propagate) and ensure the method returns false on write/IO
failures instead of unconditionally true. Ensure you modify the writeFile method
and references to PrintWriter/checkError to implement this behavior.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/WhereClauseCollector.java`:
- Line 61: Fix the typo in the comment inside WhereClauseCollector: change
"unwraping" to "unwrapping" (the comment near the select-body handling logic in
the WhereClauseCollector class) so the inline documentation reads "unwrapping
the select body".
🧹 Nitpick comments (14)
src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java (5)
510-510: Per-classINFOlogging inside batch loops is excessively verbose.Lines 510, 571, 863, and 879 all emit
INFO-level messages for every class processed in the batch loop. For repositories with hundreds of dependent classes, this floods the log. The progress logging every 100 classes (line 522) already provides adequate visibility.Demote these to
DEBUGto keepINFOoutput meaningful.Example changes
- logger.info(" Visiting class: {} (fields: {})", className, fieldNames); + logger.debug(" Visiting class: {} (fields: {})", className, fieldNames);- logger.info(" Got CompilationUnit in {}ms, starting processing...", getCuTime); + logger.debug(" Got CompilationUnit in {}ms, starting processing...", getCuTime);- logger.info(" Found {} total method calls, filtering...", allMethodCalls.size()); + logger.debug(" Found {} total method calls, filtering...", allMethodCalls.size());- logger.info(" Processed {} method calls in {}ms (filter: {}ms)", + logger.debug(" Processed {} method calls in {}ms (filter: {}ms)",Also applies to: 571-571, 862-863, 879-880
482-488: Diagnostic comparison with the old approach is unnecessary overhead in production.Lines 483–488 call
Fields.getFieldDependencies(fullyQualifiedName)and compute a reduction percentage solely for a log message. IfgetFieldDependenciesis non-trivial (e.g., builds/copies a map), this work is wasted. Consider guarding it behindlogger.isInfoEnabled()or removing it once the optimization is validated.
651-653: StaticparameterMappingCacheis never cleared between repositories.The cache key now correctly includes
repositoryFqnand full signatures (good fix from the prior review). However, the cache grows unboundedly across the entire run. For very large codebases with many repositories, consider clearing it at the end ofanalyzeRepository()or using a bounded cache.
962-968: Method reference rename skips ambiguous cases silently.When multiple
MethodRenamecandidates exist for the same old method name (overloaded methods), the method reference rename is skipped entirely (line 963:candidates.size() == 1). This is a safe default, but the skip is silent — no log message is emitted to help operators understand why a particular method reference wasn't updated.Add a debug log for skipped ambiguous renames
List<MethodRename> candidates = renameMap.get(currentMethodName); if (candidates != null && candidates.size() == 1) { // Safe to rename - only one candidate, no ambiguity mre.setIdentifier(candidates.get(0).newMethodName()); modified = true; methodCallsUpdated++; + } else if (candidates != null && candidates.size() > 1) { + visitorLogger.debug("Skipping ambiguous method reference rename for '{}' — {} overloaded candidates", + currentMethodName, candidates.size()); }
45-46: Static mutablemodifiedFilesandwrittenFilessets leak state across instances.Both sets are
static finalcollections that are never cleared. In tests or multi-instance scenarios, state from a previousQueryOptimizerinstance persists. The profiling counters (lines 49–51) are correctly made instance-scoped; these sets should follow the same pattern. Additionally, the parent classQueryOptimizationCheckeruses instance fields (e.g.,suggestedNewIndexes,suggestedMultiColumnIndexes), establishing the pattern for this codebase.Note: the
updateMethodCallSignaturemethod at line 418 isprivate staticand accesses the staticmodifiedFilesset. Converting these sets to instance-scoped requires either making that method non-static or passingmodifiedFilesas a parameter.src/main/java/sa/com/cloudsolutions/antikythera/examples/WhereClauseCollector.java (1)
261-275:processFromItemacceptsObjectinstead ofFromItem, losing compile-time type safety.The parameter was widened from
FromItemtoObject, but the only meaningful type handled isParenthesedSelect. All existing call sites pass either aFromItemorParenthesedSelect(which implementsFromItem). UsingFromItemas the parameter type would preserve compile-time safety while still matchingParenthesedSelectviainstanceof.Suggested fix
- private void processFromItem(Object fromItem) { + private void processFromItem(FromItem fromItem) {src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationExtractor.java (2)
106-124:getWhereFromSelectfor UNION queries silently returns only the first branch's WHERE clause.For a
SetOperationList(UNION), this returns the WHERE from the first sub-select that has one, ignoring all others. This is consistent with the tests (e.g.,testExtractWhereClauseText_Unionexpects"id = 1"), but the Javadoc forextractWhereClauseTextsays it "preserves OR/AND structure" which could be misleading for UNION queries. Consider clarifying in the doc that only the first WHERE clause is returned for set operations.
126-130: Indentation ofConditionExtractionResultrecord is inconsistent.The record declaration is indented with 8 spaces while the rest of the class uses 4-space indentation.
Suggested fix
- /** - * Result object containing both WHERE and JOIN conditions. - */ - public record ConditionExtractionResult(List<WhereCondition> whereConditions, List<JoinCondition> joinConditions) { + /** + * Result object containing both WHERE and JOIN conditions. + */ + public record ConditionExtractionResult(List<WhereCondition> whereConditions, List<JoinCondition> joinConditions) { }src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.java (1)
588-622: Consider asserting the fallback result, not just absence of exceptions.The test verifies no exception is thrown but doesn't validate the returned
OptimizedQueryResult. Since the fallback should return the original query's column order, a quick assertion would strengthen the test:Suggested addition
- assertDoesNotThrow(() -> geminiAIService.extractRecommendedColumnOrder(optimizedCode, mockQuery)); + GeminiAIService.OptimizedQueryResult result = + assertDoesNotThrow(() -> geminiAIService.extractRecommendedColumnOrder(optimizedCode, mockQuery)); + assertNotNull(result); + assertEquals(mockQuery, result.optimizedQuery(), "Should fall back to original query");src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java (1)
55-59: Static mutable fieldstargetClassandskipClassare not thread-safe.These are
staticmutable fields set via public static setters and read duringanalyze(). If this class is ever used concurrently (e.g., in tests running in parallel), this will cause data races. For a CLI tool this is likely acceptable, but consider making them instance fields instead, which would also eliminate the global state coupling.src/test/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationCheckerTest.java (2)
276-278: Weak assertions —assertTrue(true)and overly permissive catch blocks don't verify behavior.Multiple tests (lines 278, 324, 387, 457) use
assertTrue(true)as a "no exception thrown" check, and catch blocks at lines 458-461 accept any exception. Consider usingassertDoesNotThrow(...)for the happy path, andassertThrows(SpecificException.class, ...)for expected failures. This makes intent explicit and avoids silently passing on unexpected exceptions.Also applies to: 385-388, 455-461
247-256: Add getter/setter forskipClassto match accessor pattern oftargetClassandquietMode.The field
skipClassis accessed directly without accessors (lines 251, 254), whiletargetClassusessetTargetClass()/getTargetClass()andquietModeusessetQuietMode()/isQuietMode(). Add corresponding accessors forskipClassto enforce the same encapsulation pattern and improve refactoring resilience.src/test/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationCheckerRedundancyTest.java (2)
54-58: Inconsistent annotation style: use the import instead of the fully-qualified name.Line 54 uses
@org.junit.jupiter.api.AfterEachwhile the other annotations (@BeforeAll,@BeforeEach,@Test) are imported at the top of the file. Add the import and use@AfterEachfor consistency.Proposed fix
Add to imports:
import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test;Then at line 54:
- `@org.junit.jupiter.api.AfterEach` + `@AfterEach`
28-45: Duplicated Liquibase temp-file setup across test classes.The
setupClass()method (lines 28–45) is nearly identical toQueryOptimizationCheckerTest.setupClass(). Consider extracting this into a shared test utility or abstract base class to reduce duplication.
| /** | ||
| * Configures QueryOptimizationChecker from generator.yml settings. | ||
| * Reads target_class configuration. | ||
| * Call this from main() after Settings.loadConfigMap(). | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public static void configureFromSettings() { | ||
| // Read target_class from query_optimizer section | ||
| Map<String, Object> queryOptimizer = (Map<String, Object>) Settings.getProperty("query_optimizer"); | ||
| if (queryOptimizer != null) { | ||
| Object targetClassValue = queryOptimizer.get("target_class"); | ||
| if (targetClassValue instanceof String s && !s.isBlank()) { | ||
| targetClass = s; | ||
| System.out.printf("🎯 Target class filter: %s%n", s); | ||
| } else { | ||
| System.out.println("ℹ️ No target_class filter specified (processing all repositories)"); | ||
| } | ||
|
|
||
| Object skipClassValue = queryOptimizer.get("skip_class"); | ||
| if (skipClassValue instanceof String s && !s.isBlank()) { | ||
| skipClass = s; | ||
| System.out.printf("🚫 Skip class filter: %s%n", s); | ||
| } | ||
| } else { | ||
| System.out.println("ℹ️ No query_optimizer section in settings (processing all repositories)"); | ||
| } | ||
| } |
There was a problem hiding this comment.
configureFromSettings has inconsistent indentation inside the if block.
Lines 1399-1410 are indented with an extra 4 spaces beyond what the surrounding if block requires, making it look like they're inside a nested block that doesn't exist.
Suggested fix
if (queryOptimizer != null) {
Object targetClassValue = queryOptimizer.get("target_class");
- if (targetClassValue instanceof String s && !s.isBlank()) {
- targetClass = s;
- System.out.printf("🎯 Target class filter: %s%n", s);
- } else {
- System.out.println("ℹ️ No target_class filter specified (processing all repositories)");
- }
-
- Object skipClassValue = queryOptimizer.get("skip_class");
- if (skipClassValue instanceof String s && !s.isBlank()) {
- skipClass = s;
- System.out.printf("🚫 Skip class filter: %s%n", s);
- }
+ if (targetClassValue instanceof String s && !s.isBlank()) {
+ targetClass = s;
+ System.out.printf("🎯 Target class filter: %s%n", s);
+ } else {
+ System.out.println("ℹ️ No target_class filter specified (processing all repositories)");
+ }
+
+ Object skipClassValue = queryOptimizer.get("skip_class");
+ if (skipClassValue instanceof String s && !s.isBlank()) {
+ skipClass = s;
+ System.out.printf("🚫 Skip class filter: %s%n", s);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Configures QueryOptimizationChecker from generator.yml settings. | |
| * Reads target_class configuration. | |
| * Call this from main() after Settings.loadConfigMap(). | |
| */ | |
| @SuppressWarnings("unchecked") | |
| public static void configureFromSettings() { | |
| // Read target_class from query_optimizer section | |
| Map<String, Object> queryOptimizer = (Map<String, Object>) Settings.getProperty("query_optimizer"); | |
| if (queryOptimizer != null) { | |
| Object targetClassValue = queryOptimizer.get("target_class"); | |
| if (targetClassValue instanceof String s && !s.isBlank()) { | |
| targetClass = s; | |
| System.out.printf("🎯 Target class filter: %s%n", s); | |
| } else { | |
| System.out.println("ℹ️ No target_class filter specified (processing all repositories)"); | |
| } | |
| Object skipClassValue = queryOptimizer.get("skip_class"); | |
| if (skipClassValue instanceof String s && !s.isBlank()) { | |
| skipClass = s; | |
| System.out.printf("🚫 Skip class filter: %s%n", s); | |
| } | |
| } else { | |
| System.out.println("ℹ️ No query_optimizer section in settings (processing all repositories)"); | |
| } | |
| } | |
| /** | |
| * Configures QueryOptimizationChecker from generator.yml settings. | |
| * Reads target_class configuration. | |
| * Call this from main() after Settings.loadConfigMap(). | |
| */ | |
| `@SuppressWarnings`("unchecked") | |
| public static void configureFromSettings() { | |
| // Read target_class from query_optimizer section | |
| Map<String, Object> queryOptimizer = (Map<String, Object>) Settings.getProperty("query_optimizer"); | |
| if (queryOptimizer != null) { | |
| Object targetClassValue = queryOptimizer.get("target_class"); | |
| if (targetClassValue instanceof String s && !s.isBlank()) { | |
| targetClass = s; | |
| System.out.printf("🎯 Target class filter: %s%n", s); | |
| } else { | |
| System.out.println("ℹ️ No target_class filter specified (processing all repositories)"); | |
| } | |
| Object skipClassValue = queryOptimizer.get("skip_class"); | |
| if (skipClassValue instanceof String s && !s.isBlank()) { | |
| skipClass = s; | |
| System.out.printf("🚫 Skip class filter: %s%n", s); | |
| } | |
| } else { | |
| System.out.println("ℹ️ No query_optimizer section in settings (processing all repositories)"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`
around lines 1388 - 1414, The block in configureFromSettings that reads
"target_class" and "skip_class" is over-indented making it appear nested
incorrectly; adjust the indentation so the statements under the if
(queryOptimizer != null) conditional (the retrieval of targetClassValue, the
targetClass assignment/print, the skipClassValue handling and related prints)
align with the if body, keeping the existing logic intact and ensuring
targetClass and skipClass (and the Settings.getProperty("query_optimizer") call)
remain at the correct single indentation level.
| private boolean writeFile(File f, String content) throws FileNotFoundException { | ||
| PrintWriter writer = new PrintWriter(f); | ||
| writer.print(content); // Use the content variable we already computed | ||
| writer.close(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
PrintWriter is not closed safely, and write failures are silently ignored.
PrintWriter swallows I/O exceptions — print() never throws. If the underlying write fails (e.g., disk full), checkError() returns true but the method unconditionally returns true. Additionally, if the constructor itself fails, there's no try-with-resources to ensure cleanup.
Proposed fix using try-with-resources and error checking
- private boolean writeFile(File f, String content) throws FileNotFoundException {
- PrintWriter writer = new PrintWriter(f);
- writer.print(content); // Use the content variable we already computed
- writer.close();
- return true;
+ private boolean writeFile(File f, String content) throws IOException {
+ try (PrintWriter writer = new PrintWriter(f)) {
+ writer.print(content);
+ if (writer.checkError()) {
+ throw new IOException("Failed to write file: " + f.getAbsolutePath());
+ }
+ }
+ return true;
}🤖 Prompt for AI Agents
In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizer.java`
around lines 644 - 649, The writeFile method currently constructs a PrintWriter
without safe cleanup and blindly returns true even if writes fail; update
writeFile to open the PrintWriter in a try-with-resources (using the existing
File f) so it is always closed, call writer.print(content) inside that block,
then check writer.checkError() after printing and return false if it indicates
failure; also catch any IO-related exceptions (or let the constructor's
FileNotFoundException propagate) and ensure the method returns false on write/IO
failures instead of unconditionally true. Ensure you modify the writeFile method
and references to PrintWriter/checkError to implement this behavior.
| // Process PlainSelect statements | ||
| if (select.getPlainSelect() != null) { | ||
| processPlainSelect(select.getPlainSelect()); | ||
| // Handle different select types safely by unwraping the select body |
There was a problem hiding this comment.
Typo: "unwraping" → "unwrapping".
🤖 Prompt for AI Agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/WhereClauseCollector.java`
at line 61, Fix the typo in the comment inside WhereClauseCollector: change
"unwraping" to "unwrapping" (the comment near the select-body handling logic in
the WhereClauseCollector class) so the inline documentation reads "unwrapping
the select body".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`:
- Around line 998-1013: Update the Javadoc on parseIndexColumnsForTable(String
indexKey, String expectedTable) to state that it returns an empty List when the
key is invalid or the table doesn't match (rather than null); ensure the
description and `@return` tag mention an empty list (List.of()) is returned and
callers should check isEmpty() rather than null.
- Around line 1338-1345: The current logic only generates the Liquibase file
when repositoriesAnalyzed > 0 which discards restored suggestions from a resumed
run; update the condition after int repositoriesAnalyzed = checker.analyze() to
also check the checkpointer/restored suggestion containers (e.g.
checker.suggestedNewIndexes and checker.suggestedMultiColumnIndexes or whatever
fields hold restored suggestions) and/or repositoriesProcessed, so that
checker.generateLiquibaseChangesFile() is called if either repositoriesAnalyzed
> 0 OR there are any restored suggestions present; ensure this change is applied
around the generateLiquibaseChangesFile() call and that
checkpointManager.clear() still runs afterwards.
🧹 Nitpick comments (1)
src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java (1)
55-68: Static mutable state fortargetClass/skipClasscan cause test pollution.Both fields are
staticand mutated via static setters, which means state leaks between test cases unless each test explicitly resets them. This matches the pre-existing pattern forquietMode, but consider noting the coupling — if tests forconfigureFromSettingsorshouldSkipRepositorydon't clean up, unrelated tests may silently skip repositories.
| /** | ||
| * Parses an index key and returns the columns if the key is valid and matches the given table. | ||
| * | ||
| * @param indexKey the index key in format "table|col1,col2,..." | ||
| * @param expectedTable the table name to match | ||
| * @return list of columns if valid and table matches, null otherwise | ||
| */ | ||
| private List<String> parseIndexColumnsForTable(String indexKey, String expectedTable) { | ||
| String[] parts = indexKey.split("\\|", 2); | ||
| if (parts.length != 2) return List.of(); | ||
|
|
||
| String table = parts[0]; | ||
| if (!table.equals(expectedTable)) return List.of(); | ||
|
|
||
| return List.of(parts[1].split(",")); | ||
| } |
There was a problem hiding this comment.
Javadoc says "null otherwise" but the method returns an empty List.
Line 1003 documents the return as null when the key is invalid or the table doesn't match, but lines 1007 and 1010 return List.of(). Callers correctly check isEmpty(), so the behavior is fine — just the doc is misleading.
Suggested Javadoc fix
- * `@return` list of columns if valid and table matches, null otherwise
+ * `@return` list of columns if valid and table matches, empty list otherwise🤖 Prompt for AI Agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`
around lines 998 - 1013, Update the Javadoc on parseIndexColumnsForTable(String
indexKey, String expectedTable) to state that it returns an empty List when the
key is invalid or the table doesn't match (rather than null); ensure the
description and `@return` tag mention an empty list (List.of()) is returned and
callers should check isEmpty() rather than null.
| int repositoriesAnalyzed = checker.analyze(); | ||
|
|
||
| // Generate Liquibase file with suggested changes and include in master | ||
| checker.generateLiquibaseChangesFile(); | ||
| // Only generate Liquibase file if at least one repository was analyzed | ||
| if (repositoriesAnalyzed > 0) { | ||
| checker.generateLiquibaseChangesFile(); | ||
| } else { | ||
| System.out.println("\n⏭️ Skipping Liquibase generation - no new repositories were analyzed"); | ||
| } |
There was a problem hiding this comment.
Edge case: restored index suggestions are discarded when all repos are resumed.
If a previous run completed the analysis loop but crashed between OptimizationStatsLogger.flush() (line 151) and checkpointManager.clear() (line 165), the next run resumes all repos from checkpoint (repositoriesProcessed = 0), restores suggestedNewIndexes/suggestedMultiColumnIndexes from the checkpoint (line 200-201), but then skips Liquibase generation because repositoriesAnalyzed == 0. The checkpoint is subsequently cleared (line 165), losing the accumulated suggestions.
Consider checking whether there are restored index suggestions, not just whether new repos were analyzed:
Suggested fix
- if (repositoriesAnalyzed > 0) {
+ if (repositoriesAnalyzed > 0 || !checker.getSuggestedNewIndexes().isEmpty()
+ || !checker.getSuggestedMultiColumnIndexes().isEmpty()) {
checker.generateLiquibaseChangesFile();
} else {🤖 Prompt for AI Agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/QueryOptimizationChecker.java`
around lines 1338 - 1345, The current logic only generates the Liquibase file
when repositoriesAnalyzed > 0 which discards restored suggestions from a resumed
run; update the condition after int repositoriesAnalyzed = checker.analyze() to
also check the checkpointer/restored suggestion containers (e.g.
checker.suggestedNewIndexes and checker.suggestedMultiColumnIndexes or whatever
fields hold restored suggestions) and/or repositoriesProcessed, so that
checker.generateLiquibaseChangesFile() is called if either repositoriesAnalyzed
> 0 OR there are any restored suggestions present; ensure this change is applied
around the generateLiquibaseChangesFile() call and that
checkpointManager.clear() still runs afterwards.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
|
User description
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores
Removals
CodeAnt-AI Description
Make query optimization resumable and batch method-renames to only update actual callers
What Changed
Impact
✅ Faster repository-wide method renames✅ Fewer files rewritten during refactors✅ Shorter resume time after interruptions💡 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.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
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.