mihaibudiu commented on code in PR #5266:
URL: https://github.com/apache/calcite/pull/5266#discussion_r4020868985
##########
elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchRules.java:
##########
@@ -136,10 +138,57 @@ static String stripQuotes(String s) {
? s.substring(1, s.length() - 1) : s;
}
- private static String escapeSpecialSymbols(String s) {
+ /** Escapes the two characters that would terminate a JSON string. A SQL
Review Comment:
I am not sure about the connection between this and JSON.
If this is about JSON, the function name should reflect that.
Maybe this happens to work just fine for JSON, but then this is not the main
thing JavaDoc should say.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -3913,8 +3899,13 @@ private SqlDialect nonOrdinalDialect() {
@Test void testHiveAndSparkTrimWithBothSpecialCharacter() {
final String query = "SELECT TRIM(BOTH '$@*A' from
'$@*AABC$@*AADCAA$@*A')\n"
+ "from \"foodmart\".\"reserve_employee\"";
+ // Hive and Spark recognize backslash escape sequences inside single-quoted
+ // string constants, so each backslash produced by escapeSpecialChar is
+ // doubled by the dialect. The emitted regex therefore contains \\$\\@\\*A
+ // rather than \$\@\*A; when Hive/Spark parse the literal they collapse
+ // it back to \$\@\*A, which is what the REGEXP_REPLACE engine sees.
final String expected = "SELECT REGEXP_REPLACE('$@*AABC$@*AADCAA$@*A',"
- + " '^(\\$\\@\\*A)*|(\\$\\@\\*A)*$', '')\n"
+ + " '^(\\\\$\\\\@\\\\*A)*|(\\\\$\\\\@\\\\*A)*$', '')\n"
Review Comment:
This looks very complicated. I hope it works.
##########
geode/src/main/java/org/apache/calcite/adapter/geode/util/GeodeUtils.java:
##########
@@ -317,6 +318,38 @@ public static RelDataType
autodetectRelTypeFromRegion(Region<?, ?> region) {
return createRelDataType(entry);
}
+ /**
+ * OQL has no way to escape identifiers, so an identifier that is copied
+ * verbatim into the generated query must be a simple alphanumeric name (with
+ * optional dotted path or bracketed integer subscript for nested access).
+ * Anything else could shift the boundaries of the emitted statement.
+ */
+ private static final Pattern SAFE_OQL_IDENTIFIER =
+ Pattern.compile(
+
"[A-Za-z_$][A-Za-z0-9_$]*+(?:\\.[A-Za-z_$][A-Za-z0-9_$]*+|\\[[0-9]++\\])*+");
+
+ /**
+ * Returns whether {@code name} may be emitted verbatim as an OQL identifier
+ * (a field name, alias, or dotted path). Since OQL provides no way to quote
+ * an identifier, callers should refuse to push down when this returns false.
+ */
+ public static boolean isSafeOqlIdentifier(String name) {
+ return name != null && SAFE_OQL_IDENTIFIER.matcher(name).matches();
+ }
+
+ /**
+ * Validates that {@code name} may be emitted verbatim into a generated OQL
+ * statement, throwing a {@link RuntimeException} otherwise so the planner
Review Comment:
The function does not need to mention what callers are using it.
##########
splunk/src/test/java/org/apache/calcite/adapter/splunk/SplunkPushDownRuleTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.adapter.splunk;
+
+import org.apache.calcite.jdbc.JavaTypeFactoryImpl;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexInputRef;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.type.SqlTypeName;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * Unit tests for the field-name allowlist inside
+ * {@link SplunkPushDownRule}. SPL has no generic identifier-quoting
Review Comment:
in general, some of these comments are repeated many times. Maybe it's fine.
##########
splunk/src/test/java/org/apache/calcite/adapter/splunk/SplunkPushDownRuleTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.adapter.splunk;
+
+import org.apache.calcite.jdbc.JavaTypeFactoryImpl;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexInputRef;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.type.SqlTypeName;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * Unit tests for the field-name allowlist inside
+ * {@link SplunkPushDownRule}. SPL has no generic identifier-quoting
+ * mechanism, so a field name that is copied verbatim into the generated
+ * search must match a strict allowlist; anything else could shift the
+ * boundaries of the emitted command, and the corresponding filter or
+ * rename is evaluated by Calcite instead.
+ *
+ * <p>The tests exercise the filter translation directly and do not
+ * require a running Splunk instance.
+ */
+class SplunkPushDownRuleTest {
+
+ private static final RelDataTypeFactory TYPE_FACTORY =
+ new JavaTypeFactoryImpl();
+ private static final RexBuilder REX_BUILDER = new RexBuilder(TYPE_FACTORY);
+
+ /** Translates {@code <field> = 'foo'} via the rule's (private)
+ * {@code getFilter}, returning the SPL string, or null if the rule
+ * refuses the push down. */
+ private static String filterString(String fieldName) throws Throwable {
+ RelDataType varchar = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR);
+ RexInputRef ref = REX_BUILDER.makeInputRef(varchar, 0);
+ RexNode lit = REX_BUILDER.makeLiteral("foo");
+ Method m =
Review Comment:
why do you need to use reflection to invoke the method?
##########
cassandra/src/test/java/org/apache/calcite/adapter/cassandra/CassandraIdentifierQuotingTest.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.adapter.cassandra;
+
+import com.datastax.oss.driver.api.core.CqlIdentifier;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * Unit tests documenting how {@link CqlIdentifier#fromInternal} is used in
+ * {@link CassandraTable#query} to emit the keyspace, column-family, column
+ * and alias tokens of a generated CQL {@code SELECT} statement. Any of
+ * those pieces is copied verbatim into the query text, so a value that
+ * contains punctuation or the {@code "} delimiter must be routed through
+ * the driver so that it cannot shift the boundaries of the emitted CQL.
Review Comment:
I am not sure what the driver is
"shift the boundaries" is rather vague to me
Emit incorrect CQL?
##########
elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchRules.java:
##########
@@ -136,10 +138,57 @@ static String stripQuotes(String s) {
? s.substring(1, s.length() - 1) : s;
}
- private static String escapeSpecialSymbols(String s) {
+ /** Escapes the two characters that would terminate a JSON string. A SQL
+ * name or alias concatenated between double quotes stays a single JSON
+ * string only if backslash and double quote are doubled. Callers that
+ * embed the value inside a JSON body must route it through this helper
+ * before calling {@link #quote}. */
+ static String escapeSpecialSymbols(String s) {
return s.replace("\\", "\\\\").replace("\"", "\\\"");
}
+ /** Shape of field paths that may be embedded in a scripted-field
+ * script using the historical dot notation, for example {@code city},
+ * {@code b.a} or {@code loc[0]}. Possessive quantifiers avoid
Review Comment:
I would move the comment about quantifiers inside the function and explain
that *+ is a possessive; I had to look it up.
##########
cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraTable.java:
##########
@@ -190,18 +197,20 @@ public Enumerable<Object> query(final CqlSession session,
List<Map.Entry<String,
whereClause += Util.toString(predicates, "", " AND ", "");
}
- // Build and issue the query and return an Enumerator over the results
+ // Build and issue the query and return an Enumerator over the results.
+ // Route the keyspace and column-family names through CqlIdentifier so
Review Comment:
should this comment be on line 210?
##########
elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchRules.java:
##########
@@ -136,10 +138,57 @@ static String stripQuotes(String s) {
? s.substring(1, s.length() - 1) : s;
}
- private static String escapeSpecialSymbols(String s) {
+ /** Escapes the two characters that would terminate a JSON string. A SQL
+ * name or alias concatenated between double quotes stays a single JSON
+ * string only if backslash and double quote are doubled. Callers that
+ * embed the value inside a JSON body must route it through this helper
+ * before calling {@link #quote}. */
+ static String escapeSpecialSymbols(String s) {
return s.replace("\\", "\\\\").replace("\"", "\\\"");
}
+ /** Shape of field paths that may be embedded in a scripted-field
+ * script using the historical dot notation, for example {@code city},
+ * {@code b.a} or {@code loc[0]}. Possessive quantifiers avoid
+ * backtracking on inputs that almost, but do not quite, match. */
+ private static final Pattern SIMPLE_SCRIPT_PATH =
+ Pattern.compile("[A-Za-z_][A-Za-z0-9_]*+"
+ + "(?:\\.[A-Za-z_][A-Za-z0-9_]*+)*+(?:\\[[0-9]++\\])*+");
+
+ /** Trailing array subscripts of a field path, for example {@code [0]}
+ * in {@code loc[0]}. Possessive quantifiers as above. */
+ private static final Pattern TRAILING_SUBSCRIPTS =
+ Pattern.compile("(?:\\[[0-9]++\\])++$");
+
+ /** Builds the script snippet that reads {@code path} from the document
Review Comment:
what is the language of the script?
##########
geode/src/main/java/org/apache/calcite/adapter/geode/rel/GeodeAggregate.java:
##########
@@ -100,7 +101,10 @@ public GeodeAggregate(RelOptCluster cluster,
List<String> groupByFields = new ArrayList<>();
for (int group : groupSet) {
- groupByFields.add(inputFields.get(group));
+ // OQL provides no way to quote identifiers, so the group key must
+ // match a strict allowlist before it can be copied verbatim into the
+ // generated statement.
+
groupByFields.add(GeodeUtils.checkSafeOqlIdentifier(inputFields.get(group)));
Review Comment:
I assume that the SafeOqlIdentifier function throws when needed
##########
elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchRules.java:
##########
@@ -136,10 +138,57 @@ static String stripQuotes(String s) {
? s.substring(1, s.length() - 1) : s;
}
- private static String escapeSpecialSymbols(String s) {
+ /** Escapes the two characters that would terminate a JSON string. A SQL
+ * name or alias concatenated between double quotes stays a single JSON
+ * string only if backslash and double quote are doubled. Callers that
+ * embed the value inside a JSON body must route it through this helper
+ * before calling {@link #quote}. */
+ static String escapeSpecialSymbols(String s) {
return s.replace("\\", "\\\\").replace("\"", "\\\"");
}
+ /** Shape of field paths that may be embedded in a scripted-field
+ * script using the historical dot notation, for example {@code city},
+ * {@code b.a} or {@code loc[0]}. Possessive quantifiers avoid
+ * backtracking on inputs that almost, but do not quite, match. */
+ private static final Pattern SIMPLE_SCRIPT_PATH =
+ Pattern.compile("[A-Za-z_][A-Za-z0-9_]*+"
+ + "(?:\\.[A-Za-z_][A-Za-z0-9_]*+)*+(?:\\[[0-9]++\\])*+");
+
+ /** Trailing array subscripts of a field path, for example {@code [0]}
+ * in {@code loc[0]}. Possessive quantifiers as above. */
+ private static final Pattern TRAILING_SUBSCRIPTS =
+ Pattern.compile("(?:\\[[0-9]++\\])++$");
+
+ /** Builds the script snippet that reads {@code path} from the document
+ * source, for example {@code params._source.city}.
+ *
+ * <p>Simple paths keep the historical dot notation. Any other path is
+ * accessed via map subscripts (for example
+ * {@code params._source['a-b']}), with the field name confined to a
+ * single-quoted script string literal (backslashes and quotes escaped),
+ * so that a field name containing script punctuation is always treated
+ * as data instead of being parsed as Painless/Groovy code. */
Review Comment:
Is this for Groovy? The function name should mention that
##########
elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchRules.java:
##########
@@ -136,10 +138,57 @@ static String stripQuotes(String s) {
? s.substring(1, s.length() - 1) : s;
}
- private static String escapeSpecialSymbols(String s) {
+ /** Escapes the two characters that would terminate a JSON string. A SQL
+ * name or alias concatenated between double quotes stays a single JSON
+ * string only if backslash and double quote are doubled. Callers that
+ * embed the value inside a JSON body must route it through this helper
+ * before calling {@link #quote}. */
+ static String escapeSpecialSymbols(String s) {
return s.replace("\\", "\\\\").replace("\"", "\\\"");
}
+ /** Shape of field paths that may be embedded in a scripted-field
+ * script using the historical dot notation, for example {@code city},
+ * {@code b.a} or {@code loc[0]}. Possessive quantifiers avoid
+ * backtracking on inputs that almost, but do not quite, match. */
+ private static final Pattern SIMPLE_SCRIPT_PATH =
+ Pattern.compile("[A-Za-z_][A-Za-z0-9_]*+"
+ + "(?:\\.[A-Za-z_][A-Za-z0-9_]*+)*+(?:\\[[0-9]++\\])*+");
+
+ /** Trailing array subscripts of a field path, for example {@code [0]}
Review Comment:
Pattern matching trailing subscripts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
