Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[CALCITE-5253] NATURAL join and USING should fail if join columns are…
… not unique - expression validation partially broken
  • Loading branch information
zstan authored and asolimando committed Oct 5, 2022
1 parent c2407f5 commit ab296ac
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 25 deletions.
11 changes: 10 additions & 1 deletion core/src/main/java/org/apache/calcite/rel/type/RelCrossType.java
Expand Up @@ -31,7 +31,7 @@
public class RelCrossType extends RelDataTypeImpl {
//~ Instance fields --------------------------------------------------------

public final ImmutableList<RelDataType> types;
private final ImmutableList<RelDataType> types;

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

Expand All @@ -58,6 +58,15 @@ public RelCrossType(
return false;
}

/**
* Returns the contained types.
*
* @return data types.
*/
public List<RelDataType> getTypes() {
return types;
}

@Override protected void generateTypeString(StringBuilder sb, boolean withDetail) {
sb.append("CrossType(");
for (Ord<RelDataType> type : Ord.zip(types)) {
Expand Down
Expand Up @@ -449,11 +449,11 @@ private static List<RelDataTypeField> getFieldList(List<RelDataType> types) {
* Returns a list of all atomic types in a list.
*/
private static void getTypeList(
ImmutableList<RelDataType> inTypes,
List<RelDataType> inTypes,
List<RelDataType> flatTypes) {
for (RelDataType inType : inTypes) {
if (inType instanceof RelCrossType) {
getTypeList(((RelCrossType) inType).types, flatTypes);
getTypeList(((RelCrossType) inType).getTypes(), flatTypes);
} else {
flatTypes.add(inType);
}
Expand All @@ -470,7 +470,7 @@ private static void addFields(
List<RelDataTypeField> fieldList) {
if (type instanceof RelCrossType) {
final RelCrossType crossType = (RelCrossType) type;
for (RelDataType type1 : crossType.types) {
for (RelDataType type1 : crossType.getTypes()) {
addFields(type1, fieldList);
}
} else {
Expand Down
Expand Up @@ -22,6 +22,7 @@
import org.apache.calcite.plan.RelOptUtil;
import org.apache.calcite.prepare.Prepare;
import org.apache.calcite.rel.type.DynamicRecordType;
import org.apache.calcite.rel.type.RelCrossType;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.rel.type.RelDataTypeField;
Expand Down Expand Up @@ -3500,9 +3501,9 @@ protected void validateJoin(SqlJoin join, SqlValidatorScope scope) {
(List) getCondition(join);

// Parser ensures that using clause is not empty.
Preconditions.checkArgument(list.size() > 0, "Empty USING clause");
Preconditions.checkArgument(!list.isEmpty(), "Empty USING clause");
for (SqlIdentifier id : list) {
validateCommonJoinColumn(id, left, right, scope);
validateCommonJoinColumn(id, left, right, scope, natural);
}
break;
default:
Expand All @@ -3521,7 +3522,7 @@ protected void validateJoin(SqlJoin join, SqlValidatorScope scope) {
for (String name : deriveNaturalJoinColumnList(join)) {
final SqlIdentifier id =
new SqlIdentifier(name, join.isNaturalNode().getParserPosition());
validateCommonJoinColumn(id, left, right, scope);
validateCommonJoinColumn(id, left, right, scope, natural);
}
}

Expand Down Expand Up @@ -3585,27 +3586,39 @@ private void validateNoAggs(AggFinder aggFinder, SqlNode node,
}
}

/** Validates a column in a USING clause, or an inferred join key in a
* NATURAL join. */
/** Validates a column in a USING clause, or an inferred join key in a NATURAL join. */
private void validateCommonJoinColumn(SqlIdentifier id, SqlNode left,
SqlNode right, SqlValidatorScope scope) {
SqlNode right, SqlValidatorScope scope, boolean natural) {
if (id.names.size() != 1) {
throw newValidationError(id, RESOURCE.columnNotFound(id.toString()));
}

final RelDataType leftColType = validateCommonInputJoinColumn(id, left, scope);
final RelDataType rightColType = validateCommonInputJoinColumn(id, right, scope);
final RelDataType leftColType = natural
? checkAndDeriveDataType(id, left)
: validateCommonInputJoinColumn(id, left, scope, natural);
final RelDataType rightColType = validateCommonInputJoinColumn(id, right, scope, natural);
if (!SqlTypeUtil.isComparable(leftColType, rightColType)) {
throw newValidationError(id,
RESOURCE.naturalOrUsingColumnNotCompatible(id.getSimple(),
leftColType.toString(), rightColType.toString()));
}
}

private RelDataType checkAndDeriveDataType(SqlIdentifier id, SqlNode node) {
Preconditions.checkArgument(id.names.size() == 1);
String name = id.names.get(0);
SqlNameMatcher nameMatcher = getCatalogReader().nameMatcher();
RelDataType rowType = getNamespaceOrThrow(node).getRowType();
RelDataType colType = requireNonNull(
nameMatcher.field(rowType, name),
() -> "unable to find left field " + name + " in " + rowType).getType();
return colType;
}

/** Validates a column in a USING clause, or an inferred join key in a
* NATURAL join, in the left or right input to the join. */
private RelDataType validateCommonInputJoinColumn(SqlIdentifier id,
SqlNode leftOrRight, SqlValidatorScope scope) {
SqlNode leftOrRight, SqlValidatorScope scope, boolean natural) {
Preconditions.checkArgument(id.names.size() == 1);
final String name = id.names.get(0);
final SqlValidatorNamespace namespace = getNamespaceOrThrow(leftOrRight);
Expand All @@ -3615,9 +3628,17 @@ private RelDataType validateCommonInputJoinColumn(SqlIdentifier id,
if (field == null) {
throw newValidationError(id, RESOURCE.columnNotFound(name));
}
if (nameMatcher.frequency(rowType.getFieldNames(), name) > 1) {
throw newValidationError(id,
RESOURCE.columnInUsingNotUnique(name));
Collection<RelDataType> rowTypes;
if (!natural && rowType instanceof RelCrossType) {
final RelCrossType crossType = (RelCrossType) rowType;
rowTypes = new ArrayList<>(crossType.getTypes());
} else {
rowTypes = Collections.singleton(rowType);
}
for (RelDataType rowType0 : rowTypes) {
if (nameMatcher.frequency(rowType0.getFieldNames(), name) > 1) {
throw newValidationError(id, RESOURCE.columnInUsingNotUnique(name));
}
}
checkRollUpInUsing(id, leftOrRight, scope);
return field.getType();
Expand Down
61 changes: 52 additions & 9 deletions core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
Expand Up @@ -63,6 +63,7 @@
import com.google.common.collect.Ordering;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -6163,7 +6164,7 @@ private ImmutableList<ImmutableBitSet> cube(ImmutableBitSet... sets) {
/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-5171">[CALCITE-5171]
* NATURAL join and USING should fail if join columns are not unique</a>. */
@Test void testNaturalJoinDuplicateColumns() {
@Test void testJoinDuplicateColumns() {
// NATURAL join and USING should fail if join columns are not unique
final String message = "Column name 'DEPTNO' in NATURAL join or "
+ "USING clause is not unique on one side of join";
Expand All @@ -6179,27 +6180,71 @@ private ImmutableList<ImmutableBitSet> cube(ImmutableBitSet... sets) {
+ " using (^deptno^)")
.fails(message);

// Reversed query gives reversed error message
sql("select e.ename, d.name\n"
+ "from (select ename, sal as deptno, deptno from emp) as e\n"
+ "join dept as d\n"
+ " using (^deptno^)")
.fails(message);

// Also with "*". (Proves that FROM is validated before SELECT.)
sql("select *\n"
+ "from emp\n"
+ "left join (select deptno, name as deptno from dept)\n"
+ " using (^deptno^)")
.fails(message);
}

// Reversed query gives reversed error message
sql("select e.ename, d.name\n"
+ "from (select ename, sal as deptno, deptno from emp) as e\n"
+ "join dept as d\n"
+ " using (^deptno^)")
@Test @DisplayName("Natural join require input column uniqueness")
void testNaturalJoinRequireInputColumnUniqueness() {
final String message = "Column name 'DEPTNO' in NATURAL join or "
+ "USING clause is not unique on one side of join";
// Invalid. NATURAL JOIN eliminates duplicate columns from its output but
// requires input columns to be unique.
sql("select *\n"
+ "from (emp as e cross join dept as d)\n"
+ "^natural^ join\n"
+ "(emp as e2 cross join dept as d2)")
.fails(message);
}

@Test @DisplayName("Should produce two DEPTNO columns")
void testReturnsCorrectRowTypeOnCombinedJoin() {
sql("select *\n"
+ "from emp as e\n"
+ "natural join dept as d\n"
+ "join (select deptno as x, deptno from dept) as d2"
+ " on d2.deptno = e.deptno")
.type("RecordType("
+ "INTEGER NOT NULL DEPTNO, "
+ "INTEGER NOT NULL EMPNO, "
+ "VARCHAR(20) NOT NULL ENAME, "
+ "VARCHAR(10) NOT NULL JOB, "
+ "INTEGER MGR, "
+ "TIMESTAMP(0) NOT NULL HIREDATE, "
+ "INTEGER NOT NULL SAL, "
+ "INTEGER NOT NULL COMM, "
+ "BOOLEAN NOT NULL SLACKER, "
+ "VARCHAR(10) NOT NULL NAME, "
+ "INTEGER NOT NULL X, "
+ "INTEGER NOT NULL DEPTNO1) NOT NULL");
}

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-5171">[CALCITE-5171]
* NATURAL join and USING should fail if join columns are not unique</a>. */
@Test void testCorrectJoinDuplicateColumns() {
// The error only occurs if the duplicate column is referenced. The
// following query has a duplicate hiredate column.
sql("select e.ename, d.name\n"
+ "from dept as d\n"
+ "join (select ename, sal as hiredate, deptno from emp) as e\n"
+ " using (deptno)")
.ok();

// Previous join chain does not affect validation.
sql("select * from EMP natural join EMPNULLABLES natural join DEPT")
.ok();
}

@Test void testNaturalEmptyKey() {
Expand Down Expand Up @@ -6347,9 +6392,7 @@ private ImmutableList<ImmutableBitSet> cube(ImmutableBitSet... sets) {
+ "from emp as e\n"
+ "join dept as d using (deptno)\n"
+ "join dept as d2 using (^deptno^)";
final String expected = "Column name 'DEPTNO' in NATURAL join or "
+ "USING clause is not unique on one side of join";
sql(sql1).fails(expected);
sql(sql1).ok();

final String sql2 = "select *\n"
+ "from emp as e\n"
Expand Down

0 comments on commit ab296ac

Please sign in to comment.