Skip to content

Commit 6ef78f9

Browse files
anloweeanlowee
committed
Address comments
1 parent a9c4d8d commit 6ef78f9

File tree

6 files changed

+203
-201
lines changed

6 files changed

+203
-201
lines changed

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ public class ClpExpression
2727
{
2828
// Optional KQL query or column name representing the fully or partially translatable part of the expression.
2929
private final Optional<String> pushDownExpression;
30-
// Optinal SQL string extracted from the definition, which is only made of given metadata columns.
30+
// Optional SQL string extracted from the definition, which is only made of given metadata columns.
3131
private final Optional<String> metadataSql;
3232
// The remaining (non-translatable) portion of the RowExpression, if any.
3333
private final Optional<RowExpression> remainingExpression;

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -129,14 +129,16 @@ public ClpExpression visitCall(CallExpression node, Void context)
129129
}
130130

131131
/**
132-
* Handles the BETWEEN expression for numeric range, all arguments must be numeric. If the first
133-
* argument is not a variable reference expression or either the second or the third is not
134-
* constant expressions it won't translate the expression.
132+
* Handles the <code>BETWEEN</code> expression. All arguments must be of numeric types.
133+
* <p></p>
134+
* The translation is only performed if:
135+
* - The first argument is a variable reference expression.
136+
* - The second and third arguments are constant expressions.
135137
*
136138
* <p></p>
137139
* Example: <code>col1 BETWEEN 0 AND 5</code> → <code>col1 >= 0 AND col1 <= 5</code>
138140
*
139-
* @param node the BETWEEN call expression
141+
* @param node the {@code BETWEEN} call expression
140142
* @return a ClpExpression containing either the equivalent KQL query, or the original
141143
* expression if it couldn't be translated
142144
*/
@@ -151,10 +153,9 @@ private ClpExpression handleBetween(CallExpression node)
151153
|| !(node.getArguments().get(2) instanceof ConstantExpression)) {
152154
return new ClpExpression(node);
153155
}
154-
if (!isNumericType(node.getArguments().get(0).getType())
155-
|| !isNumericType(node.getArguments().get(1).getType())
156-
|| !isNumericType(node.getArguments().get(2).getType())) {
157-
// Let the Presto SQL analyzer throw exception
156+
if (!isNumericType(node.getArguments().get(0).getType(), true)
157+
|| !isNumericType(node.getArguments().get(1).getType(), false)
158+
|| !isNumericType(node.getArguments().get(2).getType(), false)) {
158159
return new ClpExpression(node);
159160
}
160161
Optional<String> variableReferencePushDownExpression = node.getArguments().get(0).accept(this, null).getPushDownExpression();

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpMetadataFilterProvider.java‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import com.fasterxml.jackson.annotation.JsonProperty;
2020
import com.fasterxml.jackson.core.type.TypeReference;
2121
import com.fasterxml.jackson.databind.ObjectMapper;
22+
import com.google.common.collect.ImmutableList;
2223
import com.google.common.collect.ImmutableMap;
2324
import com.google.common.collect.ImmutableSet;
2425
import com.google.inject.Inject;
@@ -87,19 +88,20 @@ public ClpMetadataFilterProvider(ClpConfig config)
8788
}
8889
}
8990

