From 5e19bf3d17430aaee460c8d7a2614aaaedacec07 Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Mon, 11 Aug 2025 09:50:02 -0700 Subject: [PATCH 01/10] eval command support Signed-off-by: Kai Huang --- .../sql/calcite/CalciteRexNodeVisitor.java | 16 +++++++++++++++ .../sql/ppl/calcite/CalcitePPLEvalTest.java | 20 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index fec5d2dfa1d..200bc5bc16f 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -488,6 +488,22 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { } } + // Handle string concatenation with + operator + if ("+".equals(node.getFuncName()) && arguments.size() == 2) { + RexNode left = arguments.get(0); + RexNode right = arguments.get(1); + + // Check if both operands are strings + if (left.getType().getSqlTypeName() == SqlTypeName.VARCHAR + || left.getType().getSqlTypeName() == SqlTypeName.CHAR) { + if (right.getType().getSqlTypeName() == SqlTypeName.VARCHAR + || right.getType().getSqlTypeName() == SqlTypeName.CHAR) { + // Convert to CONCAT operation for string concatenation + return context.rexBuilder.makeCall(SqlStdOperatorTable.CONCAT, left, right); + } + } + } + RexNode resolvedNode = PPLFuncImpTable.INSTANCE.resolve( context.rexBuilder, node.getFuncName(), arguments.toArray(new RexNode[0])); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java index db759a7b9af..efcbd365834 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java @@ -354,4 +354,24 @@ public void testDependedLateralEval() { + "GROUP BY `DEPTNO`"; verifyPPLToSparkSQL(root, expectedSparkSql); } + + @Test + public void testEvalStringConcatenationWithPlus() { + String ppl = + "source=EMP | eval full_name = ENAME + ' ' + JOB | fields EMPNO, ENAME, JOB, full_name |" + + " head 3"; + RelNode root = getRelNode(ppl); + String expectedLogical = + "LogicalSort(fetch=[3])\n" + + " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], full_name=[||(||($1, ' ')," + + " $2)])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + + String expectedSparkSql = + "SELECT `EMPNO`, `ENAME`, `JOB`, `ENAME` || ' ' || `JOB` `full_name`\n" + + "FROM `scott`.`EMP`\n" + + "LIMIT 3"; + verifyPPLToSparkSQL(root, expectedSparkSql); + } } From e181038ec4d4365adf09b385d5f8b9ddd621ba8a Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Mon, 11 Aug 2025 15:22:33 -0700 Subject: [PATCH 02/10] improvment Signed-off-by: Kai Huang --- .../sql/calcite/CalciteRexNodeVisitor.java | 76 +++++++++++++++---- 1 file changed, 62 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index 200bc5bc16f..bc223cffc82 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -488,20 +488,10 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { } } - // Handle string concatenation with + operator - if ("+".equals(node.getFuncName()) && arguments.size() == 2) { - RexNode left = arguments.get(0); - RexNode right = arguments.get(1); - - // Check if both operands are strings - if (left.getType().getSqlTypeName() == SqlTypeName.VARCHAR - || left.getType().getSqlTypeName() == SqlTypeName.CHAR) { - if (right.getType().getSqlTypeName() == SqlTypeName.VARCHAR - || right.getType().getSqlTypeName() == SqlTypeName.CHAR) { - // Convert to CONCAT operation for string concatenation - return context.rexBuilder.makeCall(SqlStdOperatorTable.CONCAT, left, right); - } - } + // Try to handle special operator cases first + RexNode specialCase = tryHandleSpecialOperators(node, arguments, context); + if (specialCase != null) { + return specialCase; } RexNode resolvedNode = @@ -513,6 +503,64 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { throw new IllegalArgumentException("Unsupported operator: " + node.getFuncName()); } + /** + * Handles special operator cases that need custom logic before delegating to PPLFuncImpTable. + * This method uses a dispatch pattern to handle different operators cleanly. + * + * @param node the function node + * @param arguments the processed arguments + * @param context the plan context + * @return RexNode if handled, null otherwise + */ + private RexNode tryHandleSpecialOperators( + Function node, List arguments, CalcitePlanContext context) { + switch (node.getFuncName()) { + case "+": + return tryHandleStringConcatenation(arguments, context); + default: + return null; + } + } + + /** + * Handles string concatenation with the + operator by converting to CONCAT operation when both + * operands are string types. + * + * @param arguments the function arguments (should be exactly 2 for + operator) + * @param context the plan context + * @return RexNode with CONCAT operation if both operands are strings, null otherwise + */ + private RexNode tryHandleStringConcatenation( + List arguments, CalcitePlanContext context) { + if (arguments.size() != 2) { + return null; + } + + RexNode left = arguments.get(0); + RexNode right = arguments.get(1); + + // Check if both operands are string types (VARCHAR or CHAR) + boolean leftIsString = isStringType(left.getType().getSqlTypeName()); + boolean rightIsString = isStringType(right.getType().getSqlTypeName()); + + if (leftIsString && rightIsString) { + // Convert to CONCAT operation for string concatenation + return context.rexBuilder.makeCall(SqlStdOperatorTable.CONCAT, left, right); + } + + return null; + } + + /** + * Checks if the given SQL type name represents a string type. + * + * @param typeName the SQL type name to check + * @return true if the type is VARCHAR or CHAR, false otherwise + */ + private boolean isStringType(SqlTypeName typeName) { + return typeName == SqlTypeName.VARCHAR || typeName == SqlTypeName.CHAR; + } + @Override public RexNode visitWindowFunction(WindowFunction node, CalcitePlanContext context) { Function windowFunction = (Function) node.getFunction(); From e96073260f3533d33b5b006fb35a74d65511333c Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Tue, 12 Aug 2025 15:14:28 -0700 Subject: [PATCH 03/10] Refactor Signed-off-by: Kai Huang --- .../sql/calcite/CalciteRexNodeVisitor.java | 64 --------------- .../expression/function/PPLFuncImpTable.java | 40 +++++++++- .../expression/function/PPLTypeChecker.java | 67 ++++++++++++++++ .../sql/ppl/calcite/CalcitePPLEvalTest.java | 78 +++++++++++++++++++ 4 files changed, 183 insertions(+), 66 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index bc223cffc82..fec5d2dfa1d 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -488,12 +488,6 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { } } - // Try to handle special operator cases first - RexNode specialCase = tryHandleSpecialOperators(node, arguments, context); - if (specialCase != null) { - return specialCase; - } - RexNode resolvedNode = PPLFuncImpTable.INSTANCE.resolve( context.rexBuilder, node.getFuncName(), arguments.toArray(new RexNode[0])); @@ -503,64 +497,6 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { throw new IllegalArgumentException("Unsupported operator: " + node.getFuncName()); } - /** - * Handles special operator cases that need custom logic before delegating to PPLFuncImpTable. - * This method uses a dispatch pattern to handle different operators cleanly. - * - * @param node the function node - * @param arguments the processed arguments - * @param context the plan context - * @return RexNode if handled, null otherwise - */ - private RexNode tryHandleSpecialOperators( - Function node, List arguments, CalcitePlanContext context) { - switch (node.getFuncName()) { - case "+": - return tryHandleStringConcatenation(arguments, context); - default: - return null; - } - } - - /** - * Handles string concatenation with the + operator by converting to CONCAT operation when both - * operands are string types. - * - * @param arguments the function arguments (should be exactly 2 for + operator) - * @param context the plan context - * @return RexNode with CONCAT operation if both operands are strings, null otherwise - */ - private RexNode tryHandleStringConcatenation( - List arguments, CalcitePlanContext context) { - if (arguments.size() != 2) { - return null; - } - - RexNode left = arguments.get(0); - RexNode right = arguments.get(1); - - // Check if both operands are string types (VARCHAR or CHAR) - boolean leftIsString = isStringType(left.getType().getSqlTypeName()); - boolean rightIsString = isStringType(right.getType().getSqlTypeName()); - - if (leftIsString && rightIsString) { - // Convert to CONCAT operation for string concatenation - return context.rexBuilder.makeCall(SqlStdOperatorTable.CONCAT, left, right); - } - - return null; - } - - /** - * Checks if the given SQL type name represents a string type. - * - * @param typeName the SQL type name to check - * @return true if the type is VARCHAR or CHAR, false otherwise - */ - private boolean isStringType(SqlTypeName typeName) { - return typeName == SqlTypeName.VARCHAR || typeName == SqlTypeName.CHAR; - } - @Override public RexNode visitWindowFunction(WindowFunction node, CalcitePlanContext context) { Function windowFunction = (Function) node.getFunction(); diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java index 64f3e5a8f0e..f0aff829b1b 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java @@ -689,8 +689,36 @@ void populate() { registerOperator(AND, SqlStdOperatorTable.AND); registerOperator(OR, SqlStdOperatorTable.OR); registerOperator(NOT, SqlStdOperatorTable.NOT); - registerOperator(ADD, SqlStdOperatorTable.PLUS); - registerOperator(ADDFUNCTION, SqlStdOperatorTable.PLUS); + + // Register ADD with custom implementation that handles both numeric and string types + register( + ADD, + createFunctionImpWithTypeChecker( + (builder, arg1, arg2) -> { + boolean arg1IsString = isStringType(arg1.getType().getSqlTypeName()); + boolean arg2IsString = isStringType(arg2.getType().getSqlTypeName()); + + if (arg1IsString && arg2IsString) { + return builder.makeCall(SqlStdOperatorTable.CONCAT, arg1, arg2); + } else { + return builder.makeCall(SqlStdOperatorTable.PLUS, arg1, arg2); + } + }, + new PPLTypeChecker.PPLAddTypeChecker())); + register( + ADDFUNCTION, + createFunctionImpWithTypeChecker( + (builder, arg1, arg2) -> { + boolean arg1IsString = isStringType(arg1.getType().getSqlTypeName()); + boolean arg2IsString = isStringType(arg2.getType().getSqlTypeName()); + + if (arg1IsString && arg2IsString) { + return builder.makeCall(SqlStdOperatorTable.CONCAT, arg1, arg2); + } else { + return builder.makeCall(SqlStdOperatorTable.PLUS, arg1, arg2); + } + }, + new PPLTypeChecker.PPLAddTypeChecker())); registerOperator(SUBTRACT, SqlStdOperatorTable.MINUS); registerOperator(SUBTRACTFUNCTION, SqlStdOperatorTable.MINUS); registerOperator(MULTIPLY, SqlStdOperatorTable.MULTIPLY); @@ -1191,4 +1219,12 @@ void populate() { null); } } + + /** + * Helper method to check if a SqlTypeName represents a string type. Used by ADD operator to + * determine whether to perform string concatenation or numeric addition. + */ + private static boolean isStringType(SqlTypeName typeName) { + return typeName == SqlTypeName.VARCHAR || typeName == SqlTypeName.CHAR; + } } diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java index adc10e63b71..d10b9f374c8 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java @@ -601,4 +601,71 @@ private static String formatExprSignatures(List> signatures) { + "]") .collect(Collectors.joining(",")); } + + /** + * A custom type checker for the ADD operator that supports both numeric addition and string + * concatenation. + */ + class PPLAddTypeChecker implements PPLTypeChecker { + @Override + public boolean checkOperandTypes(List types) { + if (types.size() != 2) { + return false; + } + + RelDataType type1 = types.get(0); + RelDataType type2 = types.get(1); + + // Check if both are strings (for concatenation) + boolean type1IsString = isStringType(type1.getSqlTypeName()); + boolean type2IsString = isStringType(type2.getSqlTypeName()); + + if (type1IsString && type2IsString) { + return true; + } + + // Check if both are numeric (for arithmetic addition) + boolean type1IsNumeric = SqlTypeUtil.isNumeric(type1); + boolean type2IsNumeric = SqlTypeUtil.isNumeric(type2); + + return type1IsNumeric && type2IsNumeric; + } + + @Override + public String getAllowedSignatures() { + return "[INTEGER,INTEGER],[DOUBLE,DOUBLE],[FLOAT,FLOAT],[BIGINT,BIGINT]," + + "[INTEGER,DOUBLE],[DOUBLE,INTEGER],[INTEGER,FLOAT],[FLOAT,INTEGER]," + + "[DOUBLE,FLOAT],[FLOAT,DOUBLE],[INTEGER,BIGINT],[BIGINT,INTEGER]," + + "[DOUBLE,BIGINT],[BIGINT,DOUBLE],[FLOAT,BIGINT],[BIGINT,FLOAT]," + + "[STRING,STRING]"; + } + + @Override + public List> getParameterTypes() { + return List.of( + // Numeric signatures + List.of(ExprCoreType.INTEGER, ExprCoreType.INTEGER), + List.of(ExprCoreType.DOUBLE, ExprCoreType.DOUBLE), + List.of(ExprCoreType.FLOAT, ExprCoreType.FLOAT), + List.of(ExprCoreType.LONG, ExprCoreType.LONG), + List.of(ExprCoreType.INTEGER, ExprCoreType.DOUBLE), + List.of(ExprCoreType.DOUBLE, ExprCoreType.INTEGER), + List.of(ExprCoreType.INTEGER, ExprCoreType.FLOAT), + List.of(ExprCoreType.FLOAT, ExprCoreType.INTEGER), + List.of(ExprCoreType.DOUBLE, ExprCoreType.FLOAT), + List.of(ExprCoreType.FLOAT, ExprCoreType.DOUBLE), + List.of(ExprCoreType.INTEGER, ExprCoreType.LONG), + List.of(ExprCoreType.LONG, ExprCoreType.INTEGER), + List.of(ExprCoreType.DOUBLE, ExprCoreType.LONG), + List.of(ExprCoreType.LONG, ExprCoreType.DOUBLE), + List.of(ExprCoreType.FLOAT, ExprCoreType.LONG), + List.of(ExprCoreType.LONG, ExprCoreType.FLOAT), + // String concatenation + List.of(ExprCoreType.STRING, ExprCoreType.STRING)); + } + + private boolean isStringType(SqlTypeName typeName) { + return typeName == SqlTypeName.CHAR || typeName == SqlTypeName.VARCHAR; + } + } } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java index efcbd365834..04ac01b0c95 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java @@ -374,4 +374,82 @@ public void testEvalStringConcatenationWithPlus() { + "LIMIT 3"; verifyPPLToSparkSQL(root, expectedSparkSql); } + + @Test + public void testEvalStringConcatenationWithNullableField() { + // Test with COMM field which naturally contains nulls in the EMP table + // When COMM is null, concatenation should result in null per SQL standard + String ppl = + "source=EMP | eval " + + "comm_desc = 'Commission: ' + CAST(COMM AS STRING) " + + "| fields EMPNO, ENAME, COMM, comm_desc | head 5"; + RelNode root = getRelNode(ppl); + + String expectedLogical = + "LogicalSort(fetch=[5])\n" + + " LogicalProject(EMPNO=[$0], ENAME=[$1], COMM=[$6], " + + "comm_desc=[||('Commission: ':VARCHAR, SAFE_CAST($6))])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + + String expectedSparkSql = + "SELECT `EMPNO`, `ENAME`, `COMM`, " + + "'Commission: ' || SAFE_CAST(`COMM` AS STRING) `comm_desc`\n" + + "FROM `scott`.`EMP`\n" + + "LIMIT 5"; + verifyPPLToSparkSQL(root, expectedSparkSql); + } + + @Test + public void testEvalStringConcatenationChained() { + // Test chained concatenation + String ppl = + "source=EMP | eval " + + "description = ENAME + ' - ' + JOB + ' (Dept: ' + CAST(DEPTNO AS STRING) + ')' " + + "| where EMPNO IN (7369, 7499) | fields EMPNO, description"; + RelNode root = getRelNode(ppl); + + String expectedLogical = + "LogicalProject(EMPNO=[$0], description=[$8])\n" + + " LogicalFilter(condition=[SEARCH($0, Sarg[7369, 7499])])\n" + + " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4]," + + " SAL=[$5], COMM=[$6], DEPTNO=[$7], description=[||(||(||(||(||($1, ' - ':VARCHAR)," + + " $2), ' (Dept: ':VARCHAR), SAFE_CAST($7)), ')')])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + + String expectedSparkSql = + "SELECT `EMPNO`, `description`\n" + + "FROM (SELECT `EMPNO`, `ENAME`, `JOB`, `MGR`, `HIREDATE`, `SAL`, `COMM`, `DEPTNO`," + + " `ENAME` || ' - ' || `JOB` || ' (Dept: ' || SAFE_CAST(`DEPTNO` AS STRING) || ')'" + + " `description`\n" + + "FROM `scott`.`EMP`) `t`\n" + + "WHERE `EMPNO` IN (7369, 7499)"; + verifyPPLToSparkSQL(root, expectedSparkSql); + } + + @Test + public void testEvalStringConcatenationMultipleExpressions() { + // Test multiple concatenation expressions in a single eval command + String ppl = + "source=EMP | eval " + + "name_job = ENAME + ' - ' + JOB, " + + "name_dept = ENAME + ' (Dept ' + CAST(DEPTNO AS STRING) + ')' " + + "| fields EMPNO, name_job, name_dept | head 2"; + RelNode root = getRelNode(ppl); + + String expectedLogical = + "LogicalSort(fetch=[2])\n" + + " LogicalProject(EMPNO=[$0], name_job=[||(||($1, ' - ':VARCHAR), $2)], " + + "name_dept=[||(||(||($1, ' (Dept ':VARCHAR), SAFE_CAST($7)), ')')])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + + String expectedSparkSql = + "SELECT `EMPNO`, `ENAME` || ' - ' || `JOB` `name_job`, " + + "`ENAME` || ' (Dept ' || SAFE_CAST(`DEPTNO` AS STRING) || ')' `name_dept`\n" + + "FROM `scott`.`EMP`\n" + + "LIMIT 2"; + verifyPPLToSparkSQL(root, expectedSparkSql); + } } From 85259758443b1e45215d2ca843f1647089137e0f Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Tue, 12 Aug 2025 16:46:18 -0700 Subject: [PATCH 04/10] fix CI Signed-off-by: Kai Huang --- .../java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java index 04ac01b0c95..d56f0e4d9ab 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java @@ -388,7 +388,7 @@ public void testEvalStringConcatenationWithNullableField() { String expectedLogical = "LogicalSort(fetch=[5])\n" + " LogicalProject(EMPNO=[$0], ENAME=[$1], COMM=[$6], " - + "comm_desc=[||('Commission: ':VARCHAR, SAFE_CAST($6))])\n" + + "comm_desc=[||('Commission: ':VARCHAR, NUMBER_TO_STRING($6))])\n" + " LogicalTableScan(table=[[scott, EMP]])\n"; verifyLogical(root, expectedLogical); From 4246bdaaa231b3d02562459b98cc1d1a2245f5b0 Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Tue, 12 Aug 2025 17:49:43 -0700 Subject: [PATCH 05/10] fix CI Signed-off-by: Kai Huang --- .../java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java index d56f0e4d9ab..a76b9049965 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java @@ -394,7 +394,7 @@ public void testEvalStringConcatenationWithNullableField() { String expectedSparkSql = "SELECT `EMPNO`, `ENAME`, `COMM`, " - + "'Commission: ' || SAFE_CAST(`COMM` AS STRING) `comm_desc`\n" + + "'Commission: ' || NUMBER_TO_STRING(`COMM` AS STRING) `comm_desc`\n" + "FROM `scott`.`EMP`\n" + "LIMIT 5"; verifyPPLToSparkSQL(root, expectedSparkSql); From 49d2f207aca98caf351ae3b554b30dd0010f8b5c Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Tue, 12 Aug 2025 22:23:02 -0700 Subject: [PATCH 06/10] fix CI Signed-off-by: Kai Huang --- .../java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java index a76b9049965..2d58b3a0b2d 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java @@ -394,7 +394,7 @@ public void testEvalStringConcatenationWithNullableField() { String expectedSparkSql = "SELECT `EMPNO`, `ENAME`, `COMM`, " - + "'Commission: ' || NUMBER_TO_STRING(`COMM` AS STRING) `comm_desc`\n" + + "'Commission: ' || `NUMBER_TO_STRING`(`COMM`) `comm_desc`\n" + "FROM `scott`.`EMP`\n" + "LIMIT 5"; verifyPPLToSparkSQL(root, expectedSparkSql); From 1154ef5d765a38e8f2e9e96a77e50cf93c58dca9 Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Sun, 17 Aug 2025 22:03:54 -0700 Subject: [PATCH 07/10] fixes Signed-off-by: Kai Huang --- .../expression/function/PPLFuncImpTable.java | 42 +++--------- .../expression/function/PPLTypeChecker.java | 67 ------------------- 2 files changed, 10 insertions(+), 99 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java index f0aff829b1b..c44780fcfe2 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java @@ -690,35 +690,21 @@ void populate() { registerOperator(OR, SqlStdOperatorTable.OR); registerOperator(NOT, SqlStdOperatorTable.NOT); - // Register ADD with custom implementation that handles both numeric and string types + // Register ADD (+ symbol) for numeric addition register( ADD, - createFunctionImpWithTypeChecker( - (builder, arg1, arg2) -> { - boolean arg1IsString = isStringType(arg1.getType().getSqlTypeName()); - boolean arg2IsString = isStringType(arg2.getType().getSqlTypeName()); + (RexBuilder builder, RexNode... args) -> builder.makeCall(SqlStdOperatorTable.PLUS, args), + new PPLTypeChecker.PPLFamilyTypeChecker(SqlTypeFamily.NUMERIC, SqlTypeFamily.NUMERIC)); - if (arg1IsString && arg2IsString) { - return builder.makeCall(SqlStdOperatorTable.CONCAT, arg1, arg2); - } else { - return builder.makeCall(SqlStdOperatorTable.PLUS, arg1, arg2); - } - }, - new PPLTypeChecker.PPLAddTypeChecker())); + // Register ADD (+ symbol) for string concatenation register( - ADDFUNCTION, - createFunctionImpWithTypeChecker( - (builder, arg1, arg2) -> { - boolean arg1IsString = isStringType(arg1.getType().getSqlTypeName()); - boolean arg2IsString = isStringType(arg2.getType().getSqlTypeName()); + ADD, + (RexBuilder builder, RexNode... args) -> + builder.makeCall(SqlStdOperatorTable.CONCAT, args), + new PPLTypeChecker.PPLFamilyTypeChecker(SqlTypeFamily.STRING, SqlTypeFamily.STRING)); - if (arg1IsString && arg2IsString) { - return builder.makeCall(SqlStdOperatorTable.CONCAT, arg1, arg2); - } else { - return builder.makeCall(SqlStdOperatorTable.PLUS, arg1, arg2); - } - }, - new PPLTypeChecker.PPLAddTypeChecker())); + // Register ADDFUNCTION for numeric addition only + registerOperator(ADDFUNCTION, SqlStdOperatorTable.PLUS); registerOperator(SUBTRACT, SqlStdOperatorTable.MINUS); registerOperator(SUBTRACTFUNCTION, SqlStdOperatorTable.MINUS); registerOperator(MULTIPLY, SqlStdOperatorTable.MULTIPLY); @@ -1219,12 +1205,4 @@ void populate() { null); } } - - /** - * Helper method to check if a SqlTypeName represents a string type. Used by ADD operator to - * determine whether to perform string concatenation or numeric addition. - */ - private static boolean isStringType(SqlTypeName typeName) { - return typeName == SqlTypeName.VARCHAR || typeName == SqlTypeName.CHAR; - } } diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java index d10b9f374c8..adc10e63b71 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java @@ -601,71 +601,4 @@ private static String formatExprSignatures(List> signatures) { + "]") .collect(Collectors.joining(",")); } - - /** - * A custom type checker for the ADD operator that supports both numeric addition and string - * concatenation. - */ - class PPLAddTypeChecker implements PPLTypeChecker { - @Override - public boolean checkOperandTypes(List types) { - if (types.size() != 2) { - return false; - } - - RelDataType type1 = types.get(0); - RelDataType type2 = types.get(1); - - // Check if both are strings (for concatenation) - boolean type1IsString = isStringType(type1.getSqlTypeName()); - boolean type2IsString = isStringType(type2.getSqlTypeName()); - - if (type1IsString && type2IsString) { - return true; - } - - // Check if both are numeric (for arithmetic addition) - boolean type1IsNumeric = SqlTypeUtil.isNumeric(type1); - boolean type2IsNumeric = SqlTypeUtil.isNumeric(type2); - - return type1IsNumeric && type2IsNumeric; - } - - @Override - public String getAllowedSignatures() { - return "[INTEGER,INTEGER],[DOUBLE,DOUBLE],[FLOAT,FLOAT],[BIGINT,BIGINT]," - + "[INTEGER,DOUBLE],[DOUBLE,INTEGER],[INTEGER,FLOAT],[FLOAT,INTEGER]," - + "[DOUBLE,FLOAT],[FLOAT,DOUBLE],[INTEGER,BIGINT],[BIGINT,INTEGER]," - + "[DOUBLE,BIGINT],[BIGINT,DOUBLE],[FLOAT,BIGINT],[BIGINT,FLOAT]," - + "[STRING,STRING]"; - } - - @Override - public List> getParameterTypes() { - return List.of( - // Numeric signatures - List.of(ExprCoreType.INTEGER, ExprCoreType.INTEGER), - List.of(ExprCoreType.DOUBLE, ExprCoreType.DOUBLE), - List.of(ExprCoreType.FLOAT, ExprCoreType.FLOAT), - List.of(ExprCoreType.LONG, ExprCoreType.LONG), - List.of(ExprCoreType.INTEGER, ExprCoreType.DOUBLE), - List.of(ExprCoreType.DOUBLE, ExprCoreType.INTEGER), - List.of(ExprCoreType.INTEGER, ExprCoreType.FLOAT), - List.of(ExprCoreType.FLOAT, ExprCoreType.INTEGER), - List.of(ExprCoreType.DOUBLE, ExprCoreType.FLOAT), - List.of(ExprCoreType.FLOAT, ExprCoreType.DOUBLE), - List.of(ExprCoreType.INTEGER, ExprCoreType.LONG), - List.of(ExprCoreType.LONG, ExprCoreType.INTEGER), - List.of(ExprCoreType.DOUBLE, ExprCoreType.LONG), - List.of(ExprCoreType.LONG, ExprCoreType.DOUBLE), - List.of(ExprCoreType.FLOAT, ExprCoreType.LONG), - List.of(ExprCoreType.LONG, ExprCoreType.FLOAT), - // String concatenation - List.of(ExprCoreType.STRING, ExprCoreType.STRING)); - } - - private boolean isStringType(SqlTypeName typeName) { - return typeName == SqlTypeName.CHAR || typeName == SqlTypeName.VARCHAR; - } - } } From 2030260408dbabfd2a39a6269cfcc27e9ce26def Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Sun, 17 Aug 2025 23:00:09 -0700 Subject: [PATCH 08/10] fix Signed-off-by: Kai Huang --- .../opensearch/sql/expression/function/PPLFuncImpTable.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java index c44780fcfe2..adee1fba502 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java @@ -697,11 +697,7 @@ void populate() { new PPLTypeChecker.PPLFamilyTypeChecker(SqlTypeFamily.NUMERIC, SqlTypeFamily.NUMERIC)); // Register ADD (+ symbol) for string concatenation - register( - ADD, - (RexBuilder builder, RexNode... args) -> - builder.makeCall(SqlStdOperatorTable.CONCAT, args), - new PPLTypeChecker.PPLFamilyTypeChecker(SqlTypeFamily.STRING, SqlTypeFamily.STRING)); + registerOperator(ADD, SqlStdOperatorTable.CONCAT); // Register ADDFUNCTION for numeric addition only registerOperator(ADDFUNCTION, SqlStdOperatorTable.PLUS); From e6ab54d132320b4372eecb37d178c4fbf3708e9c Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Mon, 18 Aug 2025 20:52:05 -0700 Subject: [PATCH 09/10] Add IT Signed-off-by: Kai Huang --- .../calcite/remote/CalciteEvalCommandIT.java | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java new file mode 100644 index 00000000000..588a4a784f9 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java @@ -0,0 +1,108 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.calcite.remote; + +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; +import static org.opensearch.sql.util.MatcherUtils.rows; +import static org.opensearch.sql.util.MatcherUtils.schema; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; +import static org.opensearch.sql.util.MatcherUtils.verifySchema; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.jupiter.api.Test; +import org.opensearch.client.Request; +import org.opensearch.sql.ppl.PPLIntegTestCase; + +public class CalciteEvalCommandIT extends PPLIntegTestCase { + + @Override + public void init() throws Exception { + super.init(); + enableCalcite(); + + loadIndex(Index.BANK); + + // Create test data for string concatenation + Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true"); + request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}"); + client().performRequest(request1); + + Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true"); + request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}"); + client().performRequest(request2); + + Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true"); + request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}"); + client().performRequest(request3); + } + + @Test + public void testEvalStringConcatenation() throws IOException { + JSONObject result = executeQuery("source=test_eval | eval greeting = 'Hello ' + name"); + verifySchema( + result, + schema("name", "string"), + schema("title", "string"), + schema("age", "bigint"), + schema("greeting", "string")); + verifyDataRows( + result, + rows("Alice", "Engineer", 25, "Hello Alice"), + rows("Bob", "Manager", 30, "Hello Bob"), + rows("Charlie", "Analyst", null, "Hello Charlie")); + } + + @Test + public void testEvalStringConcatenationWithNullField() throws IOException { + JSONObject result = + executeQuery( + "source=test_eval | eval age_desc = 'Age: ' + CAST(age AS STRING) | fields name, age," + + " age_desc"); + verifySchema( + result, schema("name", "string"), schema("age", "bigint"), schema("age_desc", "string")); + verifyDataRows( + result, + rows("Alice", 25, "Age: 25"), + rows("Bob", 30, "Age: 30"), + rows("Charlie", null, null)); + } + + @Test + public void testEvalStringConcatenationWithLiterals() throws IOException { + JSONObject result = + executeQuery( + "source=test_eval | eval full_info = 'Name: ' + name + ', Title: ' + title | fields" + + " name, title, full_info"); + verifySchema( + result, schema("name", "string"), schema("title", "string"), schema("full_info", "string")); + verifyDataRows( + result, + rows("Alice", "Engineer", "Name: Alice, Title: Engineer"), + rows("Bob", "Manager", "Name: Bob, Title: Manager"), + rows("Charlie", "Analyst", "Name: Charlie, Title: Analyst")); + } + + @Test + public void testEvalStringConcatenationWithExistingData() throws IOException { + JSONObject result = + executeQuery( + String.format( + "source=%s | eval full_name = firstname + ' ' + lastname | head 3 | fields" + + " firstname, lastname, full_name", + TEST_INDEX_BANK)); + verifySchema( + result, + schema("firstname", "string"), + schema("lastname", "string"), + schema("full_name", "string")); + verifyDataRows( + result, + rows("Amber JOHnny", "Duke Willmington", "Amber JOHnny Duke Willmington"), + rows("Hattie", "Bond", "Hattie Bond"), + rows("Nanette", "Bates", "Nanette Bates")); + } +} From 46f715a1aa1d7135de5990565b617a51552eb4c7 Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Mon, 18 Aug 2025 23:11:47 -0700 Subject: [PATCH 10/10] remove redundant tests Signed-off-by: Kai Huang --- .../sql/ppl/calcite/CalcitePPLEvalTest.java | 98 ------------------- 1 file changed, 98 deletions(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java index 2d58b3a0b2d..db759a7b9af 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java @@ -354,102 +354,4 @@ public void testDependedLateralEval() { + "GROUP BY `DEPTNO`"; verifyPPLToSparkSQL(root, expectedSparkSql); } - - @Test - public void testEvalStringConcatenationWithPlus() { - String ppl = - "source=EMP | eval full_name = ENAME + ' ' + JOB | fields EMPNO, ENAME, JOB, full_name |" - + " head 3"; - RelNode root = getRelNode(ppl); - String expectedLogical = - "LogicalSort(fetch=[3])\n" - + " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], full_name=[||(||($1, ' ')," - + " $2)])\n" - + " LogicalTableScan(table=[[scott, EMP]])\n"; - verifyLogical(root, expectedLogical); - - String expectedSparkSql = - "SELECT `EMPNO`, `ENAME`, `JOB`, `ENAME` || ' ' || `JOB` `full_name`\n" - + "FROM `scott`.`EMP`\n" - + "LIMIT 3"; - verifyPPLToSparkSQL(root, expectedSparkSql); - } - - @Test - public void testEvalStringConcatenationWithNullableField() { - // Test with COMM field which naturally contains nulls in the EMP table - // When COMM is null, concatenation should result in null per SQL standard - String ppl = - "source=EMP | eval " - + "comm_desc = 'Commission: ' + CAST(COMM AS STRING) " - + "| fields EMPNO, ENAME, COMM, comm_desc | head 5"; - RelNode root = getRelNode(ppl); - - String expectedLogical = - "LogicalSort(fetch=[5])\n" - + " LogicalProject(EMPNO=[$0], ENAME=[$1], COMM=[$6], " - + "comm_desc=[||('Commission: ':VARCHAR, NUMBER_TO_STRING($6))])\n" - + " LogicalTableScan(table=[[scott, EMP]])\n"; - verifyLogical(root, expectedLogical); - - String expectedSparkSql = - "SELECT `EMPNO`, `ENAME`, `COMM`, " - + "'Commission: ' || `NUMBER_TO_STRING`(`COMM`) `comm_desc`\n" - + "FROM `scott`.`EMP`\n" - + "LIMIT 5"; - verifyPPLToSparkSQL(root, expectedSparkSql); - } - - @Test - public void testEvalStringConcatenationChained() { - // Test chained concatenation - String ppl = - "source=EMP | eval " - + "description = ENAME + ' - ' + JOB + ' (Dept: ' + CAST(DEPTNO AS STRING) + ')' " - + "| where EMPNO IN (7369, 7499) | fields EMPNO, description"; - RelNode root = getRelNode(ppl); - - String expectedLogical = - "LogicalProject(EMPNO=[$0], description=[$8])\n" - + " LogicalFilter(condition=[SEARCH($0, Sarg[7369, 7499])])\n" - + " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4]," - + " SAL=[$5], COMM=[$6], DEPTNO=[$7], description=[||(||(||(||(||($1, ' - ':VARCHAR)," - + " $2), ' (Dept: ':VARCHAR), SAFE_CAST($7)), ')')])\n" - + " LogicalTableScan(table=[[scott, EMP]])\n"; - verifyLogical(root, expectedLogical); - - String expectedSparkSql = - "SELECT `EMPNO`, `description`\n" - + "FROM (SELECT `EMPNO`, `ENAME`, `JOB`, `MGR`, `HIREDATE`, `SAL`, `COMM`, `DEPTNO`," - + " `ENAME` || ' - ' || `JOB` || ' (Dept: ' || SAFE_CAST(`DEPTNO` AS STRING) || ')'" - + " `description`\n" - + "FROM `scott`.`EMP`) `t`\n" - + "WHERE `EMPNO` IN (7369, 7499)"; - verifyPPLToSparkSQL(root, expectedSparkSql); - } - - @Test - public void testEvalStringConcatenationMultipleExpressions() { - // Test multiple concatenation expressions in a single eval command - String ppl = - "source=EMP | eval " - + "name_job = ENAME + ' - ' + JOB, " - + "name_dept = ENAME + ' (Dept ' + CAST(DEPTNO AS STRING) + ')' " - + "| fields EMPNO, name_job, name_dept | head 2"; - RelNode root = getRelNode(ppl); - - String expectedLogical = - "LogicalSort(fetch=[2])\n" - + " LogicalProject(EMPNO=[$0], name_job=[||(||($1, ' - ':VARCHAR), $2)], " - + "name_dept=[||(||(||($1, ' (Dept ':VARCHAR), SAFE_CAST($7)), ')')])\n" - + " LogicalTableScan(table=[[scott, EMP]])\n"; - verifyLogical(root, expectedLogical); - - String expectedSparkSql = - "SELECT `EMPNO`, `ENAME` || ' - ' || `JOB` `name_job`, " - + "`ENAME` || ' (Dept ' || SAFE_CAST(`DEPTNO` AS STRING) || ')' `name_dept`\n" - + "FROM `scott`.`EMP`\n" - + "LIMIT 2"; - verifyPPLToSparkSQL(root, expectedSparkSql); - } }