From b51affcf56f9266a0a926d5b11b6638335a78b4d Mon Sep 17 00:00:00 2001 From: currantw Date: Fri, 7 Feb 2025 12:54:51 -0800 Subject: [PATCH] Additional review comments, including move constants to `ExprValueUtils`. Signed-off-by: currantw --- .../org/opensearch/sql/analysis/Analyzer.java | 19 +++++++------------ ...ataSourceSchemaIdentifierNameResolver.java | 5 ++--- .../sql/data/model/ExprValueUtils.java | 10 ++++++++++ .../sql/expression/ReferenceExpression.java | 6 +++--- .../sql/planner/physical/FlattenOperator.java | 6 ++---- .../org/opensearch/sql/utils/PathUtils.java | 19 ------------------- 6 files changed, 24 insertions(+), 41 deletions(-) delete mode 100644 core/src/main/java/org/opensearch/sql/utils/PathUtils.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java index 1b4e6b0a55..f8775adb1b 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java @@ -21,7 +21,6 @@ import static org.opensearch.sql.utils.MLCommonsConstants.RCF_ANOMALY_GRADE; import static org.opensearch.sql.utils.MLCommonsConstants.RCF_SCORE; import static org.opensearch.sql.utils.MLCommonsConstants.TIME_FIELD; -import static org.opensearch.sql.utils.PathUtils.SEPARATOR; import static org.opensearch.sql.utils.SystemIndexUtils.DATASOURCES_TABLE_NAME; import com.google.common.collect.ImmutableList; @@ -77,6 +76,7 @@ import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.common.utils.StringUtils; import org.opensearch.sql.data.model.ExprMissingValue; +import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.datasource.DataSourceService; @@ -511,12 +511,13 @@ public LogicalPlan visitFlatten(Flatten node, AnalysisContext context) { Map fieldsMap = env.lookupAllTupleFields(FIELD_NAME); final String fieldParentPathPrefix = - fieldName.contains(SEPARATOR) - ? fieldName.substring(0, fieldName.lastIndexOf(SEPARATOR)) + SEPARATOR + fieldName.contains(ExprValueUtils.QUALIFIED_NAME_SEPARATOR) + ? fieldName.substring(0, fieldName.lastIndexOf(ExprValueUtils.QUALIFIED_NAME_SEPARATOR)) + + ExprValueUtils.QUALIFIED_NAME_SEPARATOR : ""; // Get entries for paths that are descended from the flattened field. - final String fieldDescendantPathPrefix = fieldName + SEPARATOR; + final String fieldDescendantPathPrefix = fieldName + ExprValueUtils.QUALIFIED_NAME_SEPARATOR; List> fieldDescendantEntries = fieldsMap.entrySet().stream() .filter(e -> e.getKey().startsWith(fieldDescendantPathPrefix)) @@ -525,14 +526,8 @@ public LogicalPlan visitFlatten(Flatten node, AnalysisContext context) { // Get fields to add from descendant entries. Map addFieldsMap = new HashMap<>(); for (Map.Entry entry : fieldDescendantEntries) { - String path = entry.getKey(); - - // Build the new path. - String newPath = path.substring(fieldDescendantPathPrefix.length()); - if (!fieldParentPathPrefix.isEmpty()) { - newPath = fieldParentPathPrefix + newPath; - } - + String newPath = + fieldParentPathPrefix + entry.getKey().substring(fieldDescendantPathPrefix.length()); addFieldsMap.put(newPath, entry.getValue()); } diff --git a/core/src/main/java/org/opensearch/sql/analysis/DataSourceSchemaIdentifierNameResolver.java b/core/src/main/java/org/opensearch/sql/analysis/DataSourceSchemaIdentifierNameResolver.java index baec575206..99f0453427 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/DataSourceSchemaIdentifierNameResolver.java +++ b/core/src/main/java/org/opensearch/sql/analysis/DataSourceSchemaIdentifierNameResolver.java @@ -7,9 +7,8 @@ package org.opensearch.sql.analysis; -import static org.opensearch.sql.utils.PathUtils.SEPARATOR; - import java.util.List; +import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.datasource.DataSourceService; public class DataSourceSchemaIdentifierNameResolver { @@ -35,7 +34,7 @@ public DataSourceSchemaIdentifierNameResolver( DataSourceService dataSourceService, List parts) { this.dataSourceService = dataSourceService; List remainingParts = captureSchemaName(captureDataSourceName(parts)); - identifierName = String.join(SEPARATOR, remainingParts); + identifierName = String.join(ExprValueUtils.QUALIFIED_NAME_SEPARATOR, remainingParts); } public String getIdentifierName() { diff --git a/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java b/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java index 890e0ef8d5..36be8dc648 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java @@ -16,6 +16,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.regex.Pattern; import lombok.experimental.UtilityClass; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.exception.ExpressionEvaluationException; @@ -23,11 +24,20 @@ /** The definition of {@link ExprValue} factory. */ @UtilityClass public class ExprValueUtils { + + // Literal constants public static final ExprValue LITERAL_TRUE = ExprBooleanValue.of(true); public static final ExprValue LITERAL_FALSE = ExprBooleanValue.of(false); public static final ExprValue LITERAL_NULL = ExprNullValue.of(); public static final ExprValue LITERAL_MISSING = ExprMissingValue.of(); + /** Qualified name separator string */ + public final String QUALIFIED_NAME_SEPARATOR = "."; + + /** Pattern that matches the qualified name separator string */ + public final Pattern QUALIFIED_NAME_SEPARATOR_PATTERN = + Pattern.compile(QUALIFIED_NAME_SEPARATOR, Pattern.LITERAL); + public static ExprValue booleanValue(Boolean value) { return value ? LITERAL_TRUE : LITERAL_FALSE; } diff --git a/core/src/main/java/org/opensearch/sql/expression/ReferenceExpression.java b/core/src/main/java/org/opensearch/sql/expression/ReferenceExpression.java index 62a8a43c91..c249b426f6 100644 --- a/core/src/main/java/org/opensearch/sql/expression/ReferenceExpression.java +++ b/core/src/main/java/org/opensearch/sql/expression/ReferenceExpression.java @@ -5,8 +5,6 @@ package org.opensearch.sql.expression; -import static org.opensearch.sql.utils.PathUtils.SEPARATOR; - import java.util.Arrays; import java.util.List; import lombok.EqualsAndHashCode; @@ -14,6 +12,7 @@ import lombok.RequiredArgsConstructor; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.expression.env.Environment; @@ -106,7 +105,8 @@ public ExprValue resolve(ExprTupleValue value) { } private ExprValue resolve(ExprValue value, List paths) { - ExprValue wholePathValue = value.keyValue(String.join(SEPARATOR, paths)); + ExprValue wholePathValue = + value.keyValue(String.join(ExprValueUtils.QUALIFIED_NAME_SEPARATOR, paths)); // For array types only first index currently supported. if (value.type().equals(ExprCoreType.ARRAY)) { wholePathValue = value.collectionValue().get(0).keyValue(paths.get(0)); diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/FlattenOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/FlattenOperator.java index f187a0611b..e42d8b65bd 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/FlattenOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/FlattenOperator.java @@ -5,8 +5,6 @@ package org.opensearch.sql.planner.physical; -import static org.opensearch.sql.utils.PathUtils.SEPARATOR_PATTERN; - import java.util.Collections; import java.util.List; import java.util.Map; @@ -53,11 +51,11 @@ public ExprValue next() { /** * Flattens the {@link ExprTupleValue} at the specified path within the given root value and * returns the result. Returns the unmodified root value if it does not contain a value at the - * specified path. + * specified path. rootExprValue is expected to be an {@link ExprTupleValue}. */ private static ExprValue flattenExprValueAtPath(ExprValue rootExprValue, String path) { - Matcher matcher = SEPARATOR_PATTERN.matcher(path); + Matcher matcher = ExprValueUtils.QUALIFIED_NAME_SEPARATOR_PATTERN.matcher(path); Map exprValueMap = ExprValueUtils.getTupleValue(rootExprValue); // [A] Flatten nested struct value diff --git a/core/src/main/java/org/opensearch/sql/utils/PathUtils.java b/core/src/main/java/org/opensearch/sql/utils/PathUtils.java deleted file mode 100644 index 3414cdfcfb..0000000000 --- a/core/src/main/java/org/opensearch/sql/utils/PathUtils.java +++ /dev/null @@ -1,19 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.utils; - -import java.util.regex.Pattern; -import lombok.experimental.UtilityClass; - -@UtilityClass -public class PathUtils { - - /** Path separator string */ - public final String SEPARATOR = "."; - - /** Pattern that matches the path separator string */ - public final Pattern SEPARATOR_PATTERN = Pattern.compile(SEPARATOR, Pattern.LITERAL); -}