90-
public void checkContainsAllFilters(SchemaTableName schemaTableName, String metadataFilterKqlQuery)
91+
public void checkContainsAllFilters(SchemaTableName schemaTableName, String metadataFilterSql)
9192
{
9293
boolean hasAllMetadataFilterColumns = true;
93-
String notFoundFilterColumnName = "";
94+
ImmutableList.Builder<String> notFoundFilterColumnNameListBuilder = ImmutableList.builder();
9495
for (String columnName : getFilterNames(format("%s.%s", CONNECTOR_NAME, schemaTableName))) {
95-
if (!metadataFilterKqlQuery.contains(columnName)) {
96+
if (!metadataFilterSql.contains(columnName)) {
9697
hasAllMetadataFilterColumns = false;
97-
notFoundFilterColumnName = columnName;
98-
break;
98+
notFoundFilterColumnNameListBuilder.add(columnName);
9999
}
100100
}
101101
if (!hasAllMetadataFilterColumns) {
102-
throw new PrestoException(CLP_MANDATORY_METADATA_FILTER_NOT_VALID, notFoundFilterColumnName + " is a mandatory metadata filter column but not valid");
102+
throw new PrestoException(
103+
CLP_MANDATORY_METADATA_FILTER_NOT_VALID,
104+
notFoundFilterColumnNameListBuilder.build() + " is a mandatory metadata filter column but not valid");
103105
}
104106
}
105107

@@ -131,9 +133,6 @@ public void checkContainsAllFilters(SchemaTableName schemaTableName, String meta
131133
public String remapFilterSql(String scope, String sql)
132134
{
133135
String[] splitScope = scope.split("\\.");
134-
if (0 == splitScope.length) {
135-
return sql;
136-
}
137136

138137
Map<String, RangeMapping> mappings = new HashMap<>(getAllMappingsFromTableConfig(filterMap.get(splitScope[0])));
139138

@@ -147,15 +146,17 @@ public String remapFilterSql(String scope, String sql)
147146

148147
String remappedSql = sql;
149148
for (Map.Entry<String, RangeMapping> entry : mappings.entrySet()) {
149+
String key = entry.getKey();
150+
RangeMapping value = entry.getValue();
150151
remappedSql = remappedSql.replaceAll(
151-
format("\"(%s)\"\\s(>=?)\\s([0-9]*)", entry.getKey()),
152-
format("%s $2 $3", entry.getValue().upperBound));
152+
format("\"(%s)\"\\s(>=?)\\s([0-9]*)", key),
153+
format("%s $2 $3", value.upperBound));
153154
remappedSql = remappedSql.replaceAll(
154-
format("\"(%s)\"\\s(<=?)\\s([0-9]*)", entry.getKey()),
155-
format("%s $2 $3", entry.getValue().lowerBound));
155+
format("\"(%s)\"\\s(<=?)\\s([0-9]*)", key),
156+
format("%s $2 $3", value.lowerBound));
156157
remappedSql = remappedSql.replaceAll(
157-
format("\"(%s)\"\\s(=)\\s([0-9]*)", entry.getKey()),
158-
format("(%s <= $3 AND %s >= $3)", entry.getValue().lowerBound, entry.getValue().upperBound));
158+
format("\"(%s)\"\\s(=)\\s([0-9]*)", key),
159+
format("(%s <= $3 AND %s >= $3)", value.lowerBound, value.upperBound));
159160
}
160161
return remappedSql;
161162
}

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public PlanNode visitFilter(FilterNode node, RewriteContext<Void> context)
9393
metadataFilterProvider.checkContainsAllFilters(clpTableHandle.getSchemaTableName(), metadataSql.orElse(""));
9494
if (metadataSql.isPresent()) {
9595
metadataSql = Optional.of(metadataFilterProvider.remapFilterSql(scope, metadataSql.get()));
96-
log.info("Metadata filter SQL query: %s", metadataSql);
96+
log.debug("Metadata filter SQL query: %s", metadataSql);
9797
}
9898

9999
if (!kqlQuery.isPresent()) {

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpUtils.java‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,16 @@
1414
package com.facebook.presto.plugin.clp;
1515

1616
import com.facebook.airlift.log.Logger;
17+
import com.facebook.presto.common.type.DecimalType;
1718
import com.facebook.presto.common.type.Type;
1819
import com.facebook.presto.plugin.clp.metadata.ClpSchemaTree;
1920

2021
import static com.facebook.presto.common.type.BigintType.BIGINT;
2122
import static com.facebook.presto.common.type.DoubleType.DOUBLE;
23+
import static com.facebook.presto.common.type.IntegerType.INTEGER;
24+
import static com.facebook.presto.common.type.RealType.REAL;
25+
import static com.facebook.presto.common.type.SmallintType.SMALLINT;
26+
import static com.facebook.presto.common.type.TinyintType.TINYINT;
2227

2328
public class ClpUtils
2429
{
@@ -33,7 +38,7 @@ private ClpUtils()
3338
* <a href="https://docs.yscope.com/clp/main/user-guide/reference-json-search-syntax">here
3439
* </a> for all special chars in the string value that need to be escaped.
3540
*
36-
* @param literalString the target string to escape special chars '\', '"', ;?' and '*'
41+
* @param literalString the target string to escape special chars '\', '"', '?' and '*'
3742
* @return the escaped string
3843
*/
3944
public static String escapeKqlSpecialCharsForStringValue(String literalString)
@@ -53,8 +58,19 @@ public static String escapeKqlSpecialCharsForStringValue(String literalString)
5358
* @param type the type to check
5459
* @return is the type numeric or not
5560
*/
56-
public static boolean isNumericType(Type type)
61+
public static boolean isNumericType(Type type, boolean isOnlyHandledByClpConnector)
5762
{
58-
return BIGINT == type || DOUBLE == type;
63+
if (isOnlyHandledByClpConnector) {
64+
return BIGINT == type || DOUBLE == type;
65+
}
66+
else {
67+
return type.equals(BIGINT)
68+
|| type.equals(INTEGER)
69+
|| type.equals(SMALLINT)
70+
|| type.equals(TINYINT)
71+
|| type.equals(DOUBLE)
72+
|| type.equals(REAL)
73+
|| type instanceof DecimalType;
74+
}
5975
}
6076
}

0 commit comments

Comments
 (0)