Refactor TSDataType switches to use TypeService - #18606
Conversation
Caideyipi
left a comment
Caideyipi
left a comment
There was a problem hiding this comment.
发现 4 个会导致功能错误或序列化不兼容的问题，建议修复后再合并。下面的行级评论分别说明了复现测试和修复方向。
|
|
||
| @Override | ||
| public void initializeFloatValues() { | ||
| floatValues = new FloatBigArray(Float.MIN_VALUE); |
There was a problem hiding this comment.
Float.MIN_VALUE and Double.MIN_VALUE are the smallest positive values, not lower bounds. With this initialization, MAX ignores zero and all negative FLOAT/DOUBLE inputs, leaving inits false and returning NULL. The existing CI tests testFloatMaxWithNonPositiveInput, testDoubleMaxWithNonPositiveInput, and testDoubleMaxWithNonPositiveIntermediateInput reproduce this regression. Please keep the previous NEGATIVE_INFINITY initialization or use another true lower bound.
There was a problem hiding this comment.
Fixed in the updated branch: grouped FLOAT/DOUBLE MAX now initializes with Float.NEGATIVE_INFINITY and Double.NEGATIVE_INFINITY, so zero and negative values can initialize the result. All 3 GroupedMaxAccumulatorTest cases pass, including non-positive intermediate input.
| } | ||
|
|
||
| public void setXResult(final TsPrimitiveType result, final Column column, final int index) { | ||
| type.setTo(result, column, index); |
There was a problem hiding this comment.
Type.setTo(TsPrimitiveType, Column, int) copies the primitive into the column; it does not copy the column value into the primitive. This call therefore leaves xResult at its default value and can overwrite the input column. CI reproduces this in AccumulatorTest.maxByAccumulatorTest (-99 expected, 0 returned). Please use the correct column-to-primitive copy operation.
There was a problem hiding this comment.
Fixed in the updated branch: setXResult now uses the column-to-primitive setter to copy the selected input into the result, without writing into the source column. The testMaxMinByReadsXWithoutModifyingInput regression verifies both result replacement and input preservation; the DataNode TypeServicesTest suite passes.
| default: | ||
| throw new UnSupportedDataTypeException(String.format(DATATYPE_UNSUPPORTED, dataType)); | ||
| } | ||
| Type.fromTsDataType(dataType).serializeArray(column, rowCount, buffer); |
There was a problem hiding this comment.
Type.serializeArray uses TsFile's binary-array format, which writes a presence byte before each binary value. The Tablet reader in RAW_ARRAY_BYTE_BUFFER_DESERIALIZER_SERVICE still reads the legacy Tablet format (length + payload only, with nulls represented by the separate bitmap), so the first presence byte is interpreted as part of the length and the stream becomes misaligned. CI fails with BufferUnderflowException in InsertMultiTabletsNodeSerdeTest. Please preserve the existing Tablet wire format or update both serialization, size calculation, and all readers together.
There was a problem hiding this comment.
Fixed in 5eb7694245c. Both ByteBuffer and DataOutputStream Tablet writers now use dedicated TypeService serializers that preserve the legacy length-plus-payload format. Null binary values retain a zero-length placeholder and use the existing separate bitmap; no TsFile presence byte is added. The new regression checks exact bytes for TEXT, STRING, BLOB and OBJECT, including nulls, empty values and inactive rows. InsertTabletNodeSerdeTest and InsertMultiTabletsNodeSerdeTest pass.
| break; | ||
| } | ||
| return size; | ||
| return Type.fromTsDataType(dataType).serializedSize(column, start, end); |
There was a problem hiding this comment.
The WAL size calculation now uses Type.serializedSize, whose binary-array size includes a presence byte, but WAL_ARRAY_WRITER_SERVICE still writes only the length and payload. The calculated WAL entry size therefore disagrees with the bytes actually written, which breaks WAL offsets/search indexes. The three WALFileTest failures in CI reproduce this. Please use a size calculation matching the WAL writer's wire format.
There was a problem hiding this comment.
Fixed in 5eb7694245c. Tablet and WAL size calculation now share a range-aware calculator that counts exactly the bytes written by the legacy WAL writer: length plus payload, including null placeholders, over [start, end). A regression compares the calculated size with the actual bytes written for a non-zero-start binary slice. All 5 WALFileTest cases pass; the targeted DataNode serialization/WAL run passed 31 tests.
There was a problem hiding this comment.
🟡 Changes recommended
The architecture check is not repository-wide, and several new localized-message definitions violate established naming and localization requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors type-specific logic into TsFile Type/TypeService abstractions across UDFs, query execution, storage, clients, and examples.
Changes:
- Replaces widespread
TSDataTypeswitches with centralized type services. - Updates TsFile APIs/version and tablet/WAL-related handling.
- Adds architecture and regression tests plus localized messages.
File summaries
| File | Description |
|---|---|
pom.xml |
Updates TsFile snapshot. |
library-udf/pom.xml |
Adds TsFile dependency. |
library-udf/.../util/Util.java |
Delegates row value operations. |
library-udf/.../match/UDAFPatternMatch.java |
Uses numeric column reader. |
library-udf/.../match/UDAFDTWMatch.java |
Uses numeric column reader. |
library-udf/.../drepair/UDTFValueRepair.java |
Delegates repaired-value output. |
library-udf/.../drepair/UDTFValueFill.java |
Delegates filled-value output. |
library-udf/.../drepair/UDTFTimestampRepair.java |
Delegates cast output. |
library-udf/.../anomaly/UDTFTwoSidedFilter.java |
Delegates filtered output. |
library-udf/.../i18n/en/LibraryUdfMessages.java |
Adds English messages. |
library-udf/.../i18n/zh/LibraryUdfMessages.java |
Adds Chinese messages. |
node-commons/.../InternalTypeManagerTest.java |
Tests type conversions. |
node-commons/.../MasterRepairUtil.java |
Uses generic numeric row access. |
node-commons/.../UDTFValueTrend.java |
Delegates previous-value reads. |
node-commons/.../UDTFValueDifference.java |
Selects difference operators. |
node-commons/.../UDTFTopK.java |
Delegates queue construction. |
node-commons/.../UDTFNonNegativeValueDifference.java |
Delegates difference calculation. |
node-commons/.../UDTFNonNegativeDerivative.java |
Delegates derivative calculation. |
node-commons/.../UDTFM4.java |
Delegates window transformation. |
node-commons/.../UDTFEqualSizeBucketRandomSample.java |
Delegates row collection. |
node-commons/.../UDTFEqualSizeBucketM4Sample.java |
Delegates M4 sampling. |
node-commons/.../UDTFEqualSizeBucketAggSample.java |
Delegates bucket aggregation. |
node-commons/.../UDTFDerivative.java |
Initializes derivative services. |
node-commons/.../UDTFCommonValueDifference.java |
Uses shared difference operator. |
node-commons/.../UDTFCommonDerivative.java |
Uses shared derivative operator. |
node-commons/.../InternalTypeManager.java |
Simplifies type mapping. |
datanode/.../TSDataTypeSwitchArchitectureTest.java |
Adds switch architecture check. |
datanode/.../DescFakedSeriesReader.java |
Uses TsFile type factory. |
datanode/.../AscFakedSeriesReader.java |
Uses TsFile type factory. |
datanode/.../PrimitiveMemTableTest.java |
Updates primitive test values. |
datanode/.../CompactionCheckerUtils.java |
Updates compaction test values. |
datanode/.../OpcUaNameSpaceMetadataTest.java |
Updates OPC UA test values. |
datanode/.../IoTDBOpcUaClientTest.java |
Updates OPC UA client values. |
datanode/.../TypeInferenceUtils.java |
Delegates auto-cast checks. |
datanode/.../TimeValuePairUtils.java |
Delegates value copying/factories. |
datanode/.../EncodingInferenceUtils.java |
Delegates encoding selection. |
datanode/.../LongTVList.java |
Uses typed primitive creation. |
datanode/.../IntTVList.java |
Uses typed primitive creation. |
datanode/.../FloatTVList.java |
Uses typed primitive creation. |
datanode/.../DoubleTVList.java |
Uses typed primitive creation. |
datanode/.../BooleanTVList.java |
Uses typed primitive creation. |
datanode/.../BinaryTVList.java |
Uses typed primitive creation. |
datanode/.../TsFileSplitTool.java |
Delegates chunk writes. |
datanode/.../TsFileSplitByPartitionTool.java |
Delegates partitioned writes. |
datanode/.../LoadTsFileManager.java |
Updates last-value conversion. |
datanode/.../TsFileResourceUtils.java |
Updates resource last values. |
datanode/.../MemAlignedPageReader.java |
Delegates statistics updates. |
datanode/.../SingleSeriesCompactionExecutor.java |
Delegates compaction writes. |
datanode/.../ReadChunkAlignedSeriesCompactionExecutor.java |
Delegates size estimation. |
datanode/.../transformation/dag/util/TypeUtils.java |
Centralizes column operations. |
datanode/.../ElasticSerializableRowRecordListBackedMultiColumnRow.java |
Widens numeric reads. |
datanode/.../InsertRowStatement.java |
Delegates value deserialization. |
datanode/.../OperatorTreeGenerator.java |
Delegates constant fills. |
datanode/.../OperatorGeneratorUtil.java |
Delegates value-size estimates. |
datanode/.../WindowManagerFactory.java |
Delegates window creation. |
datanode/.../LastQueryAggTableScanOperator.java |
Delegates primitive cloning. |
datanode/.../TreeInsertTabletStatementGenerator.java |
Uses type converter directly. |
datanode/.../TransformOperator.java |
Delegates column writes. |
datanode/.../TableInsertTabletStatementGenerator.java |
Uses type converter directly. |
datanode/.../AggregationUtil.java |
Delegates output-size estimates. |
datanode/.../AccumulatorFactory.java |
Delegates mode accumulator creation. |
datanode/.../PipeMemoryWeightUtil.java |
Uses typed size estimates. |
datanode/.../TimeSeriesRuntimeState.java |
Adds typed row dispatch. |
datanode/.../SinglePageWholeChunkReader.java |
Delegates memory estimates. |
datanode/.../PipeTabletUtils.java |
Delegates tablet value insertion. |
datanode/.../PipeRow.java |
Delegates object reads. |
datanode/.../PipeDataTypeTransformer.java |
Delegates pipe type conversion. |
datanode/.../IoTDBDescriptor.java |
Delegates default encoding. |
datanode/.../IoTDBConfig.java |
Implements encoding provider. |
datanode/.../i18n/zh/StorageEngineMessages.java |
Normalizes whitespace. |
datanode/.../i18n/zh/DataNodeQueryMessages.java |
Adds Chinese query messages. |
datanode/.../i18n/zh/DataNodePipeMessages.java |
Adds Chinese pipe message. |
datanode/.../i18n/zh/DataNodeMiscMessages.java |
Adds Chinese type messages. |
datanode/.../i18n/en/DataNodeQueryMessages.java |
Adds English query messages. |
datanode/.../i18n/en/DataNodePipeMessages.java |
Adds English pipe message. |
datanode/.../i18n/en/DataNodeMiscMessages.java |
Adds English type messages. |
iotdb-core/datanode/pom.xml |
Adds ASM test dependency. |
calc-commons/.../SerializableTVList.java |
Delegates row memory sizing. |
calc-commons/.../TryCastFunctionColumnTransformer.java |
Reuses cast dispatch. |
calc-commons/.../RoundColumnTransformer.java |
Uses generic numeric access. |
calc-commons/.../CastFunctionColumnTransformer.java |
Reuses cast dispatch. |
calc-commons/.../*GreatestColumnTransformer.java |
Exposes constructors to services. |
calc-commons/.../*LeastColumnTransformer.java |
Exposes constructors to services. |
calc-commons/.../AbstractGreatestLeastColumnTransformer.java |
Delegates transformer selection. |
calc-commons/.../MergeSortFullOuterJoinOperator.java |
Uses typed row functions. |
calc-commons/.../TableRegressionAccumulator.java |
Simplifies numeric reads. |
calc-commons/.../TableCovarianceAccumulator.java |
Simplifies numeric reads. |
calc-commons/.../TableCorrelationAccumulator.java |
Simplifies numeric reads. |
calc-commons/.../RateFunctionValidation.java |
Delegates numeric conversion. |
calc-commons/.../GroupedRegressionAccumulator.java |
Simplifies grouped reads. |
calc-commons/.../GroupedCovarianceAccumulator.java |
Simplifies grouped reads. |
calc-commons/.../GroupedCorrelationAccumulator.java |
Simplifies grouped reads. |
calc-commons/.../LongBigArray.java |
Adds byte serialization. |
calc-commons/.../IntBigArray.java |
Adds byte serialization. |
calc-commons/.../FloatBigArray.java |
Adds byte serialization. |
calc-commons/.../DoubleBigArray.java |
Adds byte serialization. |
calc-commons/.../BooleanBigArray.java |
Adds byte serialization. |
calc-commons/.../BinaryBigArray.java |
Adds byte serialization. |
calc-commons/.../AbstractApproxMostFrequentAccumulator.java |
Updates localized message key. |
calc-commons/.../ColumnList.java |
Adds cross-column equality. |
calc-commons/.../ValueWindowFunction.java |
Centralizes default-value writes. |
calc-commons/.../LeadFunction.java |
Removes local type dispatch. |
calc-commons/.../LagFunction.java |
Removes local type dispatch. |
calc-commons/.../MergeSortComparator.java |
Delegates comparator creation. |
calc-commons/.../JoinKeyComparatorFactory.java |
Delegates join comparison. |
calc-commons/.../RegressionAccumulator.java |
Simplifies numeric reads. |
calc-commons/.../CovarianceAccumulator.java |
Simplifies numeric reads. |
calc-commons/.../CorrelationAccumulator.java |
Simplifies numeric reads. |
calc-commons/.../CentralMomentAccumulator.java |
Uses numeric converter service. |
session/.../SessionUtilsTest.java |
Tests mixed value encoding. |
service-rpc/.../PreparedParameterSerdeTest.java |
Tests unsupported types. |
service-rpc/.../PreparedParameterSerde.java |
Delegates parameter decoding. |
jdbc/.../GroupedLSBWatermarkEncoder.java |
Adds typed watermark encoding. |
isession/.../TypeServices.java |
Adds RPC field readers. |
isession/.../SessionDataSet.java |
Uses RPC field readers. |
udf-api/.../RowImpl.java |
Widens numeric reads and mapping. |
udf-api/.../access/Row.java |
Documents numeric conversion. |
integration-test/.../TwoSum.java |
Delegates example arithmetic. |
rest/.../FastLastHandlerTest.java |
Updates primitive test values. |
example/udf/pom.xml |
Adds provided TsFile dependency. |
example/session/.../TabletExample.java |
Adds typed CSV parsers. |
Review details
- Files reviewed: 120/299 changed files
- Comments generated: 11
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| throw new UnSupportedDataTypeException( | ||
| String.format("Data type %s is not supported.", type.getTypeEnum())); |
There was a problem hiding this comment.
Fixed in 5eb7694245c by reusing ISessionMessages.EXCEPTION_DATA_TYPE_ARG_NOT_SUPPORTED_31213160, which already exists in both locale files. The exception no longer embeds an English literal. English and Chinese source reactor builds pass (excluding the distribution packaging module).
| public static final String DATA_TYPE_NOT_CONSISTENT_FMT = | ||
| "data type is not consistent, input %s, registered %s"; | ||
| public static final String DATA_TYPE_NOT_CONSISTENT_WITH_CAUSE_FMT = | ||
| "data type is not consistent, input %s, registered %s because %s"; |
There was a problem hiding this comment.
Fixed in 5eb7694245c. The two newly introduced inconsistent-type messages now use the generated EXCEPTION__ names in both locale files, with their TypeServices call sites updated together. The existing unrelated UNSUPPORTED_DATA_TYPE_FMT key is retained. English and Chinese source reactor builds pass.
| public static final String UNSUPPORTED_DATA_TYPE_FOR_COLUMN_FMT = | ||
| "unsupported data type %s for column %s"; |
There was a problem hiding this comment.
Fixed in 5eb7694245c: the key is now EXCEPTION_UNSUPPORTED_DATA_TYPE_ARG_FOR_COLUMN_ARG_4C4CCA6D in both locale files, and both pipe row-reading call sites use it. The message placeholders are preserved; English and Chinese source reactor builds pass.
| public static final String VALUE_CANNOT_BE_CAST_TO_DATA_TYPE_FMT = | ||
| "\"%s\" cannot be cast to [%s]"; | ||
| public static final String UNSUPPORTED_DATA_TYPE_FMT = | ||
| "Unsupported data type %s"; |
There was a problem hiding this comment.
Fixed in 5eb7694245c. All newly added keys in this block now follow the generated exception-message naming convention in both locales, including the cast, unsupported type, MaxBy/MinBy, equal/variation event and TIMESTAMP IN-list messages. The associated aggregation, predicate-conversion and TypeServices call sites and message assertions were updated together. English and Chinese source reactor builds pass.
| public static final String UNSUPPORTED_SCALAR_SUBQUERY_RESULT_DATA_TYPE_FMT = | ||
| "Unsupported data type for scalar subquery result: %s"; |
There was a problem hiding this comment.
Fixed in 5eb7694245c: both locale files and the scalar-subquery conversion call site now use EXCEPTION_UNSUPPORTED_DATA_TYPE_FOR_SCALAR_SUBQUERY_RESULT_ARG_D58CBB00. English and Chinese source reactor builds pass.
| // Avoid building the entire server dependency graph: only switch owners need checking. | ||
| .withImportOption(location -> TSDataTypeSwitchRule.hasSwitch(location.asURI())) | ||
| .importPackages("org.apache.iotdb")); |
There was a problem hiding this comment.
Fixed in 5eb7694245c. Added architecture-test to the default reactor with an aggregate production classpath covering the Java modules, including library-udf, JDBC and examples. Its ArchUnit/ASM check scans org.apache.iotdb without a violation baseline; the existing DataNode check remains explicitly scoped to org.apache.iotdb.db. The aggregate module includes the rule regression tests for ordinary switches, lambdas, anonymous classes, valid TypeService implementations and unrelated methods. Both the aggregate and DataNode architecture suites pass (7 tests each). Remaining client/example/REST type dispatches were migrated to TypeService.
| public static final String FREQUENCY_MUST_BE_POSITIVE = "The param 'frequency' must > 0."; | ||
| public static final String AMPLIFICATION_MUST_BE_AT_LEAST_1 = | ||
| "The param 'amplification' must >= 1."; |
There was a problem hiding this comment.
Fixed in 5eb7694245c. The English messages now say "must be greater than 0" and "must be greater than or equal to 1". The corresponding Chinese messages and validation behavior are preserved.
| public static final String FREQUENCY_MUST_BE_POSITIVE = "The param 'frequency' must > 0."; | ||
| public static final String AMPLIFICATION_MUST_BE_AT_LEAST_1 = | ||
| "The param 'amplification' must >= 1."; |
There was a problem hiding this comment.
Fixed in 5eb7694245c. The frequency and amplification keys now use EXCEPTION_THE_PARAM_FREQUENCY_MUST_BE_GREATER_THAN_0_45820CF9 and EXCEPTION_THE_PARAM_AMPLIFICATION_MUST_BE_GREATER_THAN_OR_EQUAL_TO_1_D64050EB, derived from the corrected English messages. Both locale files and UDFEnvelopeAnalysis call sites match.
| // ExactOrderStatistics | ||
| public static final String UNSUPPORTED_DATA_TYPE = "Unsupported data type: %s"; | ||
|
|
||
| // UDAFQuantile | ||
| public static final String UNSUPPORTED_DATA_TYPE_IN_QUANTILE = "Unsupported data type"; |
There was a problem hiding this comment.
Fixed in 5eb7694245c. Both locale files now use EXCEPTION_UNSUPPORTED_DATA_TYPE_ARG_B411C29E and EXCEPTION_UNSUPPORTED_DATA_TYPE_A8CA7BE7, with ExactOrderStatistics and UDAFQuantile updated accordingly. English and Chinese source reactor builds pass.
| public static final String QUANTILE_K_MUST_BE_AT_LEAST_100 = | ||
| "Size K has to be greater than or equal to 100."; | ||
| public static final String QUANTILE_RANK_MUST_BE_IN_RANGE = | ||
| "rank has to be greater than 0 and less than or equal to 1."; |
There was a problem hiding this comment.
Fixed in 5eb7694245c. The size/rank validation keys now use EXCEPTION_SIZE_K_HAS_TO_BE_GREATER_THAN_OR_EQUAL_TO_100_C514D1C3 and EXCEPTION_RANK_HAS_TO_BE_GREATER_THAN_0_AND_LESS_THAN_OR_EQUAL_TO_1_0F16AF94 in both locale files and UDAFQuantile. The validation boundaries are unchanged; English and Chinese source reactor builds pass.
jt2594838
left a comment
jt2594838
left a comment
There was a problem hiding this comment.
TypeService follow-up changes and validation at 5eb7694245c.
| ? null | ||
| : RpcUtils.formatDatetime(timeFormat, timestampPrecision, value, zoneId); | ||
| }; | ||
| case ROW, UNKNOWN, VECTOR -> |
There was a problem hiding this comment.
CLI value formatting now dispatches through TypeService with an exhaustive enum switch. ROW, UNKNOWN and VECTOR explicitly throw UnSupportedDataTypeException using the English/Chinese CLI message, so unsupported output types cannot silently become null. Supported DATE/TIMESTAMP/BLOB null handling and formatting are preserved. AbstractCliTest passes (6 tests).
| case DATE -> value -> LocalDate.parse(value); | ||
| case BLOB -> | ||
| value -> new Binary(parseHexStringToByteArray(value.replaceFirst("0x", ""))); | ||
| case OBJECT, ROW, UNKNOWN, VECTOR -> |
There was a problem hiding this comment.
Import value parsing now dispatches through TypeService. OBJECT, ROW, UNKNOWN and VECTOR are explicitly rejected using the localized unsupported-type exception; the default null-producing parser is removed. NumberFormatException handling still returns null for invalid numeric input. The CLI/tool test run passes all 9 tests.
| case DOUBLE -> DataIterator::getDouble; | ||
| case TEXT, STRING -> DataIterator::getString; | ||
| case DATE, BLOB, OBJECT -> DataIterator::getObject; | ||
| case ROW, UNKNOWN, VECTOR -> |
There was a problem hiding this comment.
Migration column readers now use TypeService with explicit rejection of ROW, UNKNOWN and VECTOR. An unsupported type raises a localized UnSupportedDataTypeException and reaches the existing device-failure handler; the previous null-result/log-and-continue fallback is removed. The example and its reactor dependencies compile successfully.
| case TEXT, BLOB, STRING -> | ||
| (value, index, mismatchedInfo) -> | ||
| new Binary(value.toString().getBytes(StandardCharsets.UTF_8)); | ||
| case OBJECT, ROW, UNKNOWN, VECTOR -> |
There was a problem hiding this comment.
REST row conversion now uses an exhaustive TypeService switch. OBJECT, ROW, UNKNOWN and VECTOR explicitly retain the existing IoTDBConnectionException behavior, and numeric converters use instanceof pattern variables while preserving mismatch reporting. All 19 REST tests pass.
| case INT64, TIMESTAMP -> (builder, value) -> builder.writeLong((long) value); | ||
| case DOUBLE -> (builder, value) -> builder.writeDouble((double) value); | ||
| case BOOLEAN -> (builder, value) -> builder.writeBoolean((boolean) value); | ||
| case ROW, UNKNOWN, VECTOR -> |
There was a problem hiding this comment.
JDBC metadata column writing now dispatches through TypeService. ROW, UNKNOWN and VECTOR explicitly throw UnSupportedDataTypeException using matching English/Chinese message keys, replacing the logging-only default branch. IoTDBDatabaseMetadataTest passes all 5 tests, and both locale source reactor builds succeed.
What is changed
Validation