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

[CALCITE-6764] Field access from a nullable ROW should be nullable #4127

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.calcite.sql.type.MultisetSqlType;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.SqlTypeUtil;
import org.apache.calcite.tools.RelBuilder;
import org.apache.calcite.tools.RelBuilderFactory;
import org.apache.calcite.util.ImmutableBitSet;
Expand Down Expand Up @@ -2222,6 +2223,45 @@ public static boolean eq(
return litmus.succeed();
}

/**
* Returns whether two types are equal, perhaps ignoring nullability.
*
* @param ignoreNullability If true the types must be equal ignoring the (top-level) nullability.
* @param desc1 Description of first type
* @param type1 First type
* @param desc2 Description of second type
* @param type2 Second type
* @param litmus What to do if an error is detected (types are not equal)
* @return Whether the types are equal
*/
public static boolean eqUpToNullability(
boolean ignoreNullability,
final String desc1,
RelDataType type1,
final String desc2,
RelDataType type2,
Litmus litmus) {
// if any one of the types is ANY return true
if (type1.getSqlTypeName() == SqlTypeName.ANY
|| type2.getSqlTypeName() == SqlTypeName.ANY) {
return litmus.succeed();
}

boolean success;
if (ignoreNullability) {
success = SqlTypeUtil.equalSansNullability(type1, type2);
} else {
success = type1.equals(type2);
}

if (!success) {
return litmus.fail("type mismatch:\n{}:\n{}\n{}:\n{}",
desc1, type1.getFullTypeString(),
desc2, type2.getFullTypeString());
}
return litmus.succeed();
}

/**
* Returns whether two types are equal using
* {@link #areRowTypesEqual(RelDataType, RelDataType, boolean)}. Both types
Expand Down
11 changes: 8 additions & 3 deletions core/src/main/java/org/apache/calcite/rex/RexBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -240,21 +240,26 @@ public RexNode makeFieldAccess(
private RexNode makeFieldAccessInternal(
RexNode expr,
final RelDataTypeField field) {
RelDataType fieldType = field.getType();
if (expr instanceof RexRangeRef) {
RexRangeRef range = (RexRangeRef) expr;
if (field.getIndex() < 0) {
return makeCall(
field.getType(),
fieldType,
GET_OPERATOR,
ImmutableList.of(
expr,
makeLiteral(field.getName())));
}
return new RexInputRef(
range.getOffset() + field.getIndex(),
field.getType());
fieldType);
}
return new RexFieldAccess(expr, field);

if (expr.getType().isNullable()) {
fieldType = typeFactory.createTypeWithNullability(fieldType, true);
}
return new RexFieldAccess(expr, field, fieldType);
}

/**
Expand Down
11 changes: 8 additions & 3 deletions core/src/main/java/org/apache/calcite/rex/RexChecker.java
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,12 @@ public int getFailureCount() {
return litmus.fail("RexInputRef index {} out of range 0..{}",
index, inputTypeList.size() - 1);
}
// Type of field and type of result can differ in nullability. See [CALCITE-6764]
if (!ref.getType().isStruct()
&& !RelOptUtil.eq("ref", ref.getType(), "input",
inputTypeList.get(index), litmus)) {
&& !RelOptUtil.eqUpToNullability(
ref.getType().isNullable(),
"ref", ref.getType(), "input",
inputTypeList.get(index), litmus)) {
++failCount;
return litmus.fail(null);
}
Expand Down Expand Up @@ -160,8 +163,10 @@ public int getFailureCount() {
++failCount;
return litmus.fail(null);
}
// Type of field may not match type of field access - they may differ in nullability
final RelDataTypeField typeField = refType.getFieldList().get(index);
NobiGo marked this conversation as resolved.
Show resolved Hide resolved
if (!RelOptUtil.eq(
if (!RelOptUtil.eqUpToNullability(
refType.isNullable(),
"type1",
typeField.getType(),
"type2",
Expand Down
12 changes: 11 additions & 1 deletion core/src/main/java/org/apache/calcite/rex/RexFieldAccess.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,26 @@ public class RexFieldAccess extends RexNode {

private final RexNode expr;
private final RelDataTypeField field;
// Not always the same as the field.getType().
private final RelDataType type;

//~ Constructors -----------------------------------------------------------

RexFieldAccess(
RexNode expr,
RelDataTypeField field) {
this(expr, field, field.getType());
}

RexFieldAccess(
RexNode expr,
RelDataTypeField field,
RelDataType type) {
checkValid(expr, field);
this.expr = expr;
this.field = field;
this.digest = expr + "." + field.getName();
this.type = type;
}

//~ Methods ----------------------------------------------------------------
Expand All @@ -82,7 +92,7 @@ public RelDataTypeField getField() {
}

@Override public RelDataType getType() {
return field.getType();
return type;
}

@Override public SqlKind getKind() {
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/org/apache/calcite/rex/RexProgram.java
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,8 @@ private class Marshaller extends RexVisitorImpl<@Nullable RexNode> {
fieldAccess.getReferenceExpr().accept(this);
return new RexFieldAccess(
requireNonNull(referenceExpr, "referenceExpr must not be null"),
fieldAccess.getField());
fieldAccess.getField(),
fieldAccess.getType());
}
}

Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/org/apache/calcite/rex/RexShuttle.java
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ protected List<RexFieldCollation> visitFieldCollations(
} else {
return new RexFieldAccess(
after,
fieldAccess.getField());
fieldAccess.getField(),
fieldAccess.getType());
}
}

Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/org/apache/calcite/rex/RexUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -2470,7 +2470,8 @@ protected RexNode lookup(RexNode expr) {
fieldAccess =
new RexFieldAccess(
normalizedExpr,
fieldAccess.getField());
fieldAccess.getField(),
fieldAccess.getType());
}
return register(fieldAccess);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,23 @@
*/
package org.apache.calcite.test;

