platinumhamburg commented on code in PR #2485:
URL: https://github.com/apache/fluss/pull/2485#discussion_r2758351132
##########
fluss-server/src/test/java/org/apache/fluss/server/kv/KvTabletTest.java:
##########
@@ -662,6 +663,43 @@ void testAutoIncrementWithMultiThread() throws Exception {
assertThat(actualUids).isEqualTo(expectedUids);
}
+ @Test
+ void testAutoIncrementWithInvalidTargetColumns() throws Exception {
+ Schema schema =
+ Schema.newBuilder()
+ .column("user_name", DataTypes.STRING())
+ .column("uid", DataTypes.INT())
+ .column("age", DataTypes.INT())
+ .primaryKey("user_name")
+ .enableAutoIncrement("uid")
+ .build();
+ initLogTabletAndKvTablet(schema, new HashMap<>());
+ KvRecordTestUtils.KvRecordFactory recordFactory =
+ KvRecordTestUtils.KvRecordFactory.of(
+ schema.getRowType().project(Arrays.asList("user_name",
"uid")));
+
+ KvRecordBatch kvRecordBatch =
+ kvRecordBatchFactory.ofRecords(
+ Arrays.asList(
+ recordFactory.ofRecord("k1".getBytes(), new
Object[] {"k1", null}),
+ recordFactory.ofRecord(
+ "k2".getBytes(), new Object[] {"k2",
null})));
+ // target columns don't contain auto-increment column
+ assertThatThrownBy(() -> kvTablet.putAsLeader(kvRecordBatch, new int[]
{0, 1}))
+ .isInstanceOf(InvalidTargetColumnException.class)
+ .hasMessageContaining(
+ "Auto-increment column [uid] at index 1 must not be
included in target columns.");
+
+ // no specify target columns, which is also invalid for auto-increment
+ assertThatThrownBy(() -> kvTablet.putAsLeader(kvRecordBatch, null))
+ .isInstanceOf(InvalidTargetColumnException.class)
+ .hasMessageContaining(
+ "The table contains an auto-increment column [uid],
but update target columns are not explicitly specified.");
+
+ // valid case: target columns contain auto-increment column
+ kvTablet.putAsLeader(kvRecordBatch, new int[] {0, 2});
Review Comment:
Ditto, it appears that the actual behavior of the test code is opposite to
what the comments state. Moreover, the test constructs records containing
{user_name, uid} (indexes 0, 1), but passes targetColumns = {0, 2}, which
refers to {user_name, age}—this is a code smell.
##########
fluss-server/src/test/java/org/apache/fluss/server/kv/KvTabletTest.java:
##########
@@ -662,6 +663,43 @@ void testAutoIncrementWithMultiThread() throws Exception {
assertThat(actualUids).isEqualTo(expectedUids);
}
+ @Test
+ void testAutoIncrementWithInvalidTargetColumns() throws Exception {
+ Schema schema =
+ Schema.newBuilder()
+ .column("user_name", DataTypes.STRING())
+ .column("uid", DataTypes.INT())
+ .column("age", DataTypes.INT())
+ .primaryKey("user_name")
+ .enableAutoIncrement("uid")
+ .build();
+ initLogTabletAndKvTablet(schema, new HashMap<>());
+ KvRecordTestUtils.KvRecordFactory recordFactory =
+ KvRecordTestUtils.KvRecordFactory.of(
+ schema.getRowType().project(Arrays.asList("user_name",
"uid")));
+
+ KvRecordBatch kvRecordBatch =
+ kvRecordBatchFactory.ofRecords(
+ Arrays.asList(
+ recordFactory.ofRecord("k1".getBytes(), new
Object[] {"k1", null}),
+ recordFactory.ofRecord(
+ "k2".getBytes(), new Object[] {"k2",
null})));
+ // target columns don't contain auto-increment column
+ assertThatThrownBy(() -> kvTablet.putAsLeader(kvRecordBatch, new int[]
{0, 1}))
Review Comment:
The auto-incrementing index ID is 1, which suggests that the validation
logic in the code here is inconsistent with the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
