Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Quick Fixes" to Assorted IDE Warnings #3190

Closed
61 changes: 35 additions & 26 deletions core/src/main/java/org/opensearch/sql/analysis/Analyzer.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.opensearch.sql.analysis.symbol.Namespace;
import org.opensearch.sql.analysis.symbol.Symbol;
import org.opensearch.sql.ast.AbstractNodeVisitor;
import org.opensearch.sql.ast.Node;
import org.opensearch.sql.ast.expression.Argument;
import org.opensearch.sql.ast.expression.Field;
import org.opensearch.sql.ast.expression.Let;
Expand Down Expand Up @@ -174,7 +175,7 @@ public LogicalPlan visitRelation(Relation node, AnalysisContext context) {

@Override
public LogicalPlan visitRelationSubquery(RelationSubquery node, AnalysisContext context) {
LogicalPlan subquery = analyze(node.getChild().get(0), context);
LogicalPlan subquery = visitFirstChild(node, context);
// inherit the parent environment to keep the subquery fields in current environment
TypeEnvironment curEnv = context.peek();

Expand Down Expand Up @@ -227,13 +228,13 @@ public LogicalPlan visitTableFunction(TableFunction node, AnalysisContext contex

@Override
public LogicalPlan visitLimit(Limit node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
return new LogicalLimit(child, node.getLimit(), node.getOffset());
}

@Override
public LogicalPlan visitFilter(Filter node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
Expression condition = expressionAnalyzer.analyze(node.getCondition(), context);

ExpressionReferenceOptimizer optimizer =
Expand All @@ -259,15 +260,14 @@ private void verifySupportsCondition(Expression condition) {
"Falling back to legacy engine. Nested function is not supported in WHERE,"
+ " GROUP BY, and HAVING clauses.");
}
((FunctionExpression) condition)
.getArguments().stream().forEach(e -> verifySupportsCondition(e));
((FunctionExpression) condition).getArguments().forEach(this::verifySupportsCondition);
}
}

/** Build {@link LogicalRename}. */
@Override
public LogicalPlan visitRename(Rename node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
ImmutableMap.Builder<ReferenceExpression, ReferenceExpression> renameMapBuilder =
new ImmutableMap.Builder<>();
for (Map renameMap : node.getRenameList()) {
Expand All @@ -294,7 +294,7 @@ public LogicalPlan visitRename(Rename node, AnalysisContext context) {
/** Build {@link LogicalAggregation}. */
@Override
public LogicalPlan visitAggregation(Aggregation node, AnalysisContext context) {
final LogicalPlan child = node.getChild().get(0).accept(this, context);
final LogicalPlan child = visitFirstChild(node, context);
ImmutableList.Builder<NamedAggregator> aggregatorBuilder = new ImmutableList.Builder<>();
for (UnresolvedExpression expr : node.getAggExprList()) {
NamedExpression aggExpr = namedExpressionAnalyzer.analyze(expr, context);
Expand Down Expand Up @@ -332,7 +332,7 @@ public LogicalPlan visitAggregation(Aggregation node, AnalysisContext context) {
/** Build {@link LogicalRareTopN}. */
@Override
public LogicalPlan visitRareTopN(RareTopN node, AnalysisContext context) {
final LogicalPlan child = node.getChild().get(0).accept(this, context);
final LogicalPlan child = visitFirstChild(node, context);

ImmutableList.Builder<Expression> groupbyBuilder = new ImmutableList.Builder<>();
for (UnresolvedExpression expr : node.getGroupExprList()) {
Expand All @@ -355,7 +355,7 @@ public LogicalPlan visitRareTopN(RareTopN node, AnalysisContext context) {
field -> newEnv.define(new Symbol(Namespace.FIELD_NAME, field.toString()), field.type()));

List<Argument> options = node.getNoOfResults();
Integer noOfResults = (Integer) options.get(0).getValue().getValue();
Integer noOfResults = (Integer) options.getFirst().getValue().getValue();

return new LogicalRareTopN(child, node.getCommandType(), noOfResults, fields, groupBys);
}
Expand All @@ -373,18 +373,18 @@ public LogicalPlan visitRareTopN(RareTopN node, AnalysisContext context) {
*/
@Override
public LogicalPlan visitProject(Project node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);

if (node.hasArgument()) {
Argument argument = node.getArgExprList().get(0);
Argument argument = node.getArgExprList().getFirst();
Boolean exclude = (Boolean) argument.getValue().getValue();
if (exclude) {
TypeEnvironment curEnv = context.peek();
List<ReferenceExpression> referenceExpressions =
node.getProjectList().stream()
.map(expr -> (ReferenceExpression) expressionAnalyzer.analyze(expr, context))
.collect(Collectors.toList());
referenceExpressions.forEach(ref -> curEnv.remove(ref));
referenceExpressions.forEach(curEnv::remove);
return new LogicalRemove(child, ImmutableSet.copyOf(referenceExpressions));
}
}
Expand Down Expand Up @@ -427,7 +427,7 @@ public LogicalPlan visitProject(Project node, AnalysisContext context) {
/** Build {@link LogicalEval}. */
@Override
public LogicalPlan visitEval(Eval node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
ImmutableList.Builder<Pair<ReferenceExpression, Expression>> expressionsBuilder =
new Builder<>();
for (Let let : node.getExpressionList()) {
Expand All @@ -444,7 +444,7 @@ public LogicalPlan visitEval(Eval node, AnalysisContext context) {
/** Build {@link ParseExpression} to context and skip to child nodes. */
@Override
public LogicalPlan visitParse(Parse node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
Expression sourceField = expressionAnalyzer.analyze(node.getSourceField(), context);
ParseMethod parseMethod = node.getParseMethod();
java.util.Map<String, Literal> arguments = node.getArguments();
Expand All @@ -467,7 +467,7 @@ public LogicalPlan visitParse(Parse node, AnalysisContext context) {
/** Build {@link LogicalSort}. */
@Override
public LogicalPlan visitSort(Sort node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
ExpressionReferenceOptimizer optimizer =
new ExpressionReferenceOptimizer(expressionAnalyzer.getRepository(), child);

Expand All @@ -490,7 +490,7 @@ public LogicalPlan visitSort(Sort node, AnalysisContext context) {
/** Build {@link LogicalDedupe}. */
@Override
public LogicalPlan visitDedupe(Dedupe node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
List<Argument> options = node.getOptions();
// Todo, refactor the option.
Integer allowedDuplication = (Integer) options.get(0).getValue().getValue();
Expand All @@ -509,7 +509,7 @@ public LogicalPlan visitDedupe(Dedupe node, AnalysisContext context) {

/** Logical head is identical to {@link LogicalLimit}. */
public LogicalPlan visitHead(Head node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
return new LogicalLimit(child, node.getSize(), node.getFrom());
}

Expand All @@ -529,7 +529,7 @@ public LogicalPlan visitValues(Values node, AnalysisContext context) {
/** Build {@link LogicalMLCommons} for Kmeans command. */
@Override
public LogicalPlan visitKmeans(Kmeans node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
java.util.Map<String, Literal> options = node.getArguments();

TypeEnvironment currentEnv = context.peek();
Expand All @@ -541,7 +541,7 @@ public LogicalPlan visitKmeans(Kmeans node, AnalysisContext context) {
/** Build {@link LogicalAD} for AD command. */
@Override
public LogicalPlan visitAD(AD node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
java.util.Map<String, Literal> options = node.getArguments();

TypeEnvironment currentEnv = context.peek();
Expand All @@ -561,18 +561,17 @@ public LogicalPlan visitAD(AD node, AnalysisContext context) {
/** Build {@link LogicalML} for ml command. */
@Override
public LogicalPlan visitML(ML node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(node, context);
TypeEnvironment currentEnv = context.peek();
node.getOutputSchema(currentEnv).entrySet().stream()
.forEach(
v -> currentEnv.define(new Symbol(Namespace.FIELD_NAME, v.getKey()), v.getValue()));
node.getOutputSchema(currentEnv)
.forEach((key, value) -> currentEnv.define(new Symbol(Namespace.FIELD_NAME, key), value));

return new LogicalML(child, node.getArguments());
}

@Override
public LogicalPlan visitPaginate(Paginate paginate, AnalysisContext context) {
LogicalPlan child = paginate.getChild().get(0).accept(this, context);
LogicalPlan child = visitFirstChild(paginate, context);
return new LogicalPaginate(paginate.getPageSize(), List.of(child));
}

Expand All @@ -585,15 +584,16 @@ public LogicalPlan visitFetchCursor(FetchCursor cursor, AnalysisContext context)

@Override
public LogicalPlan visitCloseCursor(CloseCursor closeCursor, AnalysisContext context) {
return new LogicalCloseCursor(closeCursor.getChild().get(0).accept(this, context));
LogicalPlan child = visitFirstChild(closeCursor, context);
return new LogicalCloseCursor(child);
}

/**
* The first argument is always "asc", others are optional. Given nullFirst argument, use its
* value. Otherwise just use DEFAULT_ASC/DESC.
*/
private SortOption analyzeSortOption(List<Argument> fieldArgs) {
Boolean asc = (Boolean) fieldArgs.get(0).getValue().getValue();
Boolean asc = (Boolean) fieldArgs.getFirst().getValue().getValue();
Optional<Argument> nullFirst =
fieldArgs.stream().filter(option -> "nullFirst".equals(option.getArgName())).findFirst();

Expand All @@ -603,4 +603,13 @@ private SortOption analyzeSortOption(List<Argument> fieldArgs) {
}
return asc ? SortOption.DEFAULT_ASC : SortOption.DEFAULT_DESC;
}

/**
* Visit the first child of the specified node with the given analysis context.
*
* @return the resulting logical plan
*/
private LogicalPlan visitFirstChild(Node node, AnalysisContext context) {
return node.getChild().getFirst().accept(this, context);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,10 @@ public LogicalPlan analyze(UnresolvedExpression projectItem, AnalysisContext con

@Override
public LogicalPlan visitAlias(Alias node, AnalysisContext context) {
if (!(node.getDelegated() instanceof WindowFunction)) {
if (!(node.getDelegated() instanceof WindowFunction unresolved)) {
return null;
}

WindowFunction unresolved = (WindowFunction) node.getDelegated();
Expression windowFunction = expressionAnalyzer.analyze(unresolved, context);
List<Expression> partitionByList = analyzePartitionList(unresolved, context);
List<Pair<SortOption, Expression>> sortList = analyzeSortList(unresolved, context);
Expand Down Expand Up @@ -98,9 +97,9 @@ private List<Pair<SortOption, Expression>> analyzeSortList(
* The final and default value for each is determined here during expression analysis.
*/
private SortOption analyzeSortOption(SortOption option) {
if (option.getNullOrder() == null) {
return (option.getSortOrder() == DESC) ? DEFAULT_DESC : DEFAULT_ASC;
if (option.nullOrder() == null) {
return (option.sortOrder() == DESC) ? DEFAULT_DESC : DEFAULT_ASC;
}
return new SortOption((option.getSortOrder() == DESC) ? DESC : ASC, option.getNullOrder());
return new SortOption((option.sortOrder() == DESC) ? DESC : ASC, option.nullOrder());
}
}
12 changes: 0 additions & 12 deletions core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,6 @@ public static List<Argument> exprList(Argument... exprList) {
return Arrays.asList(exprList);
}

public static List<UnresolvedArgument> unresolvedArgList(UnresolvedArgument... exprList) {
return Arrays.asList(exprList);
}

public static List<Argument> defaultFieldsArgs() {
return exprList(argument("exclude", booleanLiteral(false)));
}
Expand All @@ -421,10 +417,6 @@ public static List<Argument> defaultDedupArgs() {
argument("consecutive", booleanLiteral(false)));
}

public static List<Argument> sortOptions() {
return exprList(argument("desc", booleanLiteral(false)));
}

public static List<Argument> defaultSortFieldArgs() {
return exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()));
}
Expand All @@ -445,10 +437,6 @@ public static Head head(UnresolvedPlan input, Integer size, Integer from) {
return new Head(input, size, from);
}

public static List<Argument> defaultTopArgs() {
return exprList(argument("noOfResults", intLiteral(10)));
}

public static RareTopN rareTopN(
UnresolvedPlan input,
CommandType commandType,
Expand Down
7 changes: 1 addition & 6 deletions core/src/main/java/org/opensearch/sql/ast/tree/Sort.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import com.google.common.collect.ImmutableList;
import java.util.List;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
Expand Down Expand Up @@ -48,17 +47,13 @@ public <T, C> T accept(AbstractNodeVisitor<T, C> nodeVisitor, C context) {
}

/** Sort Options. */
@Data
public static class SortOption {
public record SortOption(SortOrder sortOrder, NullOrder nullOrder) {

/** Default ascending sort option, null first. */
public static SortOption DEFAULT_ASC = new SortOption(ASC, NULL_FIRST);

/** Default descending sort option, null last. */
public static SortOption DEFAULT_DESC = new SortOption(DESC, NULL_LAST);

private final SortOrder sortOrder;
private final NullOrder nullOrder;
}

public enum SortOrder {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,56 +100,35 @@ public static ExprValue nullValue() {

/** Construct ExprValue from Object. */
public static ExprValue fromObjectValue(Object o) {
if (null == o) {
return LITERAL_NULL;
}
if (o instanceof Map) {
return tupleValue((Map) o);
} else if (o instanceof List) {
return collectionValue(((List) o));
} else if (o instanceof Byte) {
return byteValue((Byte) o);
} else if (o instanceof Short) {
return shortValue((Short) o);
} else if (o instanceof Integer) {
return integerValue((Integer) o);
} else if (o instanceof Long) {
return longValue(((Long) o));
} else if (o instanceof Boolean) {
return booleanValue((Boolean) o);
} else if (o instanceof Double) {
return doubleValue((Double) o);
} else if (o instanceof String) {
return stringValue((String) o);
} else if (o instanceof Float) {
return floatValue((Float) o);
} else if (o instanceof LocalDate) {
return dateValue((LocalDate) o);
} else if (o instanceof LocalTime) {
return timeValue((LocalTime) o);
} else if (o instanceof Instant) {
return timestampValue((Instant) o);
} else if (o instanceof TemporalAmount) {
return intervalValue((TemporalAmount) o);
} else if (o instanceof LocalDateTime) {
return timestampValue(((LocalDateTime) o).toInstant(ZoneOffset.UTC));
} else {
throw new ExpressionEvaluationException("unsupported object " + o.getClass());
}
return switch (o) {
case null -> LITERAL_NULL;
case Map map -> tupleValue(map);
case List list -> collectionValue(list);
case Byte b -> byteValue(b);
case Short i -> shortValue(i);
case Integer i -> integerValue(i);
case Long l -> longValue(l);
case Boolean b -> booleanValue(b);
case Double v -> doubleValue(v);
case String s -> stringValue(s);
case Float v -> floatValue(v);
case LocalDate localDate -> dateValue(localDate);
case LocalTime localTime -> timeValue(localTime);
case Instant instant -> timestampValue(instant);
case TemporalAmount temporalAmount -> intervalValue(temporalAmount);
case LocalDateTime localDateTime -> timestampValue(localDateTime.toInstant(ZoneOffset.UTC));
default -> throw new ExpressionEvaluationException("unsupported object " + o.getClass());
};
}

/** Construct ExprValue from Object with ExprCoreType. */
public static ExprValue fromObjectValue(Object o, ExprCoreType type) {
switch (type) {
case TIMESTAMP:
return new ExprTimestampValue((String) o);
case DATE:
return new ExprDateValue((String) o);
case TIME:
return new ExprTimeValue((String) o);
default:
return fromObjectValue(o);
}
return switch (type) {
case TIMESTAMP -> new ExprTimestampValue((String) o);
case DATE -> new ExprDateValue((String) o);
case TIME -> new ExprTimeValue((String) o);
default -> fromObjectValue(o);
};
}

public static Byte getByteValue(ExprValue exprValue) {
Expand Down
Loading
Loading