You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@tsfile.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/06/16 06:22:21 UTC
Re: [PR] Optimize timeseries metadata read and fix table size accounting [tsfile]
Copilot commented on code in PR #840:
URL: https://github.com/apache/tsfile/pull/840#discussion_r3418546275
##########
java/tsfile/src/main/java/org/apache/tsfile/read/TsFileSequenceReader.java:
##########
@@ -927,19 +973,18 @@ public List<TimeseriesMetadata> readTimeseriesMetadata(
/* Get leaf MetadataIndexPair which contains path */
private Pair<IMetadataIndexEntry, Long> getLeafMetadataIndexPair(
- IDeviceID device, String measurement, LongConsumer ioSizeRecorder) throws IOException {
- readFileMetadata(ioSizeRecorder);
- MetadataIndexNode deviceMetadataIndexNode =
- tsFileMetaData.getTableMetadataIndexNode(device.getTableName());
- Pair<IMetadataIndexEntry, Long> metadataIndexPair =
- getMetadataAndEndOffsetOfDeviceNode(deviceMetadataIndexNode, device, true, ioSizeRecorder);
- if (metadataIndexPair == null) {
- return null;
- }
- ByteBuffer buffer =
- readData(metadataIndexPair.left.getOffset(), metadataIndexPair.right, ioSizeRecorder);
- MetadataIndexNode metadataIndexNode = deviceMetadataIndexNode;
- if (!metadataIndexNode.getNodeType().equals(MetadataIndexNodeType.LEAF_MEASUREMENT)) {
+ IDeviceID device,
+ long[] deviceMetadataIndexNodeOffset,
+ String measurement,
+ LongConsumer ioSizeRecorder)
+ throws IOException {
+ Pair<IMetadataIndexEntry, Long> metadataIndexPair;
+ MetadataIndexNode metadataIndexNode;
+ ByteBuffer buffer;
+ if (deviceMetadataIndexNodeOffset != null) {
+ buffer =
+ readData(
+ deviceMetadataIndexNodeOffset[0], deviceMetadataIndexNodeOffset[1], ioSizeRecorder);
try {
Review Comment:
`deviceMetadataIndexNodeOffset` is used as a `[startOffset, endOffset]` pair but the code does not validate array length before indexing. A malformed input array will throw `ArrayIndexOutOfBoundsException`; please validate and raise a clear error.
##########
java/tsfile/src/main/java/org/apache/tsfile/read/TsFileSequenceReader.java:
##########
@@ -732,22 +732,23 @@ public TimeseriesMetadata readTimeseriesMetadata(
boolean ignoreNotExistDevice,
LongConsumer ioSizeConsumer)
throws IOException {
- readFileMetadata(ioSizeConsumer);
- MetadataIndexNode deviceMetadataIndexNode =
- tsFileMetaData.getTableMetadataIndexNode(device.getTableName());
- Pair<IMetadataIndexEntry, Long> metadataIndexPair =
- getMetadataAndEndOffsetOfDeviceNode(deviceMetadataIndexNode, device, true, ioSizeConsumer);
- if (metadataIndexPair == null) {
- if (ignoreNotExistDevice) {
- return null;
- }
- throw new IOException(
- Messages.format("error.read.device_not_in_metadata_file", device, file));
- }
- ByteBuffer buffer =
- readData(metadataIndexPair.left.getOffset(), metadataIndexPair.right, ioSizeConsumer);
- MetadataIndexNode metadataIndexNode = deviceMetadataIndexNode;
- if (!metadataIndexNode.getNodeType().equals(MetadataIndexNodeType.LEAF_MEASUREMENT)) {
+ return readTimeseriesMetadata(device, null, measurement, ignoreNotExistDevice, ioSizeConsumer);
+ }
+
+ public TimeseriesMetadata readTimeseriesMetadata(
+ IDeviceID device,
+ long[] deviceMetadataIndexNodeOffset,
+ String measurement,
+ boolean ignoreNotExistDevice,
+ LongConsumer ioSizeConsumer)
+ throws IOException {
+ Pair<IMetadataIndexEntry, Long> metadataIndexPair;
+ MetadataIndexNode metadataIndexNode;
+ ByteBuffer buffer;
+ if (deviceMetadataIndexNodeOffset != null) {
+ buffer =
+ readData(
+ deviceMetadataIndexNodeOffset[0], deviceMetadataIndexNodeOffset[1], ioSizeConsumer);
try {
Review Comment:
When the fast-path `deviceMetadataIndexNodeOffset` is provided, the code indexes `[0]`/`[1]` without validating array length. Since this overload is public API, callers can accidentally pass an empty/1-element array and trigger `ArrayIndexOutOfBoundsException`. Consider validating the input and failing fast with a clear `IllegalArgumentException`.
##########
java/tsfile/src/test/java/org/apache/tsfile/write/TsFileWriteApiTest.java:
##########
@@ -1280,6 +1280,47 @@ public void calculateTableSize() throws IOException, WriteProcessException {
Assert.assertTrue(tableSizeMap.get("table2") >= 1024 * 1024);
}
+ @Test
+ public void calculateTableSize2() throws IOException, WriteProcessException {
+ TableSchema tableSchema1 =
+ new TableSchema(
+ "table1",
+ Arrays.asList(
+ new ColumnSchema("device", TSDataType.STRING, ColumnCategory.TAG),
+ new ColumnSchema("s1", TSDataType.BLOB, ColumnCategory.FIELD)));
+ TableSchema tableSchema2 =
+ new TableSchema(
+ "table2",
+ Arrays.asList(
+ new ColumnSchema("device", TSDataType.STRING, ColumnCategory.TAG),
+ new ColumnSchema("s1", TSDataType.BLOB, ColumnCategory.FIELD)));
+ Tablet tablet1 =
+ new Tablet(
+ "table1",
+ IMeasurementSchema.getMeasurementNameList(tableSchema1.getColumnSchemas()),
+ IMeasurementSchema.getDataTypeList(tableSchema1.getColumnSchemas()),
+ tableSchema1.getColumnTypes());
+ tablet1.addTimestamp(0, 0);
+ tablet1.addValue(0, 0, new byte[1024]);
+ Tablet tablet2 =
+ new Tablet(
+ "table2",
+ IMeasurementSchema.getMeasurementNameList(tableSchema2.getColumnSchemas()),
+ IMeasurementSchema.getDataTypeList(tableSchema2.getColumnSchemas()),
+ tableSchema2.getColumnTypes());
+ tablet2.addTimestamp(0, 0);
+ tablet2.addValue(0, 0, new byte[1024 * 1024]);
+ Map<String, Long> tableSizeMap = null;
+ try (TsFileWriter writer = new TsFileWriter(f)) {
+ writer.registerTableSchema(tableSchema1);
+ writer.registerTableSchema(tableSchema2);
+ writer.writeTable(tablet1);
+ writer.writeTable(tablet2);
+ tableSizeMap = writer.getIOWriter().getTableSizeMap();
+ }
+ Assert.assertTrue(tableSizeMap.get("table1") > 1024);
Review Comment:
If `tableSizeMap` (or the "table1" entry) is missing, this assertion will fail with an NPE rather than a clear test failure. Adding explicit non-null/contains checks makes the intent clearer and improves diagnostics.
##########
java/tsfile/src/test/java/org/apache/tsfile/read/TsFileDeviceIteratorTest.java:
##########
@@ -102,6 +103,33 @@ public void test() throws IOException {
}
}
+ @Test
+ public void testReadTimeseriesMetadataWithDeviceMetadataIndexNodeOffset() throws IOException {
+ try (TsFileIOWriter writer = new TsFileIOWriter(new File(FILE_PATH))) {
+ registerTableSchema(writer, "table1");
+ generateDevice(writer, "table1", 10);
+ writer.endFile();
+ }
+
+ try (TsFileSequenceReader reader = new TsFileSequenceReader(FILE_PATH)) {
+ TsFileDeviceIterator deviceIterator =
+ reader.getTableDevicesIteratorWithIsAligned("table1", null);
+ Assert.assertTrue(deviceIterator.hasNext());
+ Pair<IDeviceID, Boolean> currentDevice = deviceIterator.next();
+ long[] deviceMetadataIndexNodeOffset = deviceIterator.getCurrentDeviceMeasurementNodeOffset();
+
+ TimeseriesMetadata metadataWithoutOffset =
+ reader.readTimeseriesMetadata(currentDevice.getLeft(), "s1", false);
+ TimeseriesMetadata metadataWithOffset =
+ reader.readTimeseriesMetadata(
+ currentDevice.getLeft(), deviceMetadataIndexNodeOffset, "s1", false, null);
+
Review Comment:
If `metadataWithOffset` is unexpectedly null, the subsequent assertions will throw an NPE and hide the actual failure. Asserting non-null right after the call makes the test failure easier to diagnose.
##########
java/tsfile/src/test/java/org/apache/tsfile/read/TsFileDeviceIteratorTest.java:
##########
@@ -102,6 +103,33 @@ public void test() throws IOException {
}
}
+ @Test
+ public void testReadTimeseriesMetadataWithDeviceMetadataIndexNodeOffset() throws IOException {
+ try (TsFileIOWriter writer = new TsFileIOWriter(new File(FILE_PATH))) {
+ registerTableSchema(writer, "table1");
+ generateDevice(writer, "table1", 10);
+ writer.endFile();
+ }
+
+ try (TsFileSequenceReader reader = new TsFileSequenceReader(FILE_PATH)) {
+ TsFileDeviceIterator deviceIterator =
+ reader.getTableDevicesIteratorWithIsAligned("table1", null);
+ Assert.assertTrue(deviceIterator.hasNext());
+ Pair<IDeviceID, Boolean> currentDevice = deviceIterator.next();
+ long[] deviceMetadataIndexNodeOffset = deviceIterator.getCurrentDeviceMeasurementNodeOffset();
+
Review Comment:
This test assumes the iterator always provides a non-null 2-element offset array. Adding explicit assertions makes failures clearer (instead of an `ArrayIndexOutOfBoundsException`/NPE inside the reader) and documents the contract for `getCurrentDeviceMeasurementNodeOffset()`.
--
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: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org