Skip to content

Commit 2c24770

Browse files
aaarone90aaarone90
committed
Making code more readable and removing unnecessary logic
Signed-off-by: Aaron Alvarez <aaarone@amazon.com>
1 parent dfe6957 commit 2c24770

20 files changed

Lines changed: 73 additions & 383 deletions

File tree

‎core/src/main/java/org/opensearch/sql/analysis/Analyzer.java‎

Lines changed: 3 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
import org.opensearch.sql.ast.tree.Bin;
6767
import org.opensearch.sql.ast.tree.Chart;
6868
import org.opensearch.sql.ast.tree.CloseCursor;
69+
import org.opensearch.sql.ast.tree.Convert;
6970
import org.opensearch.sql.ast.tree.Dedupe;
7071
import org.opensearch.sql.ast.tree.Eval;
7172
import org.opensearch.sql.ast.tree.Expand;
@@ -523,46 +524,9 @@ public LogicalPlan visitEval(Eval node, AnalysisContext context) {
523524
return new LogicalEval(child, expressionsBuilder.build());
524525
}
525526

526-
/** Build {@link org.opensearch.sql.planner.logical.LogicalConvert}. */
527527
@Override
528-
public LogicalPlan visitConvert(
529-
org.opensearch.sql.ast.tree.Convert node, AnalysisContext context) {
530-
LogicalPlan child = node.getChild().get(0).accept(this, context);
531-
ImmutableList.Builder<Pair<ReferenceExpression, Expression>> conversionsBuilder =
532-
new Builder<>();
533-
534-
for (org.opensearch.sql.ast.tree.ConvertFunction convertFunc : node.getConvertFunctions()) {
535-
String functionName = convertFunc.getFunctionName();
536-
List<String> fieldList = convertFunc.getFieldList();
537-
String asField = convertFunc.getAsField();
538-
539-
// Process each field in the conversion function
540-
for (String fieldName : fieldList) {
541-
// Analyze the field reference
542-
Expression fieldExpr = expressionAnalyzer.analyze(AstDSL.field(fieldName), context);
543-
544-
// Build the conversion function call
545-
// For now, we'll create a simple function call - this will be expanded later
546-
// to properly map conversion function names to actual implementations
547-
Expression conversionExpr =
548-
expressionAnalyzer.analyze(
549-
new Function(functionName, Collections.singletonList(AstDSL.field(fieldName))),
550-
context);
551-
552-
// Determine the target field name
553-
String targetFieldName = (asField != null) ? asField : fieldName;
554-
ReferenceExpression ref = DSL.ref(targetFieldName, conversionExpr.type());
555-
556-
conversionsBuilder.add(ImmutablePair.of(ref, conversionExpr));
557-
558-
// Define the new reference in type environment
559-
TypeEnvironment typeEnvironment = context.peek();
560-
typeEnvironment.define(ref);
561-
}
562-
}
563-
564-
return new org.opensearch.sql.planner.logical.LogicalConvert(
565-
child, conversionsBuilder.build(), node.getTimeformat());
528+
public LogicalPlan visitConvert(Convert node, AnalysisContext context) {
529+
throw getOnlyForCalciteException("convert");
566530
}
567531

568532
@Override

‎core/src/main/java/org/opensearch/sql/ast/tree/Convert.java‎

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,26 +14,15 @@
1414
import lombok.ToString;
1515
import org.opensearch.sql.ast.AbstractNodeVisitor;
1616

17-
/**
18-
* AST node representing the Convert command.
19-
*
20-
* <p>Syntax: convert [timeformat="format"] function(fields) [AS alias], ...
21-
*
22-
* <p>Example: convert auto(age), num(price) AS numeric_price
23-
*/
17+
/** AST node representing the Convert command. */
2418
@Getter
2519
@Setter
2620
@ToString
2721
@EqualsAndHashCode(callSuper = false)
2822
@RequiredArgsConstructor
2923
public class Convert extends UnresolvedPlan {
30-
/** Reserved for future time conversion functions (ctime, mktime, mstime). */
3124
private final String timeformat;
32-
33-
/** List of conversion functions to apply. */
3425
private final List<ConvertFunction> convertFunctions;
35-
36-
/** Child plan node. */
3726
private UnresolvedPlan child;
3827

3928
@Override

‎core/src/main/java/org/opensearch/sql/ast/tree/ConvertFunction.java‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,22 +11,13 @@
1111
import lombok.RequiredArgsConstructor;
1212
import lombok.ToString;
1313

14-
/**
15-
* Represents a single conversion function within a convert command.
16-
*
17-
* <p>Example: auto(field1, field2) AS converted_field
18-
*/
14+
/** Represents a single conversion function within a convert command. */
1915
@Getter
2016
@ToString
2117
@EqualsAndHashCode
2218
@RequiredArgsConstructor
2319
public class ConvertFunction {
24-
/** The name of the conversion function (e.g., "auto", "num", "ctime"). */
2520
private final String functionName;
26-
27-
/** The list of field names or patterns to convert. */
2821
private final List<String> fieldList;
29-
30-
/** Optional alias for the converted field (AS clause). Null if not specified. */
3122
private final String asField;
3223
}

‎core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java‎

Lines changed: 37 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@
108108
import org.opensearch.sql.ast.tree.Bin;
109109
import org.opensearch.sql.ast.tree.Chart;
110110
import org.opensearch.sql.ast.tree.CloseCursor;
111+
import org.opensearch.sql.ast.tree.Convert;
112+
import org.opensearch.sql.ast.tree.ConvertFunction;
111113
import org.opensearch.sql.ast.tree.Dedupe;
112114
import org.opensearch.sql.ast.tree.Eval;
113115
import org.opensearch.sql.ast.tree.Expand;
@@ -891,54 +893,56 @@ public RelNode visitEval(Eval node, CalcitePlanContext context) {
891893
}
892894

893895
@Override
894-
public RelNode visitConvert(
895-
org.opensearch.sql.ast.tree.Convert node, CalcitePlanContext context) {
896+
public RelNode visitConvert(Convert node, CalcitePlanContext context) {
896897
visitChildren(node, context);
897898

898-
// Build maps to track conversions
899-
java.util.Map<String, RexNode> replacements =
900-
new java.util.HashMap<>(); // field -> converted (no alias)
901-
List<Pair<String, RexNode>> additions = new ArrayList<>(); // new fields to add (with alias)
899+
if (node.getConvertFunctions() == null || node.getConvertFunctions().isEmpty()) {
900+
return context.relBuilder.peek();
901+
}
902902

903-
for (org.opensearch.sql.ast.tree.ConvertFunction convertFunc : node.getConvertFunctions()) {
904-
String functionName = convertFunc.getFunctionName();
905-
List<String> fieldList = convertFunc.getFieldList();
906-
String asField = convertFunc.getAsField();
903+
java.util.Map<String, RexNode> replacements = new java.util.HashMap<>();
904+
List<Pair<String, RexNode>> additions = new ArrayList<>();
905+
906+
for (ConvertFunction convertFunc : node.getConvertFunctions()) {
907+
processConversionFunction(convertFunc, replacements, additions, context);
908+
}
907909

908-
// Process each field in the field list
909-
for (String fieldName : fieldList) {
910-
RexNode field = context.relBuilder.field(fieldName);
910+
return buildProjectionWithConversions(replacements, additions, context);
911+
}
911912

912-
// Create the conversion function call
913-
RexNode convertCall =
914-
PPLFuncImpTable.INSTANCE.resolve(context.rexBuilder, functionName, field);
913+
private void processConversionFunction(
914+
ConvertFunction convertFunc,
915+
java.util.Map<String, RexNode> replacements,
916+
List<Pair<String, RexNode>> additions,
917+
CalcitePlanContext context) {
918+
String functionName = convertFunc.getFunctionName();
919+
List<String> fieldList = convertFunc.getFieldList();
920+
String asField = convertFunc.getAsField();
915921

916-
if (asField != null) {
917-
// With alias: add as new field at the end
918-
additions.add(Pair.of(asField, context.relBuilder.alias(convertCall, asField)));
919-
} else {
920-
// Without alias: replace original field in-place
921-
replacements.put(fieldName, context.relBuilder.alias(convertCall, fieldName));
922-
}
922+
for (String fieldName : fieldList) {
923+
RexNode field = context.relBuilder.field(fieldName);
924+
RexNode convertCall =
925+
PPLFuncImpTable.INSTANCE.resolve(context.rexBuilder, functionName, field);
926+
927+
if (asField != null) {
928+
additions.add(Pair.of(asField, context.relBuilder.alias(convertCall, asField)));
929+
} else {
930+
replacements.put(fieldName, context.relBuilder.alias(convertCall, fieldName));
923931
}
924932
}
933+
}
925934

926-
// Build projection maintaining original field order, then add new fields
935+
private RelNode buildProjectionWithConversions(
936+
java.util.Map<String, RexNode> replacements,
937+
List<Pair<String, RexNode>> additions,
938+
CalcitePlanContext context) {
927939
List<String> originalFields = context.relBuilder.peek().getRowType().getFieldNames();
928940
List<RexNode> projectList = new ArrayList<>();
929941

930-
// First, project all original fields (with replacements where applicable)
931942
for (String fieldName : originalFields) {
932-
if (replacements.containsKey(fieldName)) {
933-
// Use the converted expression for this field
934-
projectList.add(replacements.get(fieldName));
935-
} else {
936-
// Keep the original field
937-
projectList.add(context.relBuilder.field(fieldName));
938-
}
943+
projectList.add(replacements.getOrDefault(fieldName, context.relBuilder.field(fieldName)));
939944
}
940945

941-
// Then add new aliased fields at the end
942946
for (Pair<String, RexNode> addition : additions) {
943947
projectList.add(addition.getRight());
944948
}

‎core/src/main/java/org/opensearch/sql/expression/function/udf/AutoConvertFunction.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,7 @@
1818
import org.opensearch.sql.expression.function.ImplementorUDF;
1919
import org.opensearch.sql.expression.function.UDFOperandMetadata;
2020

21-
/**
22-
* PPL auto() conversion function. Automatically converts string values to numbers using best-fit
23-
* heuristics.
24-
*/
21+
/** PPL auto() conversion function. */
2522
public class AutoConvertFunction extends ImplementorUDF {
2623

2724
public AutoConvertFunction() {
@@ -47,7 +44,8 @@ public static class AutoConvertImplementor implements NotNullImplementor {
4744
public Expression implement(
4845
RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) {
4946
Expression fieldValue = translatedOperands.get(0);
50-
Expression result = Expressions.call(ConversionUtils.class, "autoConvert", fieldValue);
47+
Expression result =
48+
Expressions.call(ConversionUtils.class, "autoConvert", Expressions.box(fieldValue));
5149
return Expressions.convert_(result, Number.class);
5250
}
5351
}

‎core/src/main/java/org/opensearch/sql/expression/function/udf/NoneConvertFunction.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
import org.opensearch.sql.expression.function.ImplementorUDF;
1818
import org.opensearch.sql.expression.function.UDFOperandMetadata;
1919

20-
/** PPL none() conversion function. Passthrough function that returns the input value unchanged. */
20+
/** PPL none() conversion function. */
2121
public class NoneConvertFunction extends ImplementorUDF {
2222

2323
public NoneConvertFunction() {
@@ -38,7 +38,6 @@ public static class NoneConvertImplementor implements NotNullImplementor {
3838
@Override
3939
public Expression implement(
4040
RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) {
41-
// Simply return the input unchanged
4241
return translatedOperands.get(0);
4342
}
4443
}

‎core/src/main/java/org/opensearch/sql/expression/function/udf/NumConvertFunction.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,7 @@
1818
import org.opensearch.sql.expression.function.ImplementorUDF;
1919
import org.opensearch.sql.expression.function.UDFOperandMetadata;
2020

21-
/**
22-
* PPL num() conversion function. Converts string values to numbers using base 10, returning null on
23-
* failure.
24-
*/
21+
/** PPL num() conversion function. */
2522
public class NumConvertFunction extends ImplementorUDF {
2623

2724
public NumConvertFunction() {
@@ -47,7 +44,8 @@ public static class NumConvertImplementor implements NotNullImplementor {
4744
public Expression implement(
4845
RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) {
4946
Expression fieldValue = translatedOperands.get(0);
50-
Expression result = Expressions.call(ConversionUtils.class, "numConvert", fieldValue);
47+
Expression result =
48+
Expressions.call(ConversionUtils.class, "numConvert", Expressions.box(fieldValue));
5149
return Expressions.convert_(result, Number.class);
5250
}
5351
}

‎core/src/main/java/org/opensearch/sql/expression/function/udf/RmcommaConvertFunction.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,7 @@
1818
import org.opensearch.sql.expression.function.ImplementorUDF;
1919
import org.opensearch.sql.expression.function.UDFOperandMetadata;
2020

21-
/**
22-
* PPL rmcomma() conversion function. Removes commas from numeric strings and converts to numbers.
23-
*/
21+
/** PPL rmcomma() conversion function. */
2422
public class RmcommaConvertFunction extends ImplementorUDF {
2523

2624
public RmcommaConvertFunction() {
@@ -29,7 +27,7 @@ public RmcommaConvertFunction() {
2927

3028
@Override
3129
public SqlReturnTypeInference getReturnTypeInference() {
32-
return ReturnTypes.VARCHAR_FORCE_NULLABLE;
30+
return ReturnTypes.VARCHAR_NULLABLE;
3331
}
3432

3533
@Override
@@ -42,7 +40,7 @@ public static class RmcommaConvertImplementor implements NotNullImplementor {
4240
public Expression implement(
4341
RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) {
4442
Expression fieldValue = translatedOperands.get(0);
45-
return Expressions.call(ConversionUtils.class, "rmcommaConvert", fieldValue);
43+
return Expressions.call(ConversionUtils.class, "rmcommaConvert", Expressions.box(fieldValue));
4644
}
4745
}
4846
}

‎core/src/main/java/org/opensearch/sql/expression/function/udf/RmunitConvertFunction.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,7 @@
1818
import org.opensearch.sql.expression.function.ImplementorUDF;
1919
import org.opensearch.sql.expression.function.UDFOperandMetadata;
2020

21-
/**
22-
* PPL rmunit() conversion function. Extracts leading numeric values from strings and removes
23-
* trailing text/units.
24-
*/
21+
/** PPL rmunit() conversion function. */
2522
public class RmunitConvertFunction extends ImplementorUDF {
2623

2724
public RmunitConvertFunction() {
@@ -46,7 +43,8 @@ public static class RmunitConvertImplementor implements NotNullImplementor {
4643
public Expression implement(
4744
RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) {
4845
Expression fieldValue = translatedOperands.get(0);
49-
Expression result = Expressions.call(ConversionUtils.class, "rmunitConvert", fieldValue);
46+
Expression result =
47+
Expressions.call(ConversionUtils.class, "rmunitConvert", Expressions.box(fieldValue));
5048
return Expressions.convert_(result, Number.class);
5149
}
5250
}

‎core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import org.opensearch.sql.executor.pagination.PlanSerializer;
99
import org.opensearch.sql.planner.logical.LogicalAggregation;
1010
import org.opensearch.sql.planner.logical.LogicalCloseCursor;
11-
import org.opensearch.sql.planner.logical.LogicalConvert;
1211
import org.opensearch.sql.planner.logical.LogicalDedupe;
1312
import org.opensearch.sql.planner.logical.LogicalEval;
1413
import org.opensearch.sql.planner.logical.LogicalFetchCursor;
@@ -28,7 +27,6 @@
2827
import org.opensearch.sql.planner.logical.LogicalValues;
2928
import org.opensearch.sql.planner.logical.LogicalWindow;
3029
import org.opensearch.sql.planner.physical.AggregationOperator;
31-
import org.opensearch.sql.planner.physical.ConvertOperator;
3230
import org.opensearch.sql.planner.physical.CursorCloseOperator;
3331
import org.opensearch.sql.planner.physical.DedupeOperator;
3432
import org.opensearch.sql.planner.physical.EvalOperator;
@@ -101,12 +99,6 @@ public PhysicalPlan visitEval(LogicalEval node, C context) {
10199
return new EvalOperator(visitChild(node, context), node.getExpressions());
102100
}
103101

104-
@Override
105-
public PhysicalPlan visitConvert(LogicalConvert node, C context) {
106-
return new ConvertOperator(
107-
visitChild(node, context), node.getConversions(), node.getTimeformat());
108-
}
109-
110102
@Override
111103
public PhysicalPlan visitNested(LogicalNested node, C context) {
112104
return new NestedOperator(visitChild(node, context), node.getFields());

0 commit comments

Comments
 (0)