import org.apache.calcite.adapter.java.AbstractQueryableTable;
import org.apache.calcite.jdbc.CalciteConnection;
import org.apache.calcite.linq4j.Enumerator;
import org.apache.calcite.linq4j.QueryProvider;
import org.apache.calcite.linq4j.Queryable;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.rel.type.RelDataTypeFieldImpl;
import org.apache.calcite.rel.type.RelRecordType;
import org.apache.calcite.rel.type.StructKind;
import org.apache.calcite.runtime.PairList;
import org.apache.calcite.schema.SchemaPlus;
import org.apache.calcite.schema.impl.AbstractTableQueryable;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.util.Smalls;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMultiset;

import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -73,4 +87,63 @@ class TableInRootSchemaTest {
connection.close();
}

/**
* Helper class for the test for [CALCITE-6764] below.
*/
private static class RowTable extends AbstractQueryableTable {
protected RowTable() {
super(Object[].class);
}

@Override public RelDataType getRowType(RelDataTypeFactory typeFactory) {
final PairList<String, RelDataType> columnDesc = PairList.withCapacity(1);
// Schema contains a column whose type is MAP<VARCHAR, ROW(VARCHAR)>, but
// the ROW type can be nullable.
final RelDataType colType =
typeFactory.createMapType(typeFactory.createSqlType(SqlTypeName.VARCHAR),
new RelRecordType(
StructKind.PEEK_FIELDS,
ImmutableList.of(
new RelDataTypeFieldImpl("K", 0,
typeFactory.createSqlType(SqlTypeName.VARCHAR))), true));
columnDesc.add("P", colType);
return typeFactory.createStructType(columnDesc);
}

@Override public <T> Queryable<T> asQueryable(
QueryProvider queryProvider, SchemaPlus schema, String tableName) {
return new AbstractTableQueryable<T>(queryProvider, schema, this,
tableName) {
@Override public Enumerator<T> enumerator() {
return new Enumerator<T>() {
@Override public T current() {
return null;
}

@Override public boolean moveNext() {
// Table is empty
return false;
}

@Override public void reset() {}

@Override public void close() {}
};
}
};
}
}

/** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6764">[CALCITE-6764]
* Field access from a nullable ROW should be nullable</a>. */
@Test void testNullableValue() throws Exception {
Connection connection = DriverManager.getConnection("jdbc:calcite:");
CalciteConnection calciteConnection = connection.unwrap(CalciteConnection.class);
calciteConnection.getRootSchema().add("T", new RowTable());
Statement statement = calciteConnection.createStatement();
ResultSet resultSet = statement.executeQuery("SELECT P['a'].K FROM T");
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
resultSet.close();
statement.close();
connection.close();
}
}
Loading