Copilot commented on code in PR #16170:
URL: https://github.com/apache/iotdb/pull/16170#discussion_r2555703519
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/relational/ColumnTransformerBuilder.java:
##########
@@ -775,7 +777,12 @@ private ColumnTransformer getFunctionColumnTransformer(
} else if
(TableBuiltinScalarFunction.LENGTH.getFunctionName().equalsIgnoreCase(functionName))
{
ColumnTransformer first = this.process(children.get(0), context);
if (children.size() == 1) {
- return new LengthColumnTransformer(INT32, first);
+ Type argumentType = first.getType();
+ if (isCharType(argumentType)) {
+ return new LengthColumnTransformer(INT32, first);
+ } else if (isBlobType(argumentType)) {
+ return new BlobLengthColumnTransformer(INT32, first);
+ }
Review Comment:
Missing return statement when the LENGTH function condition is met but
neither isCharType nor isBlobType returns true. This will cause the method to
fall through to subsequent else-if blocks and potentially throw "Unknown
function" exception. Add a return statement or throw an appropriate exception
after line 785 to handle unexpected types that pass the semantic check but
shouldn't reach this point.
```suggestion
}
// If argumentType is neither char nor blob, throw an exception
throw new SemanticException(
String.format(
"LENGTH function only supports CHAR or BLOB types, but got:
%s",
argumentType));
```
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/transformation/dag/column/unary/scalar/BlobLengthColumnTransformerTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.db.queryengine.transformation.dag.column.unary.scalar;
+
+import
org.apache.iotdb.db.queryengine.transformation.dag.column.ColumnTransformer;
+
+import org.apache.tsfile.block.column.Column;
+import org.apache.tsfile.read.common.block.column.BinaryColumn;
+import org.apache.tsfile.utils.Binary;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+
+import static org.apache.tsfile.read.common.type.IntType.INT32;
+
+public class BlobLengthColumnTransformerTest {
+
+ private ColumnTransformer mockChildColumnTransformer(Column column) {
+ ColumnTransformer mockColumnTransformer =
Mockito.mock(ColumnTransformer.class);
+ Mockito.when(mockColumnTransformer.getColumn()).thenReturn(column);
+ Mockito.doNothing().when(mockColumnTransformer).tryEvaluate();
+ Mockito.doNothing().when(mockColumnTransformer).clearCache();
+
Mockito.doNothing().when(mockColumnTransformer).evaluateWithSelection(Mockito.any());
+ return mockColumnTransformer;
+ }
+
+ private static byte[] hexStringToByteArray(String s) {
+ int len = s.length();
+ if (len % 2 != 0) {
+ throw new IllegalArgumentException("Hex string must have an even number
of characters");
+ }
+ byte[] data = new byte[len / 2];
+ for (int i = 0; i < len; i += 2) {
+ data[i / 2] =
+ (byte) ((Character.digit(s.charAt(i), 16) << 4) +
Character.digit(s.charAt(i + 1), 16));
+ }
+ return data;
+ }
+
+ /**
+ * Test blob length from string input, the length should be the byte length
of the string in UTF-8
+ * encoding.
+ */
+ @Test
+ public void testBlobLengthFromString() {
+ String input = "hello";
+ Binary[] values = new Binary[] {new Binary(input, StandardCharsets.UTF_8)};
+ Column binaryColumn = new BinaryColumn(values.length, Optional.empty(),
values);
+
+ ColumnTransformer childColumnTransformer =
mockChildColumnTransformer(binaryColumn);
+ BlobLengthColumnTransformer blobLengthColumnTransformer =
+ new BlobLengthColumnTransformer(INT32, childColumnTransformer);
+ blobLengthColumnTransformer.addReferenceCount();
+ blobLengthColumnTransformer.evaluate();
+ Column result = blobLengthColumnTransformer.getColumn();
+
+ int expectedLength = input.getBytes(StandardCharsets.UTF_8).length;
+ Assert.assertEquals(expectedLength, result.getInt(0));
+ }
+
+ @Test
+ public void testBlobLengthFromHex() {
+ String input = "68656C6C6F";
+ byte[] inputBytes = hexStringToByteArray(input);
+ Binary[] values = new Binary[] {new Binary(inputBytes)};
+ Column binaryColumn = new BinaryColumn(values.length, Optional.empty(),
values);
+
+ ColumnTransformer childColumnTransformer =
mockChildColumnTransformer(binaryColumn);
+ BlobLengthColumnTransformer blobLengthColumnTransformer =
+ new BlobLengthColumnTransformer(INT32, childColumnTransformer);
+ blobLengthColumnTransformer.addReferenceCount();
+ blobLengthColumnTransformer.evaluate();
+ Column result = blobLengthColumnTransformer.getColumn();
+
+ int expectedLength = inputBytes.length;
+ Assert.assertEquals(expectedLength, result.getInt(0));
+ }
+
+ @Test
+ public void testBlobLengthMultiRowsWithNull() {
+ String input1 = "68656C6C6F";
+ String input2 = "1F3C";
+ byte[] inputBytes1 = hexStringToByteArray(input1);
+ byte[] inputBytes2 = hexStringToByteArray(input2);
+
+ Binary[] values = new Binary[] {new Binary(inputBytes1), null, new
Binary(inputBytes2)};
+ boolean[] valueIsNull = new boolean[] {false, true, false};
+ Column binaryColumn = new BinaryColumn(values.length,
Optional.of(valueIsNull), values);
+ ColumnTransformer childColumnTransformer =
mockChildColumnTransformer(binaryColumn);
+
+ BlobLengthColumnTransformer blobLengthColumnTransformer =
+ new BlobLengthColumnTransformer(INT32, childColumnTransformer);
+ blobLengthColumnTransformer.addReferenceCount();
+ blobLengthColumnTransformer.evaluate();
+ Column result = blobLengthColumnTransformer.getColumn();
+ Assert.assertEquals(inputBytes1.length, result.getInt(0));
+ Assert.assertTrue(result.isNull(1));
+ Assert.assertEquals(inputBytes2.length, result.getInt(2));
+ }
+
+ @Test
+ public void testBlobLengthWithSelection() {
+ String input1 = "68656C6C6F";
+ String input2 = "1F3C";
+ String input3 = "";
+
+ byte[] bytes1 = input1.getBytes(StandardCharsets.UTF_8);
+ byte[] bytes2 = input2.getBytes(StandardCharsets.UTF_8);
+ byte[] bytes3 = input3.getBytes(StandardCharsets.UTF_8);
Review Comment:
Incorrect test data creation. The test uses
`getBytes(StandardCharsets.UTF_8)` on hex string literals like "68656C6C6F"
instead of converting them from hex to bytes using `hexStringToByteArray()`.
This treats the hex string as a regular string, converting each character to
its UTF-8 byte representation, rather than interpreting it as hex-encoded
bytes. This is inconsistent with other tests in the same class (e.g.,
testBlobLengthFromHex, testBlobLengthMultiRowsWithNull) and doesn't properly
test hex blob data. Use `hexStringToByteArray(input1)` instead of
`input1.getBytes(StandardCharsets.UTF_8)`.
```suggestion
byte[] bytes1 = hexStringToByteArray(input1);
byte[] bytes2 = hexStringToByteArray(input2);
byte[] bytes3 = hexStringToByteArray(input3);
```
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/query/recent/IoTDBLengthFunctionIT.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.relational.it.query.recent;
+
+import org.apache.iotdb.it.env.EnvFactory;
+import org.apache.iotdb.it.framework.IoTDBTestRunner;
+import org.apache.iotdb.itbase.category.TableClusterIT;
+import org.apache.iotdb.itbase.category.TableLocalStandaloneIT;
+import org.apache.iotdb.rpc.TSStatusCode;
+
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import static org.apache.iotdb.db.it.utils.TestUtils.prepareTableData;
+import static org.apache.iotdb.db.it.utils.TestUtils.tableAssertTestFail;
+import static org.apache.iotdb.db.it.utils.TestUtils.tableResultSetEqualTest;
+
+@RunWith(IoTDBTestRunner.class)
+@Category({TableLocalStandaloneIT.class, TableClusterIT.class})
+public class IoTDBLengthFunctionIT {
+
+ private static final String DATABASE_NAME = "test_length_function";
+ private static final String[] createSqls =
+ new String[] {
+ "CREATE DATABASE " + DATABASE_NAME,
+ "USE " + DATABASE_NAME,
+ "CREATE TABLE table1(c_text TEXT FIELD, c_string STRING FIELD, c_blob
BLOB FIELD, c_int INT32 FIELD)",
+ "INSERT INTO table1(time, c_text, c_string, c_blob) VALUES (1,
'hello', 'hello', X'68656C6C6F')",
+ "INSERT INTO table1(time, c_text, c_string, c_blob) VALUES (2, '你好',
'你好', X'e4bda0e5a5bd')",
+ "INSERT INTO table1(time, c_text, c_string, c_blob) VALUES (3, '', '',
X'')",
+ "INSERT INTO table1(time, c_int) VALUES (4, 404)"
+ };
+
+ @BeforeClass
+ public static void setUp() throws Exception {
+ EnvFactory.getEnv().initClusterEnvironment();
+ prepareTableData(createSqls);
+ }
+
+ @AfterClass
+ public static void tearDown() throws Exception {
+ EnvFactory.getEnv().cleanClusterEnvironment();
+ }
+
+ /** validate LENGTH() for TEXT and STRING types, correctly calculate the
character count */
+ @Test
+ public void testLengthOnTextAndString() {
+ String[] expectedHeader = new String[] {"time", "length(c_text)",
"length(c_string)"};
+ String[] retArray =
+ new String[] {
+ "1970-01-01T00:00:00.001Z,5,5,",
+ "1970-01-01T00:00:00.002Z,2,2,",
+ "1970-01-01T00:00:00.003Z,0,0,",
+ "1970-01-01T00:00:00.004Z,null,null,"
+ };
+
+ tableResultSetEqualTest(
+ "SELECT time, length(c_text) as \"length(c_text)\", length(c_string)
as \"length(c_string)\" FROM table1",
+ expectedHeader,
+ retArray,
+ DATABASE_NAME);
+ }
+
+ /** validate LENGTH() for TEXT and STRING types, correctly calculate the
number of bytes */
Review Comment:
Comment contains incorrect description. This test validates LENGTH() for
BLOB type (calculating byte length), not for TEXT and STRING types (which
calculate character count). The comment should say "validate LENGTH() for BLOB
type, correctly calculate the number of bytes".
```suggestion
/** validate LENGTH() for BLOB type, correctly calculate the number of
bytes */
```
--
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]
