Skip to content

Commit

Permalink
Allow implicit casts from TEXT to OBJECT again
Browse files Browse the repository at this point in the history
This fixes a regression that causes statements like the following to fail:

    INSERT INTO tbl (obj) (SELECT * FROM unnest([<jsonStrLiteral>]))

Columns of type `OBJECT` were extended to have the `innerTypes` set in
more places, this caused
`stringType.isConvertibleTo(objectTypeWithInnerTypes)` to return false
because `ALLOWED_CONVERSIONS` only contained an untyped object.
  • Loading branch information
mfussenegger authored and mergify[bot] committed Feb 13, 2020
1 parent 710a402 commit 846607c
Show file tree
Hide file tree
Showing 8 changed files with 54 additions and 27 deletions.
6 changes: 3 additions & 3 deletions common/src/main/java/io/crate/types/DataType.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,16 @@ public boolean precedes(DataType other) {
* @param other the DataType to check conversion to
* @return true or false
*/
public boolean isConvertableTo(DataType other) {
public boolean isConvertableTo(DataType<?> other) {
if (this.equals(other)) {
return true;
}
Set<DataType> possibleConversions = DataTypes.ALLOWED_CONVERSIONS.get(id());
Set<Integer> possibleConversions = DataTypes.ALLOWED_CONVERSIONS.get(id());
//noinspection SimplifiableIfStatement
if (possibleConversions == null) {
return false;
}
return possibleConversions.contains(other);
return possibleConversions.contains(other.id());
}

static <T> int nullSafeCompareValueTo(T val1, T val2, Comparator<T> cmp) {
Expand Down
25 changes: 13 additions & 12 deletions common/src/main/java/io/crate/types/DataTypes.java
Original file line number Diff line number Diff line change
Expand Up @@ -139,33 +139,34 @@ public final class DataTypes {
entry(RowType.ID, RowType::new))
);

private static final Set<DataType> NUMBER_CONVERSIONS = Stream.concat(
private static final Set<Integer> NUMBER_CONVERSIONS = Stream.concat(
Stream.of(BOOLEAN, STRING, TIMESTAMPZ, TIMESTAMP, IP),
NUMERIC_PRIMITIVE_TYPES.stream()
).collect(toSet());
).map(DataType::id).collect(toSet());

// allowed conversion from key to one of the value types
// the key type itself does not need to be in the value set
static final Map<Integer, Set<DataType>> ALLOWED_CONVERSIONS = Map.ofEntries(
static final Map<Integer, Set<Integer>> ALLOWED_CONVERSIONS = Map.ofEntries(
entry(BYTE.id(), NUMBER_CONVERSIONS),
entry(SHORT.id(), NUMBER_CONVERSIONS),
entry(INTEGER.id(), NUMBER_CONVERSIONS),
entry(LONG.id(), NUMBER_CONVERSIONS),
entry(FLOAT.id(), NUMBER_CONVERSIONS),
entry(DOUBLE.id(), NUMBER_CONVERSIONS),
entry(BOOLEAN.id(), Set.of(STRING)),
entry(BOOLEAN.id(), Set.of(STRING.id())),
entry(STRING.id(), Stream.concat(
Stream.of(GEO_SHAPE, GEO_POINT, ObjectType.untyped()),
Stream.of(GEO_SHAPE.id(), GEO_POINT.id(), ObjectType.ID),
NUMBER_CONVERSIONS.stream()
).collect(toSet())),
entry(IP.id(), Set.of(STRING)),
entry(TIMESTAMPZ.id(), Set.of(DOUBLE, LONG, STRING, TIMESTAMP)),
entry(TIMESTAMP.id(), Set.of(DOUBLE, LONG, STRING, TIMESTAMPZ)),
entry(IP.id(), Set.of(STRING.id())),
entry(TIMESTAMPZ.id(), Set.of(DOUBLE.id(), LONG.id(), STRING.id(), TIMESTAMP.id())),
entry(TIMESTAMP.id(), Set.of(DOUBLE.id(), LONG.id(), STRING.id(), TIMESTAMPZ.id())),
entry(UNDEFINED.id(), Set.of()), // actually convertible to every type, see NullType
entry(GEO_POINT.id(), Set.of(new ArrayType<>(DOUBLE))),
entry(GEO_SHAPE.id(), Set.of(ObjectType.untyped())),
entry(ObjectType.ID, Set.of(GEO_SHAPE)),
entry(ArrayType.ID, Set.of())); // convertability handled in ArrayType
entry(GEO_POINT.id(), Set.of()),
entry(GEO_SHAPE.id(), Set.of(ObjectType.ID)),
entry(ObjectType.ID, Set.of(GEO_SHAPE.id())),
entry(ArrayType.ID, Set.of()) // convertability handled in ArrayType
);

/**
* Contains number conversions which are "safe" (= a conversion would not reduce the number of bytes
Expand Down
5 changes: 5 additions & 0 deletions common/src/main/java/io/crate/types/GeoPointType.java
Original file line number Diff line number Diff line change
Expand Up @@ -170,4 +170,9 @@ public void writeValueTo(StreamOutput out, Point point) throws IOException {
public int fixedSize() {
return 40; // 2x double + array overhead
}

@Override
public boolean isConvertableTo(DataType<?> other) {
return other.id() == id() || other instanceof ArrayType && ((ArrayType<?>) other).innerType().equals(DataTypes.DOUBLE);
}
}
6 changes: 3 additions & 3 deletions common/src/main/java/io/crate/types/ObjectType.java
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,9 @@ public Map<String, Object> readValueFrom(StreamInput in) throws IOException {
}

@Override
public boolean isConvertableTo(DataType o) {
Set<DataType> conversions = ALLOWED_CONVERSIONS.getOrDefault(id(), Set.of());
if (conversions.contains(o)) {
public boolean isConvertableTo(DataType<?> o) {
Set<Integer> conversions = ALLOWED_CONVERSIONS.getOrDefault(id(), Set.of());
if (conversions.contains(o.id())) {
return true;
}

Expand Down
19 changes: 12 additions & 7 deletions common/src/test/java/io/crate/types/TypeConversionTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ public Integer call() throws Exception {

@Test
public void numberConversionTest() throws Exception {

for (Byte byteVal : bytes(10)) {
for (DataType t : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.BYTE.id())) {
for (int id : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.BYTE.id())) {
var t = DataTypes.fromId(id);
if (t.equals(DataTypes.IP)) {
byteVal = (byte) Math.abs(byteVal == Byte.MIN_VALUE ? byteVal >> 1 : byteVal);
}
Expand All @@ -103,35 +103,40 @@ public void numberConversionTest() throws Exception {
}

for (Integer shortVal : integers((int) Byte.MIN_VALUE, (int) Byte.MAX_VALUE, 10)) {
for (DataType t : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.SHORT.id())) {
for (int id : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.SHORT.id())) {
var t = DataTypes.fromId(id);
shortVal = t.equals(DataTypes.IP) ? Math.abs(shortVal) : shortVal;
t.value(shortVal.shortValue());
}
}

for (Integer intValue : integers((int) Byte.MIN_VALUE, (int) Byte.MAX_VALUE, 10)) {
for (DataType t : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.INTEGER.id())) {
for (int id : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.INTEGER.id())) {
var t = DataTypes.fromId(id);
intValue = t.equals(DataTypes.IP) ? Math.abs(intValue) : intValue;
t.value(intValue);
}
}

for (Integer longValue : integers((int) Byte.MIN_VALUE, (int) Byte.MAX_VALUE, 10)) {
for (DataType t : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.LONG.id())) {
for (int id : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.LONG.id())) {
var t = DataTypes.fromId(id);
longValue = t.equals(DataTypes.IP) ? Math.abs(longValue) : longValue;
t.value(longValue.longValue());
}
}

for (Integer floatValue : integers((int) Byte.MIN_VALUE, (int) Byte.MAX_VALUE, 10)) {
for (DataType t : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.FLOAT.id())) {
for (int id : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.FLOAT.id())) {
var t = DataTypes.fromId(id);
floatValue = t.equals(DataTypes.IP) ? Math.abs(floatValue) : floatValue;
t.value(floatValue.floatValue());
}
}

for (Integer doubleValue : integers((int) Byte.MIN_VALUE, (int) Byte.MAX_VALUE, 10)) {
for (DataType t : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.DOUBLE.id())) {
for (int id : DataTypes.ALLOWED_CONVERSIONS.get(DataTypes.DOUBLE.id())) {
var t = DataTypes.fromId(id);
doubleValue = t.equals(DataTypes.IP) ? Math.abs(doubleValue) : doubleValue;
t.value(doubleValue.doubleValue());
}
Expand Down
5 changes: 4 additions & 1 deletion docs/appendices/release-notes/unreleased.rst
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ Changes
Fixes
=====

- Fixed a regression that caused ``INSERT INTO`` statements containing an
``object`` column as target and a matching JSON string literal in the source
to fail with a type cast error.

- Fixed an issue which may result in showing the CE admin-ui view even if
running in Enterprise mode on early node startup.

2 changes: 1 addition & 1 deletion sql/src/main/java/io/crate/analyze/InsertAnalyzer.java
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ private static void checkSourceAndTargetColsForLengthAndTypesCompatibility(
for (int i = 0; i < targetColumns.size(); i++) {
Reference targetCol = targetColumns.get(i);
Symbol source = sources.get(i);
DataType targetType = targetCol.valueType();
DataType<?> targetType = targetCol.valueType();
if (targetType.id() == DataTypes.UNDEFINED.id() || source.valueType().isConvertableTo(targetType)) {
continue;
}
Expand Down
13 changes: 13 additions & 0 deletions sql/src/test/java/io/crate/analyze/InsertAnalyzerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import io.crate.sql.parser.ParsingException;
import io.crate.test.integration.CrateDummyClusterServiceUnitTest;
import io.crate.testing.SQLExecutor;
import io.crate.types.DataTypes;
import io.crate.types.StringType;
import org.hamcrest.core.Is;
import org.junit.Assert;
Expand All @@ -42,10 +43,12 @@
import java.util.List;
import java.util.Map;

import static io.crate.testing.SymbolMatchers.isField;
import static io.crate.testing.SymbolMatchers.isFunction;
import static io.crate.testing.SymbolMatchers.isInputColumn;
import static io.crate.testing.SymbolMatchers.isLiteral;
import static io.crate.testing.SymbolMatchers.isReference;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.instanceOf;
Expand Down Expand Up @@ -358,4 +361,14 @@ public void test_insert_from_query_fails_when_source_and_target_types_are_incomp
"is not convertible to the type 'object' of target column 'details'");
e.analyze("insert into users (id, name, details) (select id, name, id from users)");
}

@Test
public void test_unnest_with_json_str_can_insert_into_object_column() {
AnalyzedInsertStatement stmt = e.analyze(
"insert into users (id, address) (select * from unnest([1], ['{\"postcode\":12345}']))");
assertThat(stmt.subQueryRelation().fields(), contains(
isField("col1"),
isField("col2", DataTypes.STRING) // Planner adds a cast projection; text is okay here
));
}
}

0 comments on commit 846607c

Please sign in to comment.