This is an automated email from the ASF dual-hosted git repository.
rubenada pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/main by this push:
new 17db027dbe [CALCITE-7726] Improve identifier validation on
MemberExpression and ParameterExpression
17db027dbe is described below
commit 17db027dbefd8839869be7907b50827bf2d40cd1
Author: Ruben Quesada Lopez <[email protected]>
AuthorDate: Tue Aug 18 16:01:15 2026 +0100
[CALCITE-7726] Improve identifier validation on MemberExpression and
ParameterExpression
---
.../adapter/enumerable/StrictAggImplementor.java | 4 +-
.../apache/calcite/jdbc/JavaTypeFactoryImpl.java | 12 ++-
.../calcite/jdbc/SyntheticRecordFieldNameTest.java | 109 ++++++++++++++++++++
.../calcite/test/WindowPartitionAliasTest.java | 80 +++++++++++++++
.../calcite/linq4j/tree/MemberExpression.java | 5 +
.../calcite/linq4j/tree/ParameterExpression.java | 7 +-
.../java/org/apache/calcite/linq4j/tree/Types.java | 15 +++
.../linq4j/tree/IdentifierValidationTest.java | 113 +++++++++++++++++++++
8 files changed, 336 insertions(+), 9 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/adapter/enumerable/StrictAggImplementor.java
b/core/src/main/java/org/apache/calcite/adapter/enumerable/StrictAggImplementor.java
index 54569acc57..0457ffa61f 100644
---
a/core/src/main/java/org/apache/calcite/adapter/enumerable/StrictAggImplementor.java
+++
b/core/src/main/java/org/apache/calcite/adapter/enumerable/StrictAggImplementor.java
@@ -163,9 +163,7 @@ protected abstract void implementNotNullAdd(AggContext info,
return EnumUtils.convert(
implementNotNullResult(info, result), info.returnType());
}
- String tmpName = result.accumulator().isEmpty()
- ? "ar"
- : (result.accumulator().get(0) + "$Res");
+ String tmpName = result.accumulator().isEmpty() ? "ar" : "acc$Res";
ParameterExpression res =
Expressions.parameter(0, info.returnType(),
result.currentBlock().newName(tmpName));
diff --git
a/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java
b/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java
index 2114921971..32aed69e3d 100644
--- a/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java
+++ b/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java
@@ -367,7 +367,8 @@ private Type createSyntheticType(RelRecordType type) {
"Record" + type.getFieldCount() + "_" + syntheticTypes.size();
final SyntheticRecordType syntheticType =
new SyntheticRecordType(type, name);
- for (final RelDataTypeField recordField : type.getFieldList()) {
+ for (final Ord<RelDataTypeField> ord : Ord.zip(type.getFieldList())) {
+ final RelDataTypeField recordField = ord.e;
final Type fieldClass = getJavaClass(recordField.getType());
// A field whose type has no real Java class is stored as Object[] at
// runtime, like all rows in enumerable convention. For example, the
@@ -395,10 +396,17 @@ private Type createSyntheticType(RelRecordType type) {
final Type javaClass = fieldClass instanceof Class
? fieldClass
: Object[].class;
+ // Prefer the SQL field name to allow downstream reflection-based lookups
+ // (e.g. Avatica's Meta.CursorFactory.record(), which reads results
+ // out of the synthetic class by SQL column name); fall back to a
+ // positional name if the SQL name is not a legal Java identifier
+ final String rawName = recordField.getName();
+ final String fieldName =
+ Types.isValidJavaIdentifier(rawName) ? rawName : "f" + ord.i;
syntheticType.fields.add(
new RecordFieldImpl(
syntheticType,
- recordField.getName(),
+ fieldName,
javaClass,
recordField.getType().isNullable()
&& !Primitive.is(javaClass),
diff --git
a/core/src/test/java/org/apache/calcite/jdbc/SyntheticRecordFieldNameTest.java
b/core/src/test/java/org/apache/calcite/jdbc/SyntheticRecordFieldNameTest.java
new file mode 100644
index 0000000000..3ae027e919
--- /dev/null
+++
b/core/src/test/java/org/apache/calcite/jdbc/SyntheticRecordFieldNameTest.java
@@ -0,0 +1,109 @@
+/*
+ * 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.jdbc;
+
+import org.apache.calcite.linq4j.tree.Types;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.type.SqlTypeName;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.lang.reflect.Type;
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasSize;
+
+/**
+ * Tests for the Java field names of the synthetic classes built by
+ * {@link JavaTypeFactoryImpl} from a {@code RelRecordType}.
+ *
+ * <p>SQL quoted identifiers admit characters that are not legal in a
+ * Java identifier. When a SQL field name is a valid Java identifier it
+ * is reused as the field name of the synthetic class; otherwise the
+ * factory falls back to a positional name ({@code f0}, {@code f1}, ...).
+ * The original SQL names are preserved on the {@link RelDataType}
+ * regardless.
+ */
+public class SyntheticRecordFieldNameTest {
+
+ @Test void testValidSqlNamesReusedAsJavaNames() {
+ final JavaTypeFactoryImpl typeFactory = new JavaTypeFactoryImpl();
+ final RelDataType rowType = typeFactory.builder()
+ .add("empid", SqlTypeName.INTEGER)
+ .add("name", SqlTypeName.VARCHAR)
+ .add("deptno", SqlTypeName.INTEGER)
+ .build();
+ final Type javaType = typeFactory.getJavaClass(rowType);
+ assertThat(javaType, instanceOf(Types.RecordType.class));
+ final List<Types.RecordField> fields =
+ ((Types.RecordType) javaType).getRecordFields();
+ assertThat(fields, hasSize(3));
+ assertThat(fields.get(0).getName(), is("empid"));
+ assertThat(fields.get(1).getName(), is("name"));
+ assertThat(fields.get(2).getName(), is("deptno"));
+ }
+
+ @Test void testNonIdentifierSqlNamesFallBackToPositional() {
+ final JavaTypeFactoryImpl typeFactory = new JavaTypeFactoryImpl();
+ final RelDataType rowType = typeFactory.builder()
+ .add("has space", SqlTypeName.INTEGER)
+ .add("a.b", SqlTypeName.VARCHAR)
+ .add("ok", SqlTypeName.INTEGER)
+ .build();
+ final Type javaType = typeFactory.getJavaClass(rowType);
+ assertThat(javaType, instanceOf(Types.RecordType.class));
+ final List<Types.RecordField> fields =
+ ((Types.RecordType) javaType).getRecordFields();
+ assertThat(fields, hasSize(3));
+ // The first two SQL names are not legal Java identifiers, so they
+ // are replaced by positional names; the third one is fine and is
+ // reused verbatim
+ assertThat(fields.get(0).getName(), is("f0"));
+ assertThat(fields.get(1).getName(), is("f1"));
+ assertThat(fields.get(2).getName(), is("ok"));
+ for (Types.RecordField f : fields) {
+ assertThat("field names must be valid Java identifiers",
+ Types.isValidJavaIdentifier(f.getName()), is(true));
+ }
+ // The SQL names survive on the relational rowtype, which is where
+ // column labels come from
+ final JavaTypeFactoryImpl.SyntheticRecordType syntheticType =
+ (JavaTypeFactoryImpl.SyntheticRecordType) javaType;
+ assertThat(syntheticType.relType, is(rowType));
+ assertThat(rowType.getFieldNames().get(0), is("has space"));
+ assertThat(rowType.getFieldNames().get(1), is("a.b"));
+ assertThat(rowType.getFieldNames().get(2), is("ok"));
+ }
+
+ @Test void testListOverloadStillUsesPositionalNames() {
+ // The createSyntheticType(List<Type> l) overload produces f0..fn
+ // (the caller supplies no names)
+ final JavaTypeFactoryImpl typeFactory = new JavaTypeFactoryImpl();
+ final Type fromTypes =
+ typeFactory.createSyntheticType(ImmutableList.of(Integer.class,
String.class));
+ final List<Types.RecordField> fields =
+ ((Types.RecordType) fromTypes).getRecordFields();
+ assertThat(fields, hasSize(2));
+ assertThat(fields.get(0).getName(), is("f0"));
+ assertThat(fields.get(1).getName(), is("f1"));
+ }
+}
diff --git
a/core/src/test/java/org/apache/calcite/test/WindowPartitionAliasTest.java
b/core/src/test/java/org/apache/calcite/test/WindowPartitionAliasTest.java
new file mode 100644
index 0000000000..05c18555c8
--- /dev/null
+++ b/core/src/test/java/org/apache/calcite/test/WindowPartitionAliasTest.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.test;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for column aliases used as {@code PARTITION BY} keys of a window
+ * aggregate.
+ *
+ * <p>SQL allows quoted identifiers to contain almost any character;
+ * however, the enumerable convention materialises multi-column
+ * {@code PARTITION BY} keys as fields of a synthetic Java class emitted
+ * into generated source. Field names that are not valid Java identifiers
+ * are transparently renamed to positional placeholders
+ * ({@code f0}, {@code f1}, ...) inside the synthetic class, while the
+ * original SQL name is preserved at the rowtype level for outer
+ * references.
+ */
+class WindowPartitionAliasTest {
+
+ /** A quoted alias that is a valid Java identifier plans and runs
+ * normally when used as one of several {@code PARTITION BY} keys. The
+ * projection wraps the aliased column in a computation so field-
+ * trimming does not fold the sub-query away. */
+ @Test void testValidIdentifierAlias() {
+ final String sql = "select \"aliased_deptno\","
+ + " count(*) over ("
+ + " partition by \"aliased_deptno\", \"empid\") as c"
+ + " from ("
+ + " select \"deptno\" + 1 as \"aliased_deptno\", \"empid\""
+ + " from \"hr\".\"emps\")";
+ CalciteAssert.hr()
+ .query(sql)
+ .runs();
+ }
+
+ /** A quoted alias containing a space (a legal SQL identifier character
+ * that is not a legal Java identifier character) still runs: the
+ * synthetic partition-key class carries a positional field name while
+ * the outer projection continues to see the SQL alias. */
+ @Test void testAliasWithSpaceRuns() {
+ final String sql = "select \"has space\","
+ + " count(*) over ("
+ + " partition by \"has space\", \"empid\") as c"
+ + " from ("
+ + " select \"deptno\" + 1 as \"has space\", \"empid\""
+ + " from \"hr\".\"emps\")";
+ CalciteAssert.hr()
+ .query(sql)
+ .runs();
+ }
+
+ /** Same shape as above, with a punctuation character. */
+ @Test void testAliasWithPunctuationRuns() {
+ final String sql = "select \"a.b\","
+ + " count(*) over ("
+ + " partition by \"a.b\", \"empid\") as c"
+ + " from ("
+ + " select \"deptno\" + 1 as \"a.b\", \"empid\""
+ + " from \"hr\".\"emps\")";
+ CalciteAssert.hr()
+ .query(sql)
+ .runs();
+ }
+}
diff --git
a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MemberExpression.java
b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MemberExpression.java
index 8c4d37d7dc..54d0d73618 100644
--- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MemberExpression.java
+++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MemberExpression.java
@@ -22,6 +22,8 @@
import java.lang.reflect.Modifier;
import java.util.Objects;
+import static com.google.common.base.Preconditions.checkArgument;
+
import static java.util.Objects.requireNonNull;
/**
@@ -39,6 +41,9 @@ public MemberExpression(@Nullable Expression expression,
PseudoField field) {
super(ExpressionType.MemberAccess, field.getType());
this.expression = expression;
this.field = requireNonNull(field, "field");
+ checkArgument(Types.isValidJavaIdentifier(field.getName()),
+ "field name should be a valid java identifier: %s",
+ field.getName());
if (!Modifier.isStatic(field.getModifiers())) {
requireNonNull(expression,
"must specify expression if field is not static");
diff --git
a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ParameterExpression.java
b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ParameterExpression.java
index 17e95171b6..9e3c05d982 100644
---
a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ParameterExpression.java
+++
b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ParameterExpression.java
@@ -41,12 +41,11 @@ public ParameterExpression(Type type) {
public ParameterExpression(int modifier, Type type, String name) {
super(ExpressionType.Parameter, type);
- checkArgument(Character.isJavaIdentifierStart(name.charAt(0)),
- "parameter name should be valid java identifier: %s. "
- + "The first character is invalid.",
+ checkArgument(Types.isValidJavaIdentifier(requireNonNull(name, "name")),
+ "parameter name should be a valid java identifier: %s",
name);
this.modifier = modifier;
- this.name = requireNonNull(name, "name");
+ this.name = name;
}
@Override public Expression accept(Shuttle shuttle) {
diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Types.java
b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Types.java
index 09d371216e..e3a2a8390a 100644
--- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Types.java
+++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Types.java
@@ -47,6 +47,21 @@
public abstract class Types {
private Types() {}
+ /**
+ * Returns whether {@code name} is a syntactically valid Java identifier.
+ */
+ public static boolean isValidJavaIdentifier(String name) {
+ if (name.isEmpty() || !Character.isJavaIdentifierStart(name.charAt(0))) {
+ return false;
+ }
+ for (int i = 1; i < name.length(); i++) {
+ if (!Character.isJavaIdentifierPart(name.charAt(i))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
/**
* Creates a type with generic parameters.
*/
diff --git
a/linq4j/src/test/java/org/apache/calcite/linq4j/tree/IdentifierValidationTest.java
b/linq4j/src/test/java/org/apache/calcite/linq4j/tree/IdentifierValidationTest.java
new file mode 100644
index 0000000000..3040e00f06
--- /dev/null
+++
b/linq4j/src/test/java/org/apache/calcite/linq4j/tree/IdentifierValidationTest.java
@@ -0,0 +1,113 @@
+/*
+ * 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.linq4j.tree;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+import org.junit.jupiter.api.Test;
+
+import java.lang.reflect.Modifier;
+import java.lang.reflect.Type;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * Tests for {@link Types#isValidJavaIdentifier} and for the constructor
+ * checks in the expression-tree nodes that emit an identifier into
+ * generated source: {@link ParameterExpression} (also used by
+ * {@link FieldDeclaration}) and {@link MemberExpression}.
+ */
+public class IdentifierValidationTest {
+ /** A representative bad name: starts with a legal identifier character
+ * but contains characters that {@code Character#isJavaIdentifierPart}
+ * rejects. Historically only the first character was checked. */
+ private static final String BAD_NAME = "a b.c";
+
+ @Test void testIsValidJavaIdentifier() {
+ assertThat(Types.isValidJavaIdentifier("a"), is(true));
+ assertThat(Types.isValidJavaIdentifier("f0"), is(true));
+ assertThat(Types.isValidJavaIdentifier("_a$b9"), is(true));
+ assertThat(Types.isValidJavaIdentifier(""), is(false));
+ assertThat(Types.isValidJavaIdentifier("9a"), is(false));
+ assertThat(Types.isValidJavaIdentifier("a b"), is(false));
+ assertThat(Types.isValidJavaIdentifier("a.b"), is(false));
+ assertThat(Types.isValidJavaIdentifier("a\nb"), is(false));
+ assertThat(Types.isValidJavaIdentifier(BAD_NAME), is(false));
+ // First-character-only validation would accept this; the full check must
reject it
+ assertThat(Types.isValidJavaIdentifier("ok name"), is(false));
+ }
+
+ @Test void testParameterExpressionRejectsNonIdentifier() {
+ // Valid names are accepted
+ ParameterExpression p =
+ new ParameterExpression(0, int.class, "p0");
+ assertThat(p.name, is("p0"));
+ assertThrows(IllegalArgumentException.class, () ->
+ new ParameterExpression(0, int.class, BAD_NAME));
+ assertThrows(IllegalArgumentException.class, () ->
+ new ParameterExpression(0, int.class, "ok name"));
+ }
+
+ @Test void testFieldDeclarationCoveredViaParameterExpression() {
+ // FieldDeclaration emits parameter.name as a field name; it takes a
+ // ParameterExpression, so the constructor check above covers it
+ assertThrows(IllegalArgumentException.class, () ->
+ new FieldDeclaration(Modifier.PUBLIC,
+ new ParameterExpression(0, int.class, BAD_NAME), null));
+ }
+
+ @Test void testMemberExpressionRejectsNonIdentifierFieldName() {
+ // MemberExpression takes a PseudoField and emits field.getName()
+ // directly; it does not go through ParameterExpression
+ assertThrows(IllegalArgumentException.class, () ->
+ new MemberExpression(null, new NamedStaticField(BAD_NAME)));
+ // Sane names still work
+ MemberExpression m =
+ new MemberExpression(null, new NamedStaticField("f0"));
+ assertThat(m.field.getName(), is("f0"));
+ }
+
+ /** A synthetic static field with an arbitrary name. */
+ private static class NamedStaticField implements PseudoField {
+ private final String name;
+
+ NamedStaticField(String name) {
+ this.name = name;
+ }
+
+ @Override public String getName() {
+ return name;
+ }
+
+ @Override public Type getType() {
+ return int.class;
+ }
+
+ @Override public int getModifiers() {
+ return Modifier.PUBLIC | Modifier.STATIC;
+ }
+
+ @Override public @Nullable Object get(@Nullable Object o) {
+ return 0;
+ }
+
+ @Override public Type getDeclaringClass() {
+ return Object.class;
+ }
+ }
+}
