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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18606 +/- ##
============================================
+ Coverage 42.71% 43.51% +0.80%
Complexity 442 442
============================================
Files 5441 5460 +19
Lines 394511 391754 -2757
Branches 51718 50725 -993
============================================
+ Hits 168518 170479 +1961
+ Misses 225993 221275 -4718 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Caideyipi
left a comment
Caideyipi
left a comment
There was a problem hiding this comment.
Reviewed the latest head 6ac266b. The previously reported max-value initialization, Type.setTo direction, Tablet binary wire-format, WAL size, localization, and architecture-check issues are fixed. Targeted regression tests pass, and all CI checks except SonarCloud pass; SonarCloud failed with Java heap space OOM during analysis. No remaining functional issues found.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Several correctness and performance regressions remain at 6ac266b065f06b9bc6b16d31a4b58c66e08b90fb. The inline comments cover eight main findings and two lower-priority implementation-contract issues.
The comments include reproducible table SQL, executed class-level results, and measured performance data where available. Each performance comment identifies the measured versions and workload; the historical microbenchmark numbers are not presented as a fresh benchmark of this exact HEAD or as end-to-end SQL/RPC latency ratios. Current source and the resolved TsFile snapshot were checked against the affected paths.
The previously fixed binary serialization/WAL-size/DATE-split/MAX_BY/grouped-MAX/DATE-statistics/tablet-decoding findings are omitted. The earlier SELECT INTO numeric-widening allegation was disproved by the real planner/SQL path and is also omitted.
| dataType)); | ||
| } | ||
| private double getDoubleValue(Column column, int position) { | ||
| return column.getDouble(position); |
There was a problem hiding this comment.
[P1] Preserve native timestamp reads for TimeColumn inputs
The previous implementation read INT64/TIMESTAMP using column.getLong(position) and widened the result to double. column.getDouble(position) is not equivalent for the actual TimeColumn implementation in the pinned TsFile snapshot: it inherits the default method that throws UnsupportedOperationException.
This is reachable in table SQL. AbstractAggTableScanOperator.buildValueColumn returns inputRegion.getTimeColumn() for a TIME argument and places that same object in the aggregation block's valueColumns. Replacing the block's dedicated time slot with a placeholder does not convert this value-column object into a LongColumn. Table metadata accepts TIMESTAMP arguments for these functions.
Table-model reproducer using the values from the real-instance comparison:
CREATE DATABASE review18606_corr;
USE review18606_corr;
CREATE TABLE readings(device_id STRING TAG, d DOUBLE FIELD);
INSERT INTO readings(time,device_id,d)
VALUES (1,'neg',-8.5),(2,'neg',-2.75);
FLUSH;
SELECT corr(time,d),covar_pop(time,d),regr_slope(time,d)
FROM readings WHERE device_id='neg';The earlier BASE 25ab7941ce vs PR 9f4c0f0404 real-instance run returned [1.0, 1.4375, 0.17391304347826086] on BASE and 301: org.apache.tsfile.read.common.block.column.TimeColumn on the PR. Tree SQL and grouped variants reproduced the same failure. This getter and the relevant dependency remain unchanged at 6ac266b065; the TsFile artifact still resolves to 2.4.1-260909-20260909.035234-2.
Please preserve declared-type/native reads, or complete the concrete Column contracts before relying on the generic accessor. The equivalent correlation/covariance/regression changes in the tree, table and grouped implementations need the same treatment.
| nullCounts.increment(groupId); | ||
| } else { | ||
| countMap.compute( | ||
| column.getTsPrimitiveType(position), (key, count) -> count == null ? 1L : count + 1); |
There was a problem hiding this comment.
[P1] Preserve logical row access when constructing MODE keys
The old typed getters are not equivalent to getTsPrimitiveType(position) for two Column implementations used by this code:
TimeColumndoes not implement this method and throws.DictionaryColumn.getTsPrimitiveTypecurrently passes the logical position straight to its dictionary, without applyinggetId(position)as its typed getters do. MODE can therefore count values that are not present in its logical input.
Table-model time-column reproducer:
CREATE DATABASE review18606_mode;
USE review18606_mode;
CREATE TABLE readings(device_id STRING TAG, d DOUBLE FIELD);
INSERT INTO readings(time,device_id,d)
VALUES (1,'neg',-8.5),(2,'neg',-2.75);
FLUSH;
SELECT substring(device_id,1,1),mode(time)
FROM readings
GROUP BY substring(device_id,1,1)
ORDER BY 1;In the earlier real-instance comparison (BASE 25ab7941ce, PR 9f4c0f0404), the matching group returned time=1 on BASE, while the PR threw the TimeColumn exception.
A separate executed class-level test covers the dictionary mapping:
Underlying values: [10, 11, 12, 100, 100, 100]
Selected positions: [3, 4, 5]
Logical input: [100, 100, 100]
BASE MODE: 100
PR MODE: 10
The real DistinctGroupedAccumulator wrapper also reproduces the dictionary result with the corresponding selected mask. This is evidence for that concrete class-level path, not a claim that every MODE(DISTINCT ...) SQL plan fails. The call and relevant TsFile implementations remain unchanged at the current HEAD.
Please construct keys through declared-type native getters, or fix and validate both concrete Column implementations, including non-identity/repeated dictionary mappings.
| case INT32 -> (column, rowIndex) -> ((int[]) column)[rowIndex]; | ||
| case DATE -> | ||
| (column, rowIndex) -> | ||
| new DateTime(Date.valueOf(((LocalDate[]) column)[rowIndex])); |
There was a problem hiding this comment.
[P2] Do not pass java.sql.Date to Milo DateTime
Date here is java.sql.Date. Milo's DateTime(Date) constructor calls date.toInstant(), but java.sql.Date.toInstant() always throws UnsupportedOperationException. A normal DATE value therefore fails in the OPC UA Client/Server tablet path before it can be published.
Executed with the actual Milo dependency during the earlier review:
LocalDate date = LocalDate.of(2026, 9, 9);
new DateTime(java.sql.Date.valueOf(date));java.lang.UnsupportedOperationException
at java.sql.Date.toInstant(Date.java:316)
at org.eclipse.milo.opcua.stack.core.types.builtin.DateTime.<init>(DateTime.java:57)
The old java.util.Date.from(date.atStartOfDay(ZoneId.systemDefault()).toInstant()) expression succeeds for the same value. The conversion expression is still present in this HEAD. Please retain the java.util.Date/Instant conversion. This was verified at the actual conversion boundary; it was not a full OPC UA server integration test.
| final int columnIndex, | ||
| final long outputMinReportIntervalMilliseconds) | ||
| throws IOException { | ||
| if (rowValueUpdater == null) { |
There was a problem hiding this comment.
[P2] Refresh the cached row updater when a series changes type
AggregateProcessor retains this state by timeseries path, but rowValueUpdater is selected only for the first row's type. If the same path is recreated with a different type, later rows continue through the original getter. For example, an INT32 row followed by a DOUBLE row still calls PipeRow.getInt, which casts the DOUBLE column's double[] to int[] and fails before the window can handle the type transition.
Previously, AggregateProcessor dispatched on each row's actual type. TimeSeriesWindow explicitly handles a changed input type by purging the old window. Caching the getter indefinitely prevents reaching that existing behavior.
Executed dispatch-level reproduction with real PipeRow instances and the real TimeSeriesRuntimeState.updateWindows(..., Row, ...) entry point:
First row: INT32 [42] -> INT32 updater
Next row: DOUBLE [2.5] -> ClassCastException: [D cannot be cast to [I
Direct DOUBLE overload -> receives 2.5 normally
The typed overloads were instrumented to record dispatch; this test did not run the complete Pipe lifecycle. The state key, one-time initialization and primitive-array getters establish the failure path, and remain unchanged in this HEAD.
Please cache the associated data type together with the updater and refresh it on a type change, or dispatch using each row's actual type.
| } | ||
| } catch (DateTimeParseException e) { | ||
| throw new IoTDBRuntimeException( | ||
| "Year must be between 1000 and 9999.", |
There was a problem hiding this comment.
[P2] Retain the localized messages when moving the implementation
This catch previously used CalcMessages.EXCEPTION_YEAR_MUST_BETWEEN_1000_9999_8FBB94AA, but now emits a raw English literal. The same regression remains in the Cannot cast %s to %s type branches in this file and several SUM/AVG argument checks. These strings stay English in a Chinese-locale build even when zh-locale-compile passes.
Some of the earlier message-key feedback has been fixed, but these call sites still bypass the existing English/Chinese constants. Please restore the corresponding localized constants and keep any new messages in both locale files.
| String.format( | ||
| CalcMessages.EXCEPTION_UNSUPPORTED_DATA_TYPE_LAST_ARG_37F52124, seriesDataType)); | ||
| } | ||
| addInput(arguments[0], arguments[1], mask); |
There was a problem hiding this comment.
[P2] Keep the descending LAST/LAST_BY early-return path
The old switch invoked protected addIntInput/addLongInput/etc., so a LastDescAccumulator executed its overridden loop and returned after the first qualifying row. The factory still creates the descending subclass, but this new call enters a private superclass loop and bypasses those overrides. The equivalent LastByAccumulator change bypasses the LastByDescAccumulator optimization when canFinishAfterInit is true.
An executed BASE 25ab7941ce vs PR 9f4c0f0404 comparison instrumented the time Column on the same 1,024-row, descending, non-null block:
| Accumulator | BASE time.getLong calls | PR calls | Result |
|---|---|---|---|
| LastDescAccumulator | 1 | 1,024 | unchanged |
| LastByDescAccumulator | 1 | 1,024 | unchanged |
These are measured getter counts, not latency ratios. The outer scan can still inspect hasFinalResult() after the call and skip subsequent blocks; it cannot recover the work already done inside this block. The private loops remain unchanged at the current HEAD.
Please preserve the type-specific/overridable batch entry point or explicitly carry the descending early-return condition into the shared loop. Ascending or otherwise non-finalizable inputs must continue scanning.
| AGGREGATION_NUMERIC_COLUMN_TO_DOUBLE_CONVERTER_SERVICE = | ||
| type -> | ||
| switch (type.getTypeEnum()) { | ||
| case INT32, INT64, FLOAT, DOUBLE -> ignored -> type::getDouble; |
There was a problem hiding this comment.
[P2] Preserve mixed-type throughput when consolidating SUM's input loops
Before the refactor, SUM selected a type-specific loop once per block; the inner loop called getInt, getLong, getFloat or getDouble directly. Now all four types use the same converter expression and a common inner loop invoking valueConverter.convert(column, position) for each value. The converter is already stored per accumulator, so this is not an allegation of a new allocation or switch for every row.
I measured the actual BASE/PR SumAccumulator implementations in separate JVMs, with four accumulators (INT32/INT64/FLOAT/DOUBLE) alternating at 1,024-row batch boundaries. Both used the same TsFile dependency to isolate the Java refactor. Each run warmed up for 20.48 million rows, then took five samples of 4.096 million rows using ThreadMXBean current-thread CPU time. Final values were consumed into a volatile checksum (3.2664E7 on both sides), then accumulators were reset.
| Runtime / workload | BASE ns/value | PR ns/value |
|---|---|---|
| Corretto 17.0.5 arm64, mixed types | 0.89–0.92 | 5.60–5.69 |
| OpenJDK 24.0.2 arm64, mixed types | 0.887–0.902 | 5.49–5.65 |
| OpenJDK 24.0.2 arm64, INT32 only | about 0.90 | about 0.90 |
These measurements were taken during the earlier review against BASE 25ab7941ce and the PR implementation at 0b6c4eb8b6; they are not a new benchmark of 6ac266b065. The affected SUM loop and this conversion strategy are still unchanged. No extra per-value allocation was measured after warm-up. The shared polymorphic call path is a plausible JIT explanation, but I did not inspect generated assembly to establish the exact optimization lost.
This is an aggregation-kernel result, not evidence that end-to-end SQL is six times slower. Please retain/select type-specific batch strategies, or add an equivalent warmed mixed-type benchmark demonstrating that the shared loop preserves throughput. The new cached-service helper does not change this per-value path.
| return; | ||
| } | ||
| int middle = (from + to) >>> 1; | ||
| sortIndexes(index, scratch, from, middle, valueProvider); |
There was a problem hiding this comment.
[P2] Preserve the fast path for long ordered runs in large tablets
Previously the index array was Integer[] sorted with Arrays.sort(..., Comparator.comparingLong(...)), whose TimSort implementation recognizes existing ordered runs. This primitive merge sort reduces boxing, but unconditionally recurses, merges and copies at every level, even when the input is almost sorted. The normal insert path calls checkSorted first, so a fully sorted tablet skips sorting on both versions; a single inversion is enough to enter this more expensive path.
Executed benchmark of the real Session.sortTablet: 262,144 rows, one INT32 value column, 60 warm-up sorts followed by 21 samples (median shown). Tablet construction/cloning was outside the timing region; sorting timestamps/indices and reordering the value column were inside. Results were consumed through a volatile sink. BASE and PR ran in separate JVMs.
| Input / runtime | BASE ns/row | PR ns/row | PR / BASE |
|---|---|---|---|
| One inversion, Corretto 17.0.5 arm64 | 5.644 | 34.935 | 6.19x |
| One inversion, OpenJDK 24.0.2 arm64 | 5.628 | 29.466 | 5.24x |
| Reverse ordered, OpenJDK 24.0.2 arm64 | 5.745 | 28.368 | 4.94x |
| Random, OpenJDK 24.0.2 arm64 | 244.440 | 151.547 | 0.62x (improves) |
For the one-inversion JDK 24 case, this is approximately 1.48 ms -> 7.72 ms per sort. The input was an increasing timestamp sequence with only its final pair out of order. Smaller 1,024-row cases did not show a stable regression.
The measurements compare BASE 25ab7941ce with PR 9f4c0f0404; Session.java and this sorting path are unchanged at the current HEAD. They measure local sort latency, not complete RPC write throughput.
Please keep the primitive-array benefit while restoring ordered-run handling or skipping unnecessary merges, with equal-timestamp stability preserved. Random inputs improve, so simply reverting every part of the primitive-array change would lose that benefit.
| break; | ||
| default: | ||
| throw new UnSupportedDataTypeException(String.format(DATATYPE_UNSUPPORTED, dataType)); | ||
| if (!Type.fromTsDataType(dataType).arrayEquals(this.columns[i], columns[i], rowCount)) { |
There was a problem hiding this comment.
[P3] Keep hashCode consistent with active-row equality
equals now compares column values only through rowCount, but hashCode still uses Arrays.deepHashCode(columns) over the complete backing arrays. Two otherwise identical nodes with the same active rows and different spare-capacity values can therefore be equal while having different hashes.
The earlier real-node comparison reproduced equals == true, different hash codes, and HashSet.contains == false for such a pair. BASE considered the nodes unequal because it compared the complete arrays.
Please make the hash cover the same active values as equality, or retain consistent full-array semantics in both methods. This is a Java equality/hash contract issue; I have not identified a production caller using these nodes as hash keys, so I am not claiming an observed write/deduplication failure.
| try { | ||
| return TypeServices.NUMERIC_ROW_READER_SERVICE | ||
| .call(TypeServices.toReadType(row.getDataType(index))) | ||
| .read(row); |
There was a problem hiding this comment.
[P3] Pass the requested column index to the numeric reader
This public helper selects the type from row.getDataType(index), but NUMERIC_ROW_READER_SERVICE always reads column 0. The previous switch read row.getInt(index) / getLong(index) / etc. For a real RowImpl with two INT32 values [11, 22], the executed BASE/PR comparison of getValueAsDouble(row, 1) returned 22.0 before the refactor and 11.0 afterwards.
Please use the indexed numeric reader and pass index through. The current single-input Envelope transform no longer calls this helper, and I did not find another in-repository production caller, so this is a low-priority helper-contract regression rather than a demonstrated ordinary Envelope SQL failure.
What is changed
Validation