[Feature] implement transpose command as in the roadmap #4786 #5011
Conversation
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
asifabashar
requested review from
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
penghuo,
ps48,
seankao-az and
vamsimanohar
as code owners
asifabashar
requested review from
GumpacG,
LantaoJin,
RyanL1997,
noCharger,
qianheng-aws,
ykmr1224 and
yuancu
as code owners
📝 WalkthroughWalkthroughAdds a new end-to-end PPL Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser as PPL Parser
participant AstBuilder
participant Analyzer
participant Calcite as CalciteRelNodeVisitor
participant ES as OpenSearch Index
User->>Parser: "source=... | transpose [n] [column_name=...]"
Parser->>AstBuilder: parse transposeCommand
AstBuilder->>AstBuilder: ArgumentFactory.getArgumentList()
AstBuilder->>Analyzer: Transpose AST node
Analyzer->>Calcite: visitTranspose(node, context)
rect rgb(230,240,255)
note over Calcite: Phase 1 — Unpivot (derive row_id using ROW_NUMBER)
Calcite->>Calcite: unpivot fields -> (row_id, column, value)
end
rect rgb(230,255,235)
note over Calcite: Phase 2 — Pivot (validate maxRows, build CASE/MAX aggregations)
Calcite->>Calcite: pivot to columns: column, row1..rowN
Calcite->>ES: execute translated query
end
ES-->>Calcite: results
Calcite-->>User: transposed result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Comment |
asifabashar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (17)
ppl/src/main/antlr/OpenSearchPPLParser.g4 (1)
332-339: Grammar allows duplicate parameters.The
transposeParameter*allows multiple parameters of the same type. For example,transpose 5 10ortranspose column_name='a' column_name='b'would be syntactically valid. Per the relevant code snippet fromArgumentFactory.getArgumentList, the implementation uses aMap<String, Argument>which would silently overwrite duplicates. Consider whether this is the desired behavior or if you want to detect and reject duplicate parameters with a clear error message.core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (3)
10-10: Prefer explicit imports over wildcard import.The wildcard import
import lombok.*;could be replaced with explicit imports for better clarity and to avoid pulling in unused annotations.-import lombok.*; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NonNull; +import lombok.RequiredArgsConstructor; +import lombok.ToString;
14-22: Consider removing@Setterfor immutability.Per coding guidelines, AST nodes should be immutable where possible. The
@Setterannotation makes all fields mutable, but onlychildneeds to be set via theattach()method (which is already implemented). Removing@Setterwould make theargumentsfield effectively immutable after construction.@Getter -@Setter @ToString @EqualsAndHashCode(callSuper = false) @RequiredArgsConstructor public class Transpose extends UnresolvedPlan {
24-35: Silent exception handling without logging.The comment says "log warning and use default" but the catch block doesn't actually log anything. Either add proper logging or remove the misleading comment. Silent failures can make debugging difficult.
🔎 Proposed fix
public Integer getMaxRows() { - Integer maxRows = 5; + int maxRows = 5; if (arguments.containsKey("number")) { try { maxRows = Integer.parseInt(arguments.get("number").getValue().toString()); } catch (NumberFormatException e) { - // log warning and use default - maxRows = 5; + // Invalid number format, use default value of 5 } } return maxRows; }ppl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java (1)
250-255: Consider adding tests for edge cases.The test covers the case with both arguments provided. Consider adding tests for other variations to ensure robust coverage:
transpose(no arguments, uses defaults)transpose 10(only number provided)transpose column_name='col'(only column_name provided)🔎 Example additional tests
@Test public void testTransposeCommandDefaults() { assertEquals("source=table | transpose", anonymize("source=t | transpose")); } @Test public void testTransposeCommandWithOnlyNumber() { assertEquals("source=table | transpose 10", anonymize("source=t | transpose 10")); } @Test public void testTransposeCommandWithOnlyColumnName() { assertEquals( "source=table | transpose column_name=***", anonymize("source=t | transpose column_name='my_col'")); }core/src/main/java/org/opensearch/sql/analysis/Analyzer.java (1)
707-710: Verify exception message casing for consistency.The exception message uses
"Transpose"(capitalized). For consistency, verify whether Calcite exception messages in this codebase should be lowercase (as in"addtotals", line 529) or match the class name casing. Based on existing patterns, there's inconsistency across commands—consider standardizing.ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java (1)
640-659: Consider using constants for masked values.Two minor improvements for consistency:
Line 652: When appending
numberArg.getValue(), the code relies ontoString()of theLiteralobject. For explicitness and type safety, consider extracting the primitive value:anonymized.append(StringUtils.format(" %s", numberArg.getValue().getValue()));Line 656: The hardcoded
"***"should use theMASK_LITERALconstant (imported at line 11) for consistency with the rest of the codebase:anonymized.append(StringUtils.format(" %s=%s", "column_name", MASK_LITERAL));🔎 Proposed refactor
if (arguments.containsKey("number")) { Argument numberArg = arguments.get("number"); if (numberArg != null) { - anonymized.append(StringUtils.format(" %s", numberArg.getValue())); + anonymized.append(StringUtils.format(" %s", numberArg.getValue().getValue())); } } if (arguments.containsKey("columnName")) { - anonymized.append(StringUtils.format(" %s=***", "column_name")); + anonymized.append(StringUtils.format(" %s=%s", "column_name", MASK_LITERAL)); }integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java (1)
205-214: Consider asserting the Calcite success path intestTransposeCommand
testTransposeCommandonly exercisesverifyQueryon the error path (when aResponseExceptionis thrown), mirroring other tests in this class. That means when Calcite is enabled and the query succeeds, this test doesn’t assert anything about the result fortranspose.If you want this IT to validate both modes, consider:
- Capturing the successful
executeQueryresult when no exception is thrown, and- Calling
verifyQuery(result)in both branches (or asserting non-emptydatarowsdirectly whenisCalciteEnabled()).This would make the test more robust while staying consistent with
verifyQuerysemantics.ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java (1)
11-14: Add JavaDoc and tighten error messaging for transpose argumentsThe new transpose argument helper looks correct, but a couple of polish items would help:
Line 313:
getArgumentList(TransposeCommandContext ...)is public and lacks JavaDoc, whereas most other publicgetArgumentListoverloads in this class are documented. A brief comment describing supported keys (number,columnName) and expected types would improve clarity.Lines 319–322 & 329–332: The
IllegalArgumentExceptionmessages are a bit rough (“COLUMN_NAME requires a string literal value”, “must be a int limit, column_name , got %s”). Consider making them more precise and user-friendly, e.g.:
"transpose COLUMN_NAME parameter requires a string literal value""A transpose parameter must be either an integer row limit or COLUMN_NAME=<string>, got: %s"Functionally this is fine; these are just consistency/UX improvements. Based on learnings, keeping AST/argument helpers well-documented makes future grammar changes safer.
Also applies to: 313-335
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1)
32-33: Transpose planning logic is sound; consider removing dead code and tightening the pipelineThe new
visitTransposeimplementation correctly:
- Validates
maxRows > 0.- Adds a
ROW_NUMBER()-based__row_id__to identify source rows.- Unpivots all fields into
(columnName, value)rows with values cast toVARCHAR.- Re-pivots via
MAX(CASE WHEN __row_id__ = i THEN value END)to producerow 1,row 2, …,row Ncolumns grouped bycolumnName.Two small improvements:
Unused
__row_pos__window columnLines 769–778 compute
rowPos(__row_pos__) withROW_NUMBER()partitioned bycolumnName, but this column is never referenced afterwards. It’s also discarded implicitly by the final aggregate. You can drop this computation entirely:
- RexNode rowPos =
context.relBuilder.aggregateCall(SqlStdOperatorTable.ROW_NUMBER).over().partitionBy(context.relBuilder.field(columnName)).orderBy(context.relBuilder.field("__row_id__")).rowsTo(RexWindowBounds.CURRENT_ROW).as("__row_pos__");- context.relBuilder.projectPlus(rowPos);
2. **Optional: filter by `__row_id__ <= maxRows` before unpivot for efficiency** Currently rows with `__row_id__ > maxRows` are ignored logically (their CASE expressions always return `NULL`), but they still flow through unpivot and aggregation. To reduce work when `maxRows` is small relative to the result set, you could insert a filter immediately after adding `__row_id__`: ```java context.relBuilder.projectPlus(rowNumber); context.relBuilder.filter( context.relBuilder.lessThanOrEqual( context.relBuilder.field("__row_id__"), context.relBuilder.literal(maxRows)));This shouldn’t change semantics but will bound the amount of data processed by transpose.
Given the complexity of Calcite plans in this class, a brief JavaDoc or block comment on
visitTransposedescribing the two-phase UNPIVOT/PIVOT strategy would also help future readers. As per Calcite integration learnings, please re-run the existing transpose explain/result tests after any change to confirm plans still match expectations.Also applies to: 700-802
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.java (1)
18-57: Good transpose coverage; add missing logical-plan assertion intestTransposeWithLimitThe new Calcite PPL tests give solid coverage of:
- Simple aggregate + transpose.
- Multiple aggregates with aliases.
- Row-limit handling.
- Custom
column_name.One small gap:
- Lines 124–138:
testTransposeWithLimitconstructs anexpectedLogicalstring but never callsverifyLogical(root, expectedLogical)(unlike the other tests). As a result, this test only checks result rows and Spark SQL, not the Calcite logical plan shape.Consider adding:
verifyLogical(root, expectedLogical);just after
expectedLogicalis built to keep plan verification consistent across all transpose tests.Also applies to: 59-118, 120-165, 167-214
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.java (6)
18-24: Remove unused BANK index loading.Line 23 loads
Index.BANK, but none of the test methods use it. Remove this line to reduce unnecessary test setup overhead.🔎 Proposed fix
@Override public void init() throws Exception { super.init(); enableCalcite(); loadIndex(Index.ACCOUNT); - loadIndex(Index.BANK); }
27-30: Remove incomplete JavaDoc or make it complete.The JavaDoc is incomplete: it has
@throws IOExceptionbut no description of what the test does. Either remove the JavaDoc entirely (preferred for simple test methods) or add a proper description.🔎 Proposed fix
- /** - * default test without parameters on account index - * - * @throws IOException - */ @Test public void testTranspose() throws IOException {
87-88: Fix misleading comment.The comment "Should have original data plus one totals row" is misleading. The transpose command doesn't add a totals row; it transforms rows into columns. The comment should describe the transpose behavior instead.
🔎 Proposed fix
- // Should have original data plus one totals row - var dataRows = result.getJSONArray("datarows"); + // Get transposed data rows (fields become rows, original rows become columns) + var dataRows = result.getJSONArray("datarows");
160-161: Fix misleading comment.The comment "Should have original data plus one totals row" is misleading. The transpose command doesn't add a totals row; it transforms rows into columns.
🔎 Proposed fix
- // Should have original data plus one totals row - var dataRows = result.getJSONArray("datarows"); + // Get transposed data rows (fields become rows, original rows become columns) + var dataRows = result.getJSONArray("datarows");
189-190: Fix misleading comment.The comment "Should have original data plus one totals row" is misleading. The transpose command doesn't add a totals row; it transforms rows into columns.
🔎 Proposed fix
- // Should have original data plus one totals row - var dataRows = result.getJSONArray("datarows"); + // Get transposed data rows (fields become rows, original rows become columns) + var dataRows = result.getJSONArray("datarows");
31-198: Consider adding error condition tests.The current test suite covers happy path scenarios well (default behavior, various limits, custom column name). Consider adding tests for error conditions such as:
- Invalid transpose arguments (negative limits, invalid column names)
- Transpose on empty result set
- Transpose with very large limits
However, if these error conditions are tested elsewhere (e.g., in unit tests or other integration test files), this comment can be disregarded.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
core/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.javacore/src/main/java/org/opensearch/sql/ast/tree/Transpose.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javadocs/category.jsondocs/user/ppl/cmd/transpose.mddocs/user/ppl/index.mdinteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.javainteg-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/resources/expectedOutput/calcite/explain_transpose.yamlppl/src/main/antlr/OpenSearchPPLLexer.g4ppl/src/main/antlr/OpenSearchPPLParser.g4ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.javappl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java
🧰 Additional context used
📓 Path-based instructions (13)
**/*.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:
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.javappl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/ast/tree/Transpose.javacore/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.javappl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.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:
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javacore/src/main/java/org/opensearch/sql/analysis/Analyzer.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.javappl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/ast/tree/Transpose.javacore/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.javappl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.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:
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.javappl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.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:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.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/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.javappl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.javappl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java
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_transpose.yaml
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/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/ast/tree/Transpose.javacore/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.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/utils/PPLQueryDataAnonymizerTest.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.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
**/*.g4
⚙️ CodeRabbit configuration file
**/*.g4: - Check if modifying unrelated grammar files (scope creep)
- Verify grammar rule placement follows project patterns
- Question if new rule needed vs reusing existing rules
- Ensure changes are relevant to the PR's stated purpose
Files:
ppl/src/main/antlr/OpenSearchPPLParser.g4ppl/src/main/antlr/OpenSearchPPLLexer.g4
ppl/src/main/antlr/OpenSearchPPLParser.g4
⚙️ CodeRabbit configuration file
ppl/src/main/antlr/OpenSearchPPLParser.g4: - Identify code reuse opportunities with existing commands
- Validate command follows PPL naming and structure patterns
- Check if command should be alias vs separate implementation
Files:
ppl/src/main/antlr/OpenSearchPPLParser.g4
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.javacore/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java
🧠 Learnings (8)
📚 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:
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.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:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/resources/expectedOutput/calcite/explain_transpose.yamlcore/src/main/java/org/opensearch/sql/analysis/Analyzer.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.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:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.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_transpose.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: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
core/src/main/java/org/opensearch/sql/analysis/Analyzer.javacore/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.javacore/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.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:
integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTransposeTest.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:
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.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: For PPL command PRs, refer docs/dev/ppl-commands.md and verify the PR satisfies the checklist
Applied to files:
docs/category.json
🧬 Code graph analysis (5)
ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java (1)
legacy/src/main/java/org/opensearch/sql/legacy/utils/StringUtils.java (1)
StringUtils(17-117)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.java (2)
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (1)
PPLIntegTestCase(36-415)integ-test/src/test/java/org/opensearch/sql/sql/IdentifierIT.java (1)
Index(218-262)
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java (1)
ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java (1)
ArgumentFactory(41-449)
integ-test/src/test/java/org/opensearch/sql/ppl/NewAddedCommandsIT.java (1)
integ-test/src/test/java/org/opensearch/sql/legacy/TestUtils.java (1)
TestUtils(36-536)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (1)
core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java (1)
AbstractNodeVisitor(95-469)
🪛 LanguageTool
docs/user/ppl/cmd/transpose.md
[grammar] ~17-~17: Ensure spelling is correct
Context: ...ults This example shows transposing wihtout any parameters. It transforms 5 rows in...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~40-~40: Ensure spelling is correct
Context: ...-+-------+-------+ ``` ## Example 2: Tranpose results up to a provided number of rows...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~42-~42: Ensure spelling is correct
Context: ...rows. This example shows transposing wihtout any parameters. It transforms 4 rows in...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~65-~65: Ensure spelling is correct
Context: ...-+---------+-------+ ``` ## Example 2: Tranpose results up to a provided number of rows...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~67-~67: Ensure spelling is correct
Context: ...n name. This example shows transposing wihtout any parameters. It transforms 4 rows in...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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 (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-linux (25, integration)
- 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: 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: build-windows-macos (macos-14, 25, doc)
- 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, 25, 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, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, 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 (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (10)
docs/user/ppl/index.md (1)
83-83: Verify the "stable" status for a new command.The transpose command is marked as
stable (since 3.5), but most other recently introduced commands (e.g., rex, regex, spath, chart, streamstats) are marked asexperimentalin their initial release. Is this intentional, or should it follow the pattern of starting asexperimentaluntil the API stabilizes?ppl/src/main/antlr/OpenSearchPPLParser.g4 (1)
1671-1672: LGTM!Adding
TRANSPOSEandCOLUMN_NAMEtosearchableKeyWordallows these reserved words to be used as identifiers when needed, following the existing pattern for other command-related keywords.integ-test/src/test/resources/expectedOutput/calcite/explain_transpose.yaml (1)
1-20: LGTM!The expected explain output correctly captures both logical and physical plans for the transpose operation. The plan demonstrates the unpivot-pivot transformation strategy with appropriate CASE expressions, aggregations using MAX, and the LIMIT being pushed down to the index scan. The structure aligns with the CalciteRelNodeVisitor's
visitTransposeimplementation.integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java (1)
106-106: LGTM!The
CalciteTransposeCommandIT.classis correctly added to the test suite in alphabetical order, ensuring transpose tests run in both pushdown-enabled and pushdown-disabled configurations.ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java (1)
740-745: LGTM!The
visitTransposeCommandmethod correctly follows the established pattern of other command visitors, delegating argument parsing toArgumentFactoryand constructing the appropriate AST node. The implementation is concise and consistent with the codebase conventions. Based on learnings, this properly updates the corresponding AST builder classes when making PPL grammar changes.docs/category.json (1)
46-46: LGTM!The documentation reference is correctly added to the PPL CLI category list.
core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java (1)
286-288: LGTM!The visitor method follows the established pattern and correctly delegates to
visitChildrenfor AST traversal.integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (1)
2024-2034: Test follows existing Calcite explain patterns with proper plan validation.Verified that the expected output file
integ-test/src/test/resources/expectedOutput/calcite/explain_transpose.yamlexists and contains valid logical and physical plan definitions for the transpose operation with proper Calcite operators and execution nodes. Test correctly:
- Enables pushdown for Calcite-specific behavior
- Uses YAML comparison with ID ignoring for plan stability
- References valid test data (
opensearch-sql_test_index_account) with realistic query parameters- Follows test naming convention
ppl/src/main/antlr/OpenSearchPPLLexer.g4 (1)
49-49: Lexer additions for TRANSPOSE and COLUMN_NAME look consistentThe new
TRANSPOSEandCOLUMN_NAMEtokens follow the existing naming/style conventions and are placed in appropriate sections of the lexer. No issues from a grammar/lexing perspective.Also applies to: 74-75
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTransposeCommandIT.java (1)
53-57: The trailing spaces are intentional padding from the transpose command to align field names in columnar output—all field names are padded to 9 characters width ("firstname", "balance ", "age "). This is the expected behavior, confirmed by the source test data in accounts.json which contains unpadded field names. No action needed.
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterSearchIT.java
Show resolved
Hide resolved
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user/ppl/index.md (1)
14-18: Add language specification to fenced code block.The code fence on line 14 is missing a language specification. It should be
```pplinstead of```to comply with markdown linting rules (MD040) and maintain consistency with other code examples in the documentation.If the original
ppl ignorekeyword was removed intentionally (e.g., to enable code rendering), consider using```pplwith the language tag restored, or document the reason for the change.🔎 Proposed fix
for example, the following query retrieve firstname and lastname from accounts if age large than 18. -``` +```ppl source=accounts | where age > 18 | fields firstname, lastname -``` +```
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/user/ppl/index.md
🧰 Additional context used
🧠 Learnings (2)
📓 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: Verify AST generation for new PPL parser syntax
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
📚 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: For PPL command PRs, refer docs/dev/ppl-commands.md and verify the PR satisfies the checklist
Applied to files:
docs/user/ppl/index.md
🪛 markdownlint-cli2 (0.18.1)
docs/user/ppl/index.md
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: bwc-tests-full-restart (21)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (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, integration)
- 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: build-windows-macos (macos-14, 25, integration)
- 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, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- 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 (1)
docs/user/ppl/index.md (1)
81-83: Transpose command entry properly added to the commands table.The new
transposecommand row is correctly formatted and integrated into the command reference table. Version alignment with other recently-added commands (addtotals, addcoltotals at 3.5) is consistent.Please verify that the referenced documentation file
docs/user/ppl/cmd/transpose.mdexists and is properly linked in the documentation structure, and thatdocs/category.jsonhas been updated to include the new transpose page under the PPL CLI category (as mentioned in the PR summary).
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
|
@LantaoJin Please review. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/user/ppl/cmd/transpose.md (1)
1-92: Documentation issues already flagged in previous review.The typos and description inaccuracies on Lines 17, 40, 42, 65, and 67 have already been identified in the previous review comments (typos: "wihtout" → "without", "Tranpose" → "Transpose"; numbering: Example 3 mislabeled as Example 2; descriptions need updating to reflect actual parameters used).
Please address the suggestions from the previous review.
🧹 Nitpick comments (1)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (1)
24-24: Use UPPER_SNAKE_CASE for constant naming.The constant
max_limitshould be renamed toMAX_LIMITto follow Java naming conventions for constants.🔎 Proposed fix
- private static final int max_limit = 1000; + private static final int MAX_LIMIT = 1000;And update the reference on Line 36:
- if (maxRows > max_limit) { + if (maxRows > MAX_LIMIT) { throw new IllegalArgumentException( - StringUtils.format("Maximum limit to transpose is %s", max_limit)); + StringUtils.format("Maximum limit to transpose is %s", MAX_LIMIT)); }As per coding guidelines, use
UPPER_SNAKE_CASEfor constants.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.javadocs/user/ppl/cmd/transpose.mddocs/user/ppl/index.md
🧰 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/ast/tree/Transpose.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/ast/tree/Transpose.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/ast/tree/Transpose.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java
🧠 Learnings (2)
📓 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: Verify AST generation for new PPL parser syntax
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
📚 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: For PPL command PRs, refer docs/dev/ppl-commands.md and verify the PR satisfies the checklist
Applied to files:
docs/user/ppl/index.md
🧬 Code graph analysis (1)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (2)
core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java (1)
AbstractNodeVisitor(95-469)legacy/src/main/java/org/opensearch/sql/legacy/utils/StringUtils.java (1)
StringUtils(17-117)
🪛 LanguageTool
docs/user/ppl/cmd/transpose.md
[grammar] ~17-~17: Ensure spelling is correct
Context: ...results This example shows transposing wihtout any parameters. It transforms 5 rows in...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~40-~40: Ensure spelling is correct
Context: ...---+-------+-------+ ``` ## Example 2: Tranpose results up to a provided number of rows...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~42-~42: Ensure spelling is correct
Context: ...f rows. This example shows transposing wihtout any parameters. It transforms 4 rows in...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~65-~65: Ensure spelling is correct
Context: ...-+---------+-------+ ``` ## Example 2: Tranpose results up to a provided number of rows...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~67-~67: Ensure spelling is correct
Context: ...n name. This example shows transposing wihtout any parameters. It transforms 4 rows in...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (25, 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 (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- 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, integration)
- 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: 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: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (2)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (1)
51-65: LGTM!The
attach(),getChild(), andaccept()methods follow standard AST patterns correctly. The use ofImmutableListfor returning children is good practice for maintaining immutability.docs/user/ppl/index.md (1)
81-83: LGTM!The new command entries for
addtotals,addcoltotals, andtransposeare correctly formatted and consistent with existing entries. Version numbering (3.5), status (stable), and descriptions are appropriate.
Signed-off-by: Asif Bashar <asif.bashar@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (1)
26-41: Validate numeric bounds and avoid silent error handling.The null check on line 28 addresses the NPE concern from the previous review. However, several validation issues remain:
- Missing validation: No check for negative or zero values of
maxRows, which would be invalid.- Silent error handling: The
NumberFormatExceptioncatch (lines 31-34) silently falls back to the default, which may hide user input errors.- Misleading comment: Line 32 mentions "log warning" but no logging occurs.
Consider validating that
maxRows > 0after parsing and either logging the error or throwing a descriptiveIllegalArgumentExceptionwhen parsing fails, so users receive clear feedback about invalid input.🔎 Proposed fix
public Integer getMaxRows() { Integer maxRows = 5; if (arguments.containsKey("number") && arguments.get("number").getValue() != null) { try { maxRows = Integer.parseInt(arguments.get("number").getValue().toString()); + if (maxRows <= 0) { + throw new IllegalArgumentException("Transpose 'number' must be positive"); + } } catch (NumberFormatException e) { - // log warning and use default - maxRows = 5; + throw new IllegalArgumentException( + StringUtils.format("Invalid number format for transpose: %s", + arguments.get("number").getValue())); } } - if (maxRows > max_limit) { + if (maxRows > MAX_LIMIT) { throw new IllegalArgumentException( - StringUtils.format("Maximum limit to transpose is %s", max_limit)); + StringUtils.format("Maximum limit to transpose is %s", MAX_LIMIT)); } return maxRows; }
🧹 Nitpick comments (2)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (2)
24-24: Use UPPER_SNAKE_CASE for constant naming.The constant
max_limitshould follow Java naming conventions for constants.🔎 Proposed fix
- private static final int max_limit = 1000; + private static final int MAX_LIMIT = 1000;Update references in line 36 and 38:
- if (maxRows > max_limit) { + if (maxRows > MAX_LIMIT) { throw new IllegalArgumentException( - StringUtils.format("Maximum limit to transpose is %s", max_limit)); + StringUtils.format("Maximum limit to transpose is %s", MAX_LIMIT));As per coding guidelines, constants should use
UPPER_SNAKE_CASE.
22-22: Consider using ImmutableMap for defensive immutability.While the
argumentsreference is final, the map itself is mutable. For better immutability guarantees, consider wrapping it in anImmutableMapin the constructor.🔎 Example approach
public class Transpose extends UnresolvedPlan { private final @NonNull ImmutableMap<String, Argument> arguments; public Transpose(java.util.Map<String, Argument> arguments) { this.arguments = ImmutableMap.copyOf(arguments); } // ... rest of class }As per coding guidelines, AST nodes should be immutable where possible. However, if the current pattern is consistent with other
UnresolvedPlannodes, this is optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.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/ast/tree/Transpose.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/ast/tree/Transpose.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/ast/tree/Transpose.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java
🧠 Learnings (1)
📓 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: Verify AST generation for new PPL parser syntax
🧬 Code graph analysis (1)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (2)
core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java (1)
AbstractNodeVisitor(95-469)legacy/src/main/java/org/opensearch/sql/legacy/utils/StringUtils.java (1)
StringUtils(17-117)
⏰ 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 (25, integration)
- GitHub Check: build-linux (25, unit)
- 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: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- 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, 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: security-it-windows-macos (macos-14, 25)
- 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 (macos-14, 21, integration)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (5)
core/src/main/java/org/opensearch/sql/ast/tree/Transpose.java (5)
1-21: LGTM: Class structure follows AST node patterns.The class correctly extends
UnresolvedPlan, uses appropriate Lombok annotations, and follows the visitor pattern. Imports are necessary and well-organized.
43-49: LGTM: Null check addresses previous concern.The null check on line 45 properly addresses the NPE concern from the previous review. The method now safely handles null values.
51-55: LGTM: Follows standard AST pattern.The
attach()method correctly implements the standard pattern for attaching child nodes.
57-60: LGTM: Correctly uses ImmutableList.The method properly returns an immutable list and handles the null child case appropriately.
62-65: LGTM: Correctly implements visitor pattern.The method properly delegates to the visitor's
visitTransposemethod, following the established AST visitor pattern.
asifabashar
changed the title
2 participants
Description
The transpose command outputs the requested number of search rows as columns, effectively transposing each result row into a corresponding column of field values. (Part of roadmap [https://github.com//issues/4287])
transpose [int] [column_name=]
Required Arguments: None.
Optional Arguments:
column_name
Syntax: column_name=
Description: The name of the first column to use when transposing rows. This column holds the field names.
Default: column
int
Syntax:
Description: Limit the number of rows to transpose.
Default: 5
Usage Notes:
When you use the transpose command, the field names in the output depend on the arguments provided.
By default, the field names are:
column, row 1, row 2, ...
Examples
os> source=accounts | stats sum(age) by gender, state
When you add the transpose command to the end of the search, the results look something like this:
os> source=accounts | stats sum(age) by gender, state | transpose
Related Issues
Resolves #4786
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.