Introduce logical dedup operators for PPL #5014
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Heng Qian <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
# Conflicts: # opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java
Signed-off-by: Heng Qian <[email protected]>
qianheng-aws
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
Contributor
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Logical Dedup operator and related planner rules, introduces OpenSearch-specific RelBuilder and field trimmer, moves several plan classes into Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SqlToRel as OpenSearchSqlToRelConverter
participant RelBuilder as OpenSearchRelBuilder
participant Planner as CalcitePlanner
participant Rules as DedupRules
participant Pushdown as DedupPushdownRule
Client->>SqlToRel: parse SQL -> RelNode
SqlToRel->>RelBuilder: build initial RelNode (may include window)
RelBuilder->>Planner: submit RelNode tree
Planner->>Rules: PPLSimplifyDedupRule => replace pattern with LogicalDedup
Planner->>Rules: PPLDedupConvertRule => expand LogicalDedup to ROW_NUMBER plan
Planner->>Pushdown: DedupPushdownRule => generate IndexScan + PushDownContext
Planner->>Client: return final Calcite plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java (1)
73-91: Projection reordering logic is correct but consider adding a clarifying comment.The logic correctly places dedup fields first in the projection (for grouping), then appends remaining fields. This ensures the aggregate's group key aligns with dedup field positions. Consider adding a brief inline comment explaining why this reordering is necessary.
🔎 Suggested comment addition
List<RexNode> targetProjections = new ArrayList<>(); Set<Integer> dedupFieldsIndexSet = new HashSet<>(); + // Dedup fields must come first in the projection to align with the aggregate's group key for (RexNode dedupColumn : dedupColumns) {core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java (1)
27-31: Missing JavaDoc on public class.Per coding guidelines, public classes should have proper JavaDoc documentation explaining the purpose of this field trimmer and when it's used.
🔎 Suggested JavaDoc
+/** + * Custom field trimmer for OpenSearch that handles trimming fields for the {@link Dedup} + * relational expression. This trimmer ensures that dedup field references are correctly + * remapped when unused columns are removed from the input. + */ public class OpenSearchRelFieldTrimmer extends RelFieldTrimmer {core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java (1)
49-54: Minor: Comment shows hardcoded value instead of parameter.Line 52 in the comment shows
<=(_row_number_dedup_, 1)but the actual filter usesallowedDuplication(line 70). Update the comment to reflect the parameterized value.🔎 Suggested fix
/* * | dedup 2 a, b keepempty=true * LogicalProject(...) - * +- LogicalFilter(condition=[OR(IS NULL(a), IS NULL(b), <=(_row_number_dedup_, 1))]) + * +- LogicalFilter(condition=[OR(IS NULL(a), IS NULL(b), <=(_row_number_dedup_, n))]) * +- LogicalProject(..., _row_number_dedup_=[ROW_NUMBER() OVER (PARTITION BY a, b ORDER BY a, b)]) * +- ... */core/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java (1)
27-27: Empty JavaDoc should be completed or removed.Line 27 has an empty JavaDoc comment
/** */. Either provide meaningful documentation for the constructor or remove the empty comment.🔎 Suggested fix
- /** */ + /** + * Creates a Dedup relational expression. + * + * @param cluster the cluster + * @param traitSet the trait set + * @param input the input relational expression + * @param dedupeFields the fields to deduplicate on + * @param allowedDuplication the maximum number of duplicate rows to keep + * @param keepEmpty whether to keep rows with null dedup fields + * @param consecutive whether to deduplicate consecutive rows only + * @throws IllegalArgumentException if allowedDuplication is not positive + * @throws CalciteUnsupportedException if consecutive is true + */ protected Dedup(core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
18-18: Missing class-level JavaDoc.Per coding guidelines, public classes should have proper JavaDoc documentation. Consider adding documentation explaining this is the logical (NONE convention) implementation of the Dedup operator.
🔎 Suggested JavaDoc
+/** + * Logical dedup relational expression with {@link Convention#NONE}. + * <p> + * This operator represents a logical deduplication operation in the Calcite planning graph. + * It is converted to standard relational operators (ROW_NUMBER windowing) by + * {@link PPLDedupConvertRule}. + */ public class LogicalDedup extends Dedup {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (64)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.javainteg-test/src/test/resources/expectedOutput/calcite/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_false_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_true_not_pushed.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_text_type_no_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr3_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_criteria_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_fields_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_nested_agg_dedup_not_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output.yamlopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/AggregateIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/EnumerableTopKMergeRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/ExpandCollationOnProjectExprRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/FilterIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/InterruptibleRelRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/LimitIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/ProjectIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RareTopPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RelevanceFunctionPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortAggregateMeasureRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortProjectExprTransposeRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.java
💤 Files with no reviewable changes (1)
- core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
🧰 Additional context used
📓 Path-based instructions (7)
integ-test/src/test/resources/**/*
⚙️ CodeRabbit configuration file
integ-test/src/test/resources/**/*: - Verify test data is realistic and representative
- Check data format matches expected schema
- Ensure test data covers edge cases and boundary conditions
Files:
integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_nested_agg_dedup_not_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr3_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_fields_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_criteria_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_text_type_no_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_true_not_pushed.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_false_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3.yaml
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RelevanceFunctionPushdownRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/FilterIndexScanRule.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RareTopPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/ExpandCollationOnProjectExprRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/InterruptibleRelRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/ProjectIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortProjectExprTransposeRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/LimitIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortAggregateMeasureRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/EnumerableTopKMergeRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/AggregateIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RelevanceFunctionPushdownRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/FilterIndexScanRule.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RareTopPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/ExpandCollationOnProjectExprRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/InterruptibleRelRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/ProjectIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortProjectExprTransposeRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/LimitIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortAggregateMeasureRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/EnumerableTopKMergeRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/AggregateIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.java
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java: - Flag methods >50 lines - this file is known to be hard to read
- Suggest extracting complex logic into helper methods
- Check for code organization and logical grouping
- Validate all RelNode types are handled
Files:
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
🧠 Learnings (11)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_nested_agg_dedup_not_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr3_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex4.yamlcore/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_fields_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2.yamlcore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.javainteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_criteria_max_option.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex1.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_text_type_no_push.yamlopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_true_not_pushed.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1_alternative.yamlopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RareTopPushdownRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_push.yamlopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr1.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_complex2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr1.yamlcore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_false_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3.yamlcore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RelevanceFunctionPushdownRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RareTopPushdownRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortProjectExprTransposeRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/LimitIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/AggregateIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RelevanceFunctionPushdownRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_output.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_fields_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_criteria_max_option.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_true_not_pushed.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_push.yamlopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortIndexScanRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_false_push.yamlcore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.java
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/RelevanceFunctionPushdownRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javainteg-test/src/test/resources/expectedOutput/calcite/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javainteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_true_not_pushed.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlcore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortIndexScanRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortProjectExprTransposeRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/EnumerableTopKMergeRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/AggregateIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggGroupMergeRule.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.javacore/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
core/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
core/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Respect existing module boundaries in the OpenSearch SQL project structure
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Maintain Java 11 compatibility when possible for OpenSearch 2.x
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.javaopensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/ProjectIndexScanRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRuleConfig.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Update corresponding AST builder classes when making PPL grammar changes
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.javacore/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
🧬 Code graph analysis (12)
core/src/test/java/org/opensearch/sql/calcite/PPLAggGroupMergeRuleTest.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.java (1)
OpenSearchRules(12-23)
core/src/main/java/org/opensearch/sql/calcite/utils/SubsearchUtils.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.java (1)
LogicalSystemLimit(25-90)
core/src/test/java/org/opensearch/sql/calcite/PPLAggregateConvertRuleTest.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.java (1)
OpenSearchRules(12-23)
core/src/main/java/org/opensearch/sql/executor/QueryService.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.java (1)
LogicalSystemLimit(25-90)
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.java (1)
OpenSearchRules(12-23)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (2)
CalciteToolsHelper(106-437)OpenSearchRelBuilder(194-225)
core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.java (1)
LogicalSystemLimit(25-90)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java (2)
opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java (1)
Value(34-167)core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (3)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
CalcitePlanContext(31-209)core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.java (1)
OpenSearchRules(12-23)
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.java (1)
LogicalSystemLimit(25-90)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/OpenSearchTableScan.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.java (1)
OpenSearchRules(12-23)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/Dedup.java (1)
core/src/main/java/org/opensearch/sql/exception/CalciteUnsupportedException.java (1)
CalciteUnsupportedException(8-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: WhiteSource Security Check
Signed-off-by: Heng Qian <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java (1)
195-204: LGTM! Well-documented and consistent implementation.The method follows the established HepPlanner pattern (similar to
mergeAdjacentFilters) and includes clear JavaDoc explaining why this transformation step is necessary. The implementation correctly applies the PPL-specific conversion rule before SQL generation.Optional observation for future extensibility: If additional PPL-specific conversion rules are introduced later, consider whether the hardcoded rules list (line 200) should be parameterized or made into a class-level constant for easier maintenance. For the current single-rule requirement, the implementation is perfectly appropriate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java
💤 Files with no reviewable changes (1)
- core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
🧰 Additional context used
📓 Path-based instructions (6)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
🧠 Learnings (7)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.javacore/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : All public classes and methods must have proper JavaDoc
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Flag missing JavaDoc on public APIs during code review
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (3)
core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchRelFieldTrimmer.java (1)
38-69: LGTM on the field trimming implementation.The
trimFieldsmethod correctly follows Calcite's established patterns for field trimming:
- Uses
InputFinderto collect all fields referenced by the dedup expressions.- Recursively trims the child input.
- Short-circuits when no trimming is possible.
- Applies
RexPermuteInputsShuttleto remap field references according to the input mapping.This integrates well with the broader PR's
Dedupoperator support.ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java (2)
24-24: LGTM! Necessary imports for dedup conversion.The imports are correctly added to support the new PPL-specific transformation step that applies the dedup conversion rule before Spark SQL generation.
Also applies to: 49-49
189-189: LGTM! Critical transformation step for PPL logical operators.The change correctly routes all PPL RelNodes through the customized conversion step before Spark SQL generation. This ensures LogicalDedup and any future PPL-specific operators are properly transformed to their composite operator equivalents, as confirmed in the PR discussion.
Signed-off-by: Heng Qian <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
LantaoJin
previously approved these changes
Jan 9, 2026
Signed-off-by: Heng Qian <[email protected]> # Conflicts: # core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java # core/src/main/java/org/opensearch/sql/executor/QueryService.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
@core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java:
- Around line 30-52: The class Javadoc for PPLSimplifyDedupRule is misleading:
update the text to say the rule simplifies a composite pattern of
LogicalProject/LogicalFilter into a single LogicalDedup (i.e., composite ->
LogicalDedup) rather than converting a logical dedup into composites, and
correct the example to show keepEmpty=false (reflecting the emitted
LogicalDedup(..., keepEmpty=false, ...)); likewise update the other occurrences
referenced around the class (the example blocks near the other Javadoc mentions)
so they all describe the rule direction and the keepEmpty=false output
consistently.
- Around line 83-89: Replace the fragile assert and unchecked casts around
numOfDedupFilter.getCondition() with defensive checks: verify
getCondition().isA(SqlKind.LESS_THAN_OR_EQUAL) and that it is a RexCall
(instanceof RexCall) before casting, ensure the call's last operand is a
RexLiteral (instanceof RexLiteral) before casting, and check
literal.getValueAs(Integer.class) for null; if any check fails, throw a clear
IllegalStateException (or return a safe fallback) with a descriptive message
referencing numOfDedupFilter and the condition so failures are explicit. Apply
the same defensive pattern to the analogous code at the other location (lines
~199-203) to avoid NPE/ClassCastException and null Integer results.
In @core/src/main/java/org/opensearch/sql/executor/QueryService.java:
- Around line 323-342: Add an @param entry for the new CalcitePlanContext
parameter in the JavaDoc for convertToCalcitePlan: document the parameter name
"context" and briefly state that it provides the Calcite plan construction
context (e.g., relBuilder and sysLimit/querySizeLimit used when creating
LogicalSystemLimit and for any collation/rel traits). Ensure the JavaDoc block
above convertToCalcitePlan includes the @param context line consistent with
existing style.
🧹 Nitpick comments (3)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
447-452: Consider removing unusedcontextparameter or documenting future use.The
contextparameter is explicitly discarded viaUtil.discard(context). If it's reserved for future use (e.g., context-aware optimization), add a brief comment explaining this. Otherwise, consider removing it to avoid confusion.💡 Suggested documentation
- public static RelNode optimize(RelNode plan, CalcitePlanContext context) { - Util.discard(context); + public static RelNode optimize(RelNode plan, CalcitePlanContext context) { + // context reserved for future context-aware optimizations (e.g., pushdown hints) + Util.discard(context);core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
18-18: Consider adding JavaDoc for the public class.Per coding guidelines, public classes should have proper JavaDoc explaining the class purpose, particularly for a new logical operator that's part of the public planning API.
📝 Suggested JavaDoc
+/** + * Logical dedup operator representing deduplication at the planner level. + * + * <p>This operator is converted to an equivalent composition of Project, Filter, + * and Window (ROW_NUMBER) operators by {@link PPLDedupConvertRule}. + */ public class LogicalDedup extends Dedup {core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (1)
168-222: Tighten rule match to avoid dropping “real” filters; remove unused helper.
PlanUtils::mayBeFilterFromBucketNonNullneeds to guarantee the filter is exactly the bucket non-null filter; otherwise this rule effectively deletes an arbitrary filter and replaces it withkeepEmpty=false. AlsoisNullOrLessThanis currently unused; either wire it in or remove it.#!/bin/bash # Inspect PlanUtils filter predicate to ensure it matches ONLY the intended not-null filter. rg -n --type=java "mayBeFilterFromBucketNonNull|containsRowNumberDedup|getRexWindowFromProject" core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java -n || true
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javainteg-test/src/test/resources/expectedOutput/calcite/big5/composite_date_histogram_daily.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/composite_terms.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/composite_terms_keyword.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/date_histogram_minute_agg.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/dedup_metrics_size_field.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/keyword_in_range.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/multi_terms_keyword.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q11.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q12.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q13.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q14.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q15.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q22.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q23.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q28.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q31.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q32.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q37.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q38.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q39.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q41.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q42.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q43.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q8.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_true_not_pushed.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_text_type_no_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_date_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_timestamp_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_with_search.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_criteria_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_join_with_fields_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_nested_agg_dedup_not_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_dedup_keepempty_false_push.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_dedup_keepempty_true_not_pushed.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_dedup_push.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_date_string.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_timestamp_string.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_join_with_criteria_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_join_with_fields_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_output.yamlppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLJoinTest.java
✅ Files skipped from review due to trivial changes (6)
- integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_dedup_keepempty_false_push.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2_alternative.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_nested_agg_dedup_not_push.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr1.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_keepempty_true_not_pushed.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLJoinTest.java
- integ-test/src/test/resources/expectedOutput/calcite/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr2_alternative.yaml
- integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_dedup_keepempty_true_not_pushed.yaml
- integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_dedup_push.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex2_alternative.yaml
- core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
- integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_join_with_criteria_max_option.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex2.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_text_type_no_push.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_join_with_criteria_max_option.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3_alternative.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr3.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr2.yaml
- integ-test/src/test/resources/expectedOutput/calcite/explain_join_with_fields_max_option.yaml
- integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_join_with_fields_max_option.yaml
🧰 Additional context used
📓 Path-based instructions (5)
integ-test/src/test/resources/**/*
⚙️ CodeRabbit configuration file
integ-test/src/test/resources/**/*: - Verify test data is realistic and representative
- Check data format matches expected schema
- Ensure test data covers edge cases and boundary conditions
Files:
integ-test/src/test/resources/expectedOutput/calcite/big5/date_histogram_minute_agg.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q38.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_with_search.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/dedup_metrics_size_field.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_timestamp_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_date_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q11.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q42.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q22.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_timestamp_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/composite_terms.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q8.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_date_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/composite_terms_keyword.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q13.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q23.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/multi_terms_keyword.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q43.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/composite_date_histogram_daily.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q41.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q37.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q12.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q15.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q14.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q31.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q39.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q28.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_output.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/keyword_in_range.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q32.yaml
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java: - Flag methods >50 lines - this file is known to be hard to read
- Suggest extracting complex logic into helper methods
- Check for code organization and logical grouping
- Validate all RelNode types are handled
Files:
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
🧠 Learnings (6)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/big5/date_histogram_minute_agg.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q38.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_with_search.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/dedup_metrics_size_field.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_timestamp_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_date_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q11.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q42.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q22.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_with_expr4_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_timestamp_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/composite_terms.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q8.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_date_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/composite_terms_keyword.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q13.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q23.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/multi_terms_keyword.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q43.yamlcore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javainteg-test/src/test/resources/expectedOutput/calcite/big5/composite_date_histogram_daily.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q41.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q37.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q12.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q15.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q14.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q31.yamlcore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javainteg-test/src/test/resources/expectedOutput/calcite/clickbench/q39.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr1.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q28.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_output.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/keyword_in_range.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr4.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q32.yaml
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_expr_complex1_alternative.yamlinteg-test/src/test/resources/expectedOutput/calcite/big5/dedup_metrics_size_field.yamlcore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_date_string.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_complex_sort_expr_pushdown_for_smj_w_max_option.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_date_string.yamlcore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javainteg-test/src/test/resources/expectedOutput/calcite/clickbench/q31.yamlinteg-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_compare_time_string.yamlinteg-test/src/test/resources/expectedOutput/calcite/clickbench/q28.yaml
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/big5/composite_terms.yamlcore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.javacore/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
🧬 Code graph analysis (4)
core/src/main/java/org/opensearch/sql/executor/QueryService.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.java (1)
LogicalSystemLimit(25-90)
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.java (1)
LogicalSystemLimit(25-90)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/OpenSearchRules.java (1)
OpenSearchRules(12-23)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: WhiteSource Security Check
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Outdated
Show resolved
Hide resolved
yuancu
previously approved these changes
Jan 9, 2026
LantaoJin
previously approved these changes
Jan 9, 2026
Member
LantaoJin
left a comment
LantaoJin
left a comment
There was a problem hiding this comment.
Please resolve the AI reviews.
qianheng-aws
commented
Jan 9, 2026
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
Signed-off-by: Heng Qian <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java:
- Line 44: The JavaDoc for this class incorrectly names "PPLDedupConvertRule";
update the comment above the class PPLSimplifyDedupRule to reference the correct
class name (e.g., change "Creates a PPLDedupConvertRule." to "Creates a
PPLSimplifyDedupRule.") so the JavaDoc matches the actual class symbol
PPLSimplifyDedupRule.
- Around line 58-101: The apply method has Java 11 incompatibility and unsafe
casts: replace usage of RexList.getLast() by indexing into the operands list
(e.g., operands.get(operands.size() - 1)) and add explicit type and structure
checks before casting — verify numOfDedupFilter.getCondition() is a RexCall,
that its operator and operand count match the expected row_number comparison
pattern, and that the final operand is a RexLiteral; after extracting the
literal use literal.getValueAs(Integer.class) and validate it is non-null
(handle invalid/missing integer by returning early or throwing a documented
exception) before assigning to dedupNumber; finally add JavaDoc to the protected
apply method including @param entries for call, finalProject, numOfDedupFilter,
projectWithWindow, bucketNonNullFilter, an @throws for any runtime validation
exceptions you choose to throw, and an @return description (or mention void).
🧹 Nitpick comments (2)
core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java (1)
465-468: Consider consistency with codebase patterns for defensive checks.The implementation uses
getLast()without an empty list check. While defensive programming is valuable, this pattern is inconsistent with the existing codebase—similargetLast()calls appear throughout without empty checks (e.g.,CaseRangeAnalyzer,PPLAggregateConvertRule). In Calcite,RelNode.getRowType().getFieldNames()always returns a non-empty list for valid RelNodes, which is an established assumption in this codebase. The design is sound: the dedup column is always added to the projection's end viarelBuilder.projectPlus(), ensuring it exists and is positioned last when the method is called.If adding a defensive check aligns with your project's standards, the proposed pattern from
JsonExtractAllFunctionImpl(checking size before callinggetLast()) could be applied for consistency.core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (1)
139-156: Optional: Consider tracking unused method differently.The
isNullOrLessThanmethod is currently unused but documented for future keepEmpty=true support. Consider either:
- Removing it and re-adding when needed (keeps codebase cleaner)
- Adding a
@SuppressWarnings("unused")annotation if keeping for future referenceThis prevents static analysis tools from flagging it as dead code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javacore/src/main/java/org/opensearch/sql/executor/QueryService.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/main/java/org/opensearch/sql/executor/QueryService.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
core/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
🧬 Code graph analysis (2)
core/src/main/java/org/opensearch/sql/executor/QueryService.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalSystemLimit.java (1)
LogicalSystemLimit(25-90)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (56)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (21, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (5)
core/src/main/java/org/opensearch/sql/executor/QueryService.java (5)
35-36: LGTM!The new imports for
LogicalSystemLimitandSystemLimitTypeare correctly added to support the query size limit enforcement at the planning stage.
107-107: LGTM!The call site correctly passes the
CalcitePlanContexttoconvertToCalcitePlan, consistent with the updated method signature.
148-148: LGTM!The call site correctly passes the
CalcitePlanContexttoconvertToCalcitePlan, consistent with the updated method signature.
338-341: LGTM!The collation handling correctly uses
calcitePlan(the wrapped version) instead ofosPlan. The logic properly checks if the plan is not already aSortand has non-empty collation before adding aLogicalSort.
325-330: Verify that the Calcite optimizer handles or eliminates redundant limits when wrapping existing sorted plans.The
LogicalSystemLimitwrapper is unconditionally applied at the planning stage for all PPL queries. While the code comment at lines 331-336 indicates reliance on the Calcite optimizer to eliminate redundant sort operators, verify that this same optimization also applies to system limits. If a PPL plan is already a sorted result, wrapping it with another limit operator could create nested limits that need optimizer handling.Note: This change only affects PPL queries (not SQL pagination queries, which use a separate legacy execution path at lines 246-248).
Based on learnings, verify this Calcite integration change through SQL generation and optimization path testing.
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
Signed-off-by: Heng Qian <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java:
- Around line 465-469: The package-private utility method containsRowNumberDedup
currently assumes node and its field names are non-null and non-empty which can
throw IndexOutOfBoundsException; update containsRowNumberDedup to first check
for null node and null/empty fieldNames and return false if absent, then perform
the final-name comparison using ROW_NUMBER_COLUMN_FOR_DEDUP; also add a complete
JavaDoc for containsRowNumberDedup including a one-line description, @param node
explanation and @return description describing the boolean result.
🧹 Nitpick comments (4)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (4)
49-56: Add JavaDoc to public method.Per coding guidelines, all public methods must have proper JavaDoc. Even though this is an override, a brief description of what nodes are extracted would improve maintainability.
📝 Suggested JavaDoc addition
+ /** + * Matches and extracts the four-node pattern: finalProject -> numOfDedupFilter -> + * projectWithWindow -> bucketNonNullFilter. + */ @Override public void onMatch(RelOptRuleCall call) {As per coding guidelines, all public methods must have proper JavaDoc.
58-67: Add JavaDoc and consider logging for early returns.This protected method lacks JavaDoc documentation. Additionally, the early return at line 66 is silent, which could make debugging difficult when the rule doesn't match as expected.
📝 Suggested improvements
+ /** + * Applies the simplification by extracting dedup columns from the window, + * validating constraints, and constructing a LogicalDedup node. + * + * @param call the rule call context + * @param finalProject the final projection node + * @param numOfDedupFilter the filter checking row number + * @param projectWithWindow the projection containing ROW_NUMBER window function + * @param bucketNonNullFilter the filter ensuring dedup columns are non-null + */ protected void apply( RelOptRuleCall call, LogicalProject finalProject, LogicalFilter numOfDedupFilter, LogicalProject projectWithWindow, LogicalFilter bucketNonNullFilter) { List<RexWindow> windows = PlanUtils.getRexWindowFromProject(projectWithWindow); if (windows.size() != 1) { + // Rule doesn't match: expected exactly one window function return; }As per coding guidelines, all public and protected methods must have proper JavaDoc.
69-77: Validate dedupColumns is non-empty before filtering.The code filters MAP/ARRAY types from dedupColumns but doesn't check if dedupColumns is empty after filtering or before the check. An empty dedup column list would indicate an invalid pattern.
🔍 Suggested validation
List<RexNode> dedupColumns = windows.get(0).partitionKeys; + if (dedupColumns.isEmpty()) { + return; + } if (dedupColumns.stream()
139-156: Remove or defer unused helper method.The
isNullOrLessThanmethod is currently unused dead code. While the comment indicates it "should be useful if we can push down Dedup with keepEmpty=true in the future," keeping unused code increases maintenance burden and can confuse readers.Consider either:
- Removing this method and re-introducing it when keepEmpty=true support is implemented, OR
- Adding a
@SuppressWarnings("unused")annotation with a TODO comment referencing a specific issue/ticket for keepEmpty=true supportPer coding guidelines: "Check for dead code, unused imports, and unused variables."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
🧠 Learnings (4)
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
🧬 Code graph analysis (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (3)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (3)
42-47: LGTM!The class declaration and constructor follow standard Calcite rule patterns correctly.
103-132: LGTM!The Config interface follows the Immutables pattern correctly and the operand supplier properly defines the four-node pattern to match: LogicalProject -> LogicalFilter -> LogicalProject -> LogicalFilter, with appropriate predicates at each level.
159-161: LGTM!The static rule instance follows standard Calcite conventions for exposing rule instances.
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java:
- Around line 26-41: The Javadoc example in PPLSimplifyDedupRule is
inconsistent: it shows "dedup 2 a, b" but the generated filter uses
"<=_row_number_dedup_, 1)"; update the example so the condition matches the
declared allowed duplication—either change the leading command to "dedup 1 a, b"
or (preferably) change the filter condition to use "<=_row_number_dedup_, 2)" so
the LogicalFilter/ROW_NUMBER example and the resulting
LogicalDedup(dedupeFields=[a, b], allowedDuplication=2, keepempty=true) are
consistent; ensure you edit the Javadoc block in class PPLSimplifyDedupRule
accordingly.
- Around line 110-116: Replace usages of Stream.toList() (which is Java 16+)
with Java 11 compatible collection logic: collect(Collectors.toList()).
Specifically in PPLSimplifyDedupRule where targetProjections is built from
projectWithWindow.getNamedProjects().stream() and where you call
targetProjections.stream().map(...).toList() for relBuilder.project, change both
to use .collect(Collectors.toList()) and add the import
java.util.stream.Collectors; ensure you map Pair::getKey and Pair::getValue the
same way but collect into Lists before calling relBuilder.project.
🧹 Nitpick comments (2)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (2)
118-119: Consider adding a clarifying comment for hardcoded parameters.The
keepEmpty=falseandconsecutive=falseare hardcoded. While the predicatevalidDedupNumberCheckerensures only non-keepEmpty patterns match (by requiringLESS_THAN_OR_EQUALwithoutOR/IS_NULL), an inline comment would improve maintainability.💡 Suggested clarification
+ // keepEmpty=false and consecutive=false are inferred from the matched pattern: + // validDedupNumberChecker ensures the filter is a simple LESS_THAN_OR_EQUAL (no OR/IS_NULL) LogicalDedup dedup = LogicalDedup.create(relBuilder.build(), dedupColumns, dedupNumber, false, false);
162-179: Unused methodisNullOrLessThan- dead code.This method is defined but never invoked. The comment suggests it's for future
keepEmpty=truepushdown support, but unreferenced code increases maintenance burden.Consider either:
- Removing the method until it's needed.
- Adding a
@SuppressWarnings("unused")with a clear TODO referencing the tracking issue.♻️ Option 1: Remove dead code
- /** - * Check if the condition is null or less than. Should be useful if we can push down Dedup with - * keepEmpty=true in the future. - */ - private static boolean isNullOrLessThan(RexNode node) { - if (node.isA(SqlKind.LESS_THAN_OR_EQUAL)) return true; - if (!node.isA(SqlKind.OR)) return false; - boolean hasLessThan = false; - for (RexNode operand : ((RexCall) node).getOperands()) { - if (operand.isA(SqlKind.LESS_THAN_OR_EQUAL)) { - if (hasLessThan) return false; // only one less than - hasLessThan = true; - } else if (!operand.isA(SqlKind.IS_NULL)) { - return false; // only null if not less_than - } - } - return hasLessThan; // should be one less than - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
🧠 Learnings (4)
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
🧬 Code graph analysis (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (2)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (2)
42-56: LGTM!The class structure follows the standard Calcite
RelRulepattern correctly, with proper use of@Value.Enclosingfor Immutables integration and delegation to theapplymethod.
182-184: LGTM!The static rule field follows the standard Calcite pattern for exposing pre-configured rule instances.
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Heng Qian <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
@core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java:
- Around line 60-66: The block comment in PPLDedupConvertRule (example showing
transformation for "dedup 2 a, b keepempty=true") incorrectly uses
"<=(_row_number_dedup_, 1)"; update the example so the filter condition matches
the dedup count by using "<=(_row_number_dedup_, 2)" (or otherwise reflect the
provided dedup count) so the inline example in PPLDedupConvertRule.java is
consistent with the "dedup 2 ..." command.
- Around line 58-119: The two public helpers buildDedupOrNull and
buildDedupNotNull lack JavaDoc; add Javadoc blocks above each method including a
one-line summary and @param tags for relBuilder, dedupeFields, and
allowedDuplication, plus a short behavior note describing that the method
appends a ROW_NUMBER() window over partitioning by dedupeFields, applies
null-handling and row-number filtering (buildDedupOrNull uses OR(IS NULL(...))
to preserve empty groups; buildDedupNotNull filters IS NOT NULL on all keys
first), that they project a temporary _row_number_dedup_ column and then remove
it, and mention ordering/determinism expectations (the partition and ordering
used and that results depend on provided dedupeFields order).
- Around line 46-56: Fix NPE risk and validate allowedDuplication in onMatch:
use Boolean.TRUE.equals(dedup.getKeepEmpty()) instead of direct unboxing of
dedup.getKeepEmpty(), and validate dedup.getAllowedDuplication() before using it
(e.g., treat null as 0 or throw IllegalArgumentException for negative/null
values) so the subsequent literal(allowedDuplication) construction in
buildDedupOrNull/buildDedupNotNull is safe; update onMatch to check/normalize
allowedDuplication (via dedup.getAllowedDuplication()) and then call
buildDedupOrNull or buildDedupNotNull accordingly.
- Around line 58-85: The comment incorrectly documents an ORDER BY that isn't
implemented; update the implementation in both buildDedupOrNull and
buildDedupNotNull to include deterministic ordering by adding
.orderBy(dedupeFields) to the ROW_NUMBER window builder (i.e., the
relBuilder.aggregateCall(...).over().partitionBy(dedupeFields).orderBy(dedupeFields).rowsTo(...).as(ROW_NUMBER_COLUMN_FOR_DEDUP)),
and then update the method comments to show ROW_NUMBER() OVER (PARTITION BY a, b
ORDER BY a, b) so the code and documentation match.
- Around line 58-85: The methods buildDedupOrNull and buildDedupNotNull may call
relBuilder.or()/and() with an empty dedupeFields list; add explicit guards
checking dedupeFields.isEmpty() before building null/non-null conjuncts: in
buildDedupOrNull, if dedupeFields is empty, do not create the isNull OR chain
and only apply the row_number <= allowedDuplication predicate; in
buildDedupNotNull, if dedupeFields is empty, skip adding the non-null AND
predicate entirely and rely solely on row_number logic; update the uses of
relBuilder.or(...) and relBuilder.and(...) to only be invoked when dedupeFields
is non-empty (refer to symbols dedupeFields, relBuilder, _row_number_dedup_,
ROW_NUMBER_COLUMN_FOR_DEDUP, buildDedupOrNull and buildDedupNotNull).
In
@core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java:
- Around line 27-42: The JavaDoc for PPLSimplifyDedupRule is inaccurate: the
rule only matches a <= pattern via Config::validDedupNumberChecker and
constructs LogicalDedup with keepEmpty=false and consecutive=false, so update
the class JavaDoc and example to reflect the actual applicability (show the
<=(_row_number_dedup_, N) pattern and that it simplifies to
LogicalDedup(dedupeFields=[a, b], allowedDuplication=N, keepEmpty=false,
consecutive=false)); alternatively, if you intend to support the OR/IS NULL
keepempty=true case, extend the matcher in PPLSimplifyDedupRule (and
Config::validDedupNumberChecker) and adjust the LogicalDedup construction to set
keepEmpty=true when NULL branches are present—pick one approach and make the
JavaDoc and implementation consistent.
- Around line 89-108: The extraction of the dedup number in PPLSimplifyDedupRule
(inside the apply logic that inspects numOfDedupFilter.getCondition()) wrongly
assumes the literal is the last operand; change the logic to scan all operands
from ((RexCall) condition).getOperands() for a RexLiteral, verify it parses to
an Integer > 0 (instead of only checking the last operand), and use that value
as dedupNumber; if none found or the value is null/<=0, return as before. Ensure
you still handle non-RexCall conditions and empty operands exactly as currently
done.
🧹 Nitpick comments (3)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (3)
79-87: MAP/ARRAY guard is incomplete for non-INPUT_REF partition keys.If the intent is “no MAP/ARRAY dedupe keys”, consider checking
rex.getType().getSqlTypeName()for all partition keys (not onlyINPUT_REF).
68-125:apply()is doing several responsibilities; consider helper extraction.Optional, but splitting into (1) window + key validation, (2) dedup number extraction, (3) relBuilder rewrite would reduce cognitive load and keep method sizes closer to the guideline.
163-180:isNullOrLessThanis currently unused—either wire it in or remove to avoid dead logic.If this is intentional scaffolding, a short comment like “reserved for keepEmpty pushdown support” would help future readers (beyond the current comment block).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
🧠 Learnings (9)
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.javacore/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Maintain Java 11 compatibility when possible for OpenSearch 2.x
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : All new business logic requires unit tests
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Flag missing JavaDoc on public APIs during code review
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : All public classes and methods must have proper JavaDoc
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
🧬 Code graph analysis (2)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java (2)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java (1)
Value(38-137)core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rel/LogicalDedup.java (1)
LogicalDedup(18-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (21, integration)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (1)
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java (1)
83-85: VerifyRelBuilder.projectExcept(...)overload used here matches the Calcite version.These calls pass a
RexNodetoprojectExcept. Please confirm the CalciteRelBuilderAPI supports this overload in your pinned Calcite version; otherwise this won’t compile (or will drop the wrong field).Also applies to: 117-119
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLDedupConvertRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java
Show resolved
Hide resolved
yuancu
approved these changes
Jan 9, 2026
LantaoJin
approved these changes
Jan 9, 2026
Contributor
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-5014-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 22669da3f6e141e594e80915de9ad64c2ca1a054
# Push it to GitHub
git push --set-upstream origin backport/backport-5014-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Introduce logical dedup operators for PPL, this PR made this by:
Project-FIlter-ProjectWindow-FiltertoLogicalDedupDedupPushDownRuleby matchingDedup-Project-ScanpatternPPLDedupConvertRuleto convert aLogicalDedupoperator to an equivalent operator composition(project -> filter -> project(OVER))Additional change:
mergeAdjacedFilterto the place before prepareStatement. Add the new addedPPLSimplifyDedupRulein the HepPlanner as well.$fxto replace the original correct field name.Related Issues
Resolves #5013
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.