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.

Sign up for GitHub

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

Add validation to Insert statement parsing to match the column names with supplied values #3070

Merged
merged 7 commits into from
Feb 11, 2025
Merged

Add validation to Insert statement parsing to match the column names with supplied values #3070

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
596776c
fix INSERT statement cases
g31pranjal Jan 28, 2025
e5cd8c2
address comments
g31pranjal Jan 31, 2025
56f8551
address comments
g31pranjal Feb 7, 2025
87ee00b
Merge remote-tracking branch 'upstream/main' into insert_bug_fix
g31pranjal Feb 7, 2025
4626a81
address comments
g31pranjal Feb 7, 2025
eeb849a
address comments
g31pranjal Feb 11, 2025
6c95027
Merge remote-tracking branch 'upstream/main' into insert_bug_fix
g31pranjal Feb 11, 2025
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
5 changes: 4 additions & 1 deletion .../java/com/apple/foundationdb/relational/recordlayer/query/visitors/ExpressionVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,9 +768,12 @@ private Expressions parseRecordFieldsUnderReorderings(@Nonnull final List<? exte
for (final var elementField : elementFields) {
final int index = targetTypeReorderings.indexOf(elementField.getFieldName());
final var fieldType = elementField.getFieldType();
final Expression currentFieldColumns;
Expression currentFieldColumns = null;
if (index >= 0 && index < providedColumnContexts.size()) {
currentFieldColumns = parseRecordField(providedColumnContexts.get(index), elementField);
} else if (index >= providedColumnContexts.size()) {
// column is declared but the value is not provided
Assert.failUnchecked(ErrorCode.SYNTAX_ERROR, "Value of column \"" + elementField.getFieldName() + "\" is not provided");
} else {
// We do not yet support default values for any types, hence it makes to simply fail if the field type
// expects non-null but no value is provided.
Expand Down
70 changes: 70 additions & 0 deletions ...core/src/test/java/com/apple/foundationdb/relational/api/ddl/DdlStatementParsingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import javax.annotation.Nullable;
import java.net.URI;
import java.sql.SQLException;
import java.sql.Types;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -254,6 +255,75 @@ public ConstantAction getCreateSchemaTemplateConstantAction(@Nonnull SchemaTempl
});
}

private static Stream<Arguments> typesMap() {
return Stream.of(
Arguments.of(Types.INTEGER, "INTEGER"),
Arguments.of(Types.BIGINT, "BIGINT"),
Arguments.of(Types.FLOAT, "FLOAT"),
Arguments.of(Types.DOUBLE, "DOUBLE"),
Arguments.of(Types.VARCHAR, "STRING"),
Arguments.of(Types.BOOLEAN, "BOOLEAN"),
Arguments.of(Types.BINARY, "BYTES"),
Arguments.of(Types.STRUCT, "BAZ"),
Arguments.of(Types.ARRAY, "STRING ARRAY")
);
}

@ParameterizedTest
@MethodSource("typesMap")
void columnTypeWithNull(int sqlType, @Nonnull String sqlTypeName) throws Exception {
final String stmt = "CREATE SCHEMA TEMPLATE test_template " +
"CREATE TYPE AS STRUCT baz (a bigint, b bigint) " +
"CREATE TABLE bar (id bigint, foo_field " + sqlTypeName + " null, PRIMARY KEY(id))";
shouldWorkWithInjectedFactory(stmt, new AbstractMetadataOperationsFactory() {
@Nonnull
@Override
public ConstantAction getCreateSchemaTemplateConstantAction(@Nonnull final SchemaTemplate template, @Nonnull final Options templateProperties) {
checkColumnNullability(template, sqlType, true);
return txn -> {
};
}
});
}

@ParameterizedTest
@MethodSource("typesMap")
void columnTypeWithNotNull(int sqlType, @Nonnull String sqlTypeName) throws Exception {
final String stmt = "CREATE SCHEMA TEMPLATE test_template " +
"CREATE TYPE AS STRUCT baz (a bigint, b bigint) " +
"CREATE TABLE bar (id bigint, foo_field " + sqlTypeName + " not null, PRIMARY KEY(id))";
if (sqlType == Types.ARRAY) {
shouldWorkWithInjectedFactory(stmt, new AbstractMetadataOperationsFactory() {
@Nonnull
@Override
public ConstantAction getCreateSchemaTemplateConstantAction(@Nonnull final SchemaTemplate template, @Nonnull final Options templateProperties) {
checkColumnNullability(template, sqlType, false);
return txn -> {
};
}
});
} else {
shouldFailWith(stmt, ErrorCode.UNSUPPORTED_OPERATION);
}
}

private static void checkColumnNullability(@Nonnull final SchemaTemplate template, int sqlType, boolean isNullable) {
Assertions.assertInstanceOf(RecordLayerSchemaTemplate.class, template);
Assertions.assertEquals(1, ((RecordLayerSchemaTemplate) template).getTables().size(), "should have only 1 table");
final var table = ((RecordLayerSchemaTemplate) template).findTableByName("BAR");
Assertions.assertTrue(table.isPresent());
final var columns = table.get().getColumns();
Assertions.assertEquals(2, columns.size());
final var maybeNullableArrayColumn = columns.stream().filter(c -> c.getName().equals("FOO_FIELD")).findFirst();
Assertions.assertTrue(maybeNullableArrayColumn.isPresent());
if (isNullable) {
Assertions.assertTrue(maybeNullableArrayColumn.get().getDatatype().isNullable());
} else {
Assertions.assertFalse(maybeNullableArrayColumn.get().getDatatype().isNullable());
}
Assertions.assertEquals(sqlType, maybeNullableArrayColumn.get().getDatatype().getJdbcSqlCode());
}

@Test
void failsToParseEmptyTemplateStatements() throws Exception {
//empty template statements are invalid, and can be rejected in the parser
Expand Down
54 changes: 13 additions & 41 deletions ...core/src/test/java/com/apple/foundationdb/relational/recordlayer/RelationalArrayTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
import com.apple.foundationdb.relational.api.EmbeddedRelationalStruct;
import com.apple.foundationdb.relational.api.RelationalPreparedStatement;
import com.apple.foundationdb.relational.api.RelationalStruct;
import com.apple.foundationdb.relational.utils.RelationalAssertions;
import com.apple.foundationdb.relational.utils.RelationalStructAssert;
import com.apple.foundationdb.relational.utils.ResultSetAssert;
import com.apple.foundationdb.relational.utils.SimpleDatabaseRule;
import org.junit.Assert;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -71,7 +71,7 @@ public class RelationalArrayTest {
"double_null double array, double_not_null double array not null, " +
"string_null string array, string_not_null string array not null, " +
"bytes_null bytes array, bytes_not_null bytes array not null, " +
"structure_null structure array, structure_not_null structure array not null, " +
"struct_null structure array, struct_not_null structure array not null, " +
// "enumeration_null enumeration array, enumeration_not_null enumeration array not null, " +
"primary key(pk))";

Expand Down Expand Up @@ -113,8 +113,7 @@ private void insertArraysViaQuerySimpleStatement() throws SQLException {
"null, [x'31', x'32'], " +
"null, [(11, '11'), (22, '22')] " +
")");
Assert.assertThrows(SQLException.class,
() -> insertQuery("INSERT INTO T VALUES (" +
RelationalAssertions.assertThrowsSqlException(() -> insertQuery("INSERT INTO T VALUES (" +
"3," +
"[true, false], null, " +
"[11, 22], null, " +
Expand All @@ -125,14 +124,14 @@ private void insertArraysViaQuerySimpleStatement() throws SQLException {
"[x'31', x'32'], null, " +
"[(11, '11'), (22, '22')], null " +
")"));
Assert.assertThrows(SQLException.class, () -> insertQuery("INSERT INTO T (pk) VALUES (4)"));
RelationalAssertions.assertThrowsSqlException(() -> insertQuery("INSERT INTO T (pk) VALUES (4)"));
}

@Test
void testInsertArraysViaQueryPreparedStatement() throws SQLException {
final var statement = "INSERT INTO T (pk, boolean_null, boolean_not_null, integer_null, integer_not_null, " +
"bigint_null, bigint_not_null, float_null, float_not_null, double_null, double_not_null, string_null, " +
"string_not_null, bytes_null, bytes_not_null, structure_null, structure_not_null) VALUES (?pk, ?boolean_null, " +
"string_not_null, bytes_null, bytes_not_null, struct_null, struct_not_null) VALUES (?pk, ?boolean_null, " +
"?boolean_not_null, ?integer_null, ?integer_not_null, ?bigint_null, ?bigint_not_null, ?float_null, " +
"?float_not_null, ?double_null, ?double_not_null, ?string_null, ?string_not_null, ?bytes_null, " +
"?bytes_not_null, ?struct_null, ?struct_not_null)";
Expand Down Expand Up @@ -208,7 +207,7 @@ void testInsertArraysViaQueryPreparedStatement() throws SQLException {
ps.setNull("bytes_not_null", Types.ARRAY);
ps.setArray("struct_null", EmbeddedRelationalArray.newBuilder().addAll(EmbeddedRelationalStruct.newBuilder().addInt("a", 11).addString("b", "11").build(), EmbeddedRelationalStruct.newBuilder().addInt("a", 22).addString("b", "22").build()).build());
ps.setNull("struct_not_null", Types.ARRAY);
Assert.assertThrows(SQLException.class, ps::executeUpdate);
RelationalAssertions.assertThrowsSqlException(ps::executeUpdate);
}
}
}
Expand All @@ -222,8 +221,7 @@ void testMoreParametersThanColumns() throws SQLException {
ps.setInt("pk", 1);
ps.setArray("boolean_null", EmbeddedRelationalArray.newBuilder().addAll(true, false).build());
ps.setArray("boolean_not_null", EmbeddedRelationalArray.newBuilder().addAll(true, false).build());
final var actualMessage = Assertions.assertThrows(SQLException.class, ps::executeUpdate).getMessage();
assertTrue(actualMessage.contains("Too many parameters"));
RelationalAssertions.assertThrowsSqlException(ps::executeUpdate).containsInMessage("Too many parameters");
}
}
}
Expand All @@ -235,9 +233,7 @@ void testNonNullViolation() throws SQLException {
conn.setAutoCommit(true);
try (final var ps = ((RelationalPreparedStatement) conn.prepareStatement("INSERT INTO T (pk) VALUES (?pk)"))) {
ps.setInt("pk", 1);
final var actualMessage = Assertions.assertThrows(SQLException.class, ps::executeUpdate).getMessage();
System.out.println(actualMessage);
assertTrue(actualMessage.contains("violates not-null constraint"));
RelationalAssertions.assertThrowsSqlException(ps::executeUpdate).containsInMessage("violates not-null constraint");
}
}
}
Expand All @@ -249,7 +245,7 @@ void testNullFieldsGetAutomaticallyFilled() throws SQLException {
conn.setAutoCommit(true);
try (final var ps = ((RelationalPreparedStatement) conn.prepareStatement("INSERT INTO T (pk, boolean_not_null, " +
"integer_not_null, bigint_not_null, float_not_null, double_not_null, string_not_null, bytes_not_null, " +
"structure_not_null) VALUES (?pk, ?boolean_not_null, ?integer_not_null, ?bigint_not_null, ?float_not_null, " +
"struct_not_null) VALUES (?pk, ?boolean_not_null, ?integer_not_null, ?bigint_not_null, ?float_not_null, " +
"?double_not_null, ?string_not_null, ?bytes_not_null, ?struct_not_null)"))) {
ps.setInt("pk", 2);
ps.setArray("boolean_not_null", EmbeddedRelationalArray.newBuilder().addAll(true, false).build());
Expand All @@ -273,42 +269,19 @@ void testNullFieldsGetAutomaticallyFilled() throws SQLException {
.hasColumn("double_null", null)
.hasColumn("string_null", null)
.hasColumn("bytes_null", null)
.hasColumn("structure_null", null);
.hasColumn("struct_null", null);
}
}
}

@Test
void testNullColumnDoesNotRequireAValueIfNotGiven() throws SQLException {
void testColumnRequiresValue() throws SQLException {
try (final var conn = DriverManager.getConnection(database.getConnectionUri().toString())) {
conn.setSchema(database.getSchemaName());
conn.setAutoCommit(true);
try (final var ps = ((RelationalPreparedStatement) conn.prepareStatement("INSERT INTO T (pk, boolean_not_null, " +
"integer_not_null, bigint_not_null, float_not_null, double_not_null, string_not_null, bytes_not_null, " +
"structure_not_null, boolean_null, integer_null) VALUES (?pk, ?boolean_not_null, ?integer_not_null, " +
"?bigint_not_null, ?float_not_null, ?double_not_null, ?string_not_null, ?bytes_not_null, ?struct_not_null)"))) {
ps.setInt("pk", 2);
ps.setArray("boolean_not_null", EmbeddedRelationalArray.newBuilder().addAll(true, false).build());
ps.setArray("integer_not_null", EmbeddedRelationalArray.newBuilder().addAll(11, 22).build());
ps.setArray("bigint_not_null", EmbeddedRelationalArray.newBuilder().addAll(11L, 22L).build());
ps.setArray("float_not_null", EmbeddedRelationalArray.newBuilder().addAll(11.0f, 22.0f).build());
ps.setArray("double_not_null", EmbeddedRelationalArray.newBuilder().addAll(11.0, 22.0).build());
ps.setArray("string_not_null", EmbeddedRelationalArray.newBuilder().addAll("11", "22").build());
ps.setArray("bytes_not_null", EmbeddedRelationalArray.newBuilder().addAll(new byte[]{49}, new byte[]{50}).build());
ps.setArray("struct_not_null", EmbeddedRelationalArray.newBuilder().addAll(EmbeddedRelationalStruct.newBuilder().addInt("a", 11).addString("b", "11").build(), EmbeddedRelationalStruct.newBuilder().addInt("a", 22).addString("b", "22").build()).build());
assertEquals(1, ps.executeUpdate());
}
}
}

@Test
void testNonNullColumnRequiresValue() throws SQLException {
try (final var conn = DriverManager.getConnection(database.getConnectionUri().toString())) {
conn.setSchema(database.getSchemaName());
conn.setAutoCommit(true);
try (final var ps = ((RelationalPreparedStatement) conn.prepareStatement("INSERT INTO T (pk, boolean_not_null, " +
"integer_not_null, bigint_not_null, float_not_null, double_not_null, string_not_null, bytes_not_null, " +
"structure_not_null) VALUES (?pk, ?boolean_not_null, ?integer_not_null, ?bigint_not_null, ?float_not_null, " +
"struct_not_null) VALUES (?pk, ?boolean_not_null, ?integer_not_null, ?bigint_not_null, ?float_not_null, " +
"?double_not_null, ?string_not_null)"))) {
ps.setInt("pk", 2);
ps.setArray("boolean_not_null", EmbeddedRelationalArray.newBuilder().addAll(true, false).build());
Expand All @@ -317,8 +290,7 @@ void testNonNullColumnRequiresValue() throws SQLException {
ps.setArray("float_not_null", EmbeddedRelationalArray.newBuilder().addAll(11.0f, 22.0f).build());
ps.setArray("double_not_null", EmbeddedRelationalArray.newBuilder().addAll(11.0, 22.0).build());
ps.setArray("string_not_null", EmbeddedRelationalArray.newBuilder().addAll("11", "22").build());
final var actualMessage = Assert.assertThrows(SQLException.class, ps::executeUpdate).getMessage();
assertTrue(actualMessage.contains("violates not-null constraint"));
RelationalAssertions.assertThrowsSqlException(ps::executeUpdate).containsInMessage("not provided");
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...a/com/apple/foundationdb/relational/recordlayer/ddl/DdlRecordLayerSchemaTemplateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,8 @@ void typeAndEnumSameNameTest() throws RelationalException, SQLException {
}

@Test
void notNullTypeNotAllowedTest() throws RelationalException, SQLException {
String template = "CREATE SCHEMA TEMPLATE not_null_column_type " +
void notNullNonArrayTypeNotAllowedTest() throws RelationalException, SQLException {
String template = "CREATE SCHEMA TEMPLATE not_null_non_array_column_type " +
"CREATE TABLE foo (id bigint, foo string not null, PRIMARY KEY(id))";

run(statement -> RelationalAssertions.assertThrowsSqlException(() -> statement.executeUpdate(template))
Expand Down
11 changes: 10 additions & 1 deletion yaml-tests/src/test/resources/inserts-updates-deletes.yamsql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,19 @@ test_block:
# Case where not all values are provided of A. Still works, since the columns for which no values are provided can be nullable.
- query: insert into A(A1, A2, A3) values (4);
- supported_version: !current_version
- count: 1
- error: "42601"
-
# Case when the number of values is more than the number of columns specified.
- query: insert into A(A1, A2, A3) values (5, 6, 7, 8, 9);
- supported_version: !current_version
- error: "42601"
-
# Case when a nullable column's value is not provided
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to cover that, could you add a test where the not provided column is the first one and another case where it's the last one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

- query: insert into A(A1, A3) values (6, 7);
- supported_version: !current_version
- count: 1
-
- query: select * from A where A1 = 6
- supported_version: !current_version
- result: [{ A1: 6, A2: !null , A3: 7 }]
...
Loading