Copilot commented on code in PR #17285:
URL: https://github.com/apache/iotdb/pull/17285#discussion_r2918906203
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/tsfile/timeindex/ArrayDeviceTimeIndex.java:
##########
@@ -95,7 +95,7 @@ public ArrayDeviceTimeIndex(
@Override
public void serialize(OutputStream outputStream) throws IOException {
- ReadWriteIOUtils.write(getTimeIndexType(), outputStream);
+ ReadWriteIOUtils.write(ARRAY_DEVICE_TIME_INDEX_TYPE, outputStream);
Review Comment:
`ArrayDeviceTimeIndex.serialize` now always writes
`ARRAY_DEVICE_TIME_INDEX_TYPE` instead of `getTimeIndexType()`. This breaks
polymorphism (e.g., `PlainDeviceTimeIndex` overrides `getTimeIndexType()`), and
makes it easy for subclasses to accidentally be serialized with a type that
doesn't match their reported `getTimeIndexType()`. If the goal is to migrate
legacy `PlainDeviceTimeIndex` to the array format on write, consider keeping
`ArrayDeviceTimeIndex` using `getTimeIndexType()` and handling the migration
explicitly (e.g., override `serialize` in `PlainDeviceTimeIndex` to write as
array, or convert the timeIndex instance before serialization).
```suggestion
ReadWriteIOUtils.write(getTimeIndexType(), outputStream);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/tsfile/timeindex/PlainDeviceTimeIndex.java:
##########
@@ -28,18 +28,12 @@
import java.io.IOException;
import java.io.InputStream;
-import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.util.HashSet;
import java.util.Set;
public class PlainDeviceTimeIndex extends ArrayDeviceTimeIndex implements
ITimeIndex {
- @Override
- public void serialize(OutputStream outputStream) throws IOException {
- throw new UnsupportedOperationException();
- }
-
@Override
public PlainDeviceTimeIndex deserialize(
InputStream inputStream, IDeviceID.Deserializer deserializer) throws
IOException {
Review Comment:
Removing the `serialize(OutputStream)` override changes
`PlainDeviceTimeIndex` from being non-serializable (it used to throw
`UnsupportedOperationException`) to being serialized via
`ArrayDeviceTimeIndex.serialize()`. With the current implementation, that
serialization writes `ARRAY_DEVICE_TIME_INDEX_TYPE`, while
`PlainDeviceTimeIndex.getTimeIndexType()` still reports
`PLAIN_DEVICE_TIME_INDEX_TYPE`, which is a surprising mismatch for callers and
future maintainers. It would be clearer to either align `getTimeIndexType()`
with the serialized type, or add an explicit `serialize` override / conversion
path documenting that this class is only for legacy deserialization and is
always written back in array format.
--
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]
