Copilot commented on code in PR #17225:
URL: https://github.com/apache/iotdb/pull/17225#discussion_r2925150176
##########
iotdb-client/session/src/main/java/org/apache/iotdb/session/subscription/payload/SubscriptionTsFileHandler.java:
##########
@@ -36,8 +38,12 @@ public SubscriptionTsFileHandler(final String absolutePath,
@Nullable final Stri
this.databaseName = databaseName;
}
- public TsFileReader openReader() throws IOException {
- return new TsFileReader(new TsFileSequenceReader(absolutePath));
+ @SuppressWarnings("unchecked")
+ public <T extends AutoCloseable> T openReader() throws IOException {
+ return (T)
+ (Objects.isNull(databaseName)
+ ? new TsFileTreeReaderBuilder().file(new
File(absolutePath)).build()
+ : new TsFileReaderBuilder().file(new File(absolutePath)).build());
}
Review Comment:
The generic method signature `<T extends AutoCloseable> T openReader()` is
type-unsafe. It relies on unchecked cast inference and the caller can assign
the result to any `AutoCloseable` subtype without a compiler warning — but a
`ClassCastException` will occur at runtime if the inferred type doesn't match
the actual return type. For example, in `AbstractSubscriptionTreeRegressionIT`
(lines 346, 413, 464), `openReader()` is called as `TsFileReader tsFileReader =
tsFileHandler.openReader()` (no explicit cast), which will silently infer `T =
TsFileReader`. If the builder returns a different type (e.g.
`ITsFileTreeReader`), this will fail at runtime.
A safer approach is to either:
1. Return `AutoCloseable` (or a common interface like `ITsFileTreeReader` /
`ITsFileReader`) and let callers cast explicitly, making the risk visible.
2. Provide two separate methods (e.g. `openTreeReader()` /
`openTableReader()`) with concrete return types.
##########
iotdb-client/session/src/main/java/org/apache/iotdb/session/subscription/payload/SubscriptionMessage.java:
##########
@@ -98,20 +100,31 @@ public String toString() {
/////////////////////////////// handlers ///////////////////////////////
- public SubscriptionSessionDataSetsHandler getSessionDataSetsHandler() {
- if (handler instanceof SubscriptionSessionDataSetsHandler) {
- return (SubscriptionSessionDataSetsHandler) handler;
+ public List<ResultSet> getRecords() {
+ if (handler instanceof SubscriptionRecordHandler) {
+ return ((SubscriptionRecordHandler) handler).getRecords();
+ }
+ throw new SubscriptionIncompatibleHandlerException(
+ String.format("%s do not support getRecords().",
handler.getClass().getSimpleName()));
+ }
Review Comment:
The new public API exposes `List<ResultSet>` from `getRecords()`, but almost
every consumer site immediately downcasts the `ResultSet` to
`SubscriptionRecordHandler.SubscriptionRecord` to call `.getTablet()`,
`.hasNext()`, `.nextRecord()`, `.getColumnNames()`, etc. This defeats the
purpose of the refactoring — the API promises a standard `ResultSet`
abstraction, but it isn't actually usable without the downcast.
Consider either:
1. Exposing `List<SubscriptionRecordHandler.SubscriptionRecord>` directly
from `getRecords()` (since that's what all callers need).
2. Adding convenience methods on `SubscriptionMessage` for the most common
operations (like the existing `getRecordTabletIterator()`) so callers don't
need to know about `SubscriptionRecord` at all.
##########
integration-test/src/test/java/org/apache/iotdb/subscription/it/triple/treemodel/regression/AbstractSubscriptionTreeRegressionIT.java:
##########
@@ -343,7 +342,7 @@ public List<Integer>
consume_tsfile(SubscriptionTreePullConsumer consumer, List<
for (final SubscriptionMessage message : messages) {
onReceived.incrementAndGet();
// System.out.println(FORMAT.format(new Date()) + " onReceived=" +
onReceived.get());
- final SubscriptionTsFileHandler tsFileHandler =
message.getTsFileHandler();
+ final SubscriptionTsFileHandler tsFileHandler = message.getTsFile();
try (final TsFileReader tsFileReader = tsFileHandler.openReader()) {
Review Comment:
These three call sites use `openReader()` without an explicit cast (e.g.
`TsFileReader tsFileReader = tsFileHandler.openReader()`), relying on the
generic type inference of the new `<T extends AutoCloseable> T openReader()`
method. While this compiles, if the builder returns a type other than
`TsFileReader` (e.g., `ITsFileTreeReader`), it will fail with a
`ClassCastException` at runtime. This should be updated to use an explicit cast
consistent with the other integration tests, or better yet, use the actual
return type of the builder.
```suggestion
try (final TsFileReader tsFileReader = (TsFileReader)
tsFileHandler.openReader()) {
```
##########
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/subscription/config/TopicConstant.java:
##########
@@ -41,9 +41,14 @@ public class TopicConstant {
public static final String MODE_DEFAULT_VALUE = MODE_LIVE_VALUE;
public static final String FORMAT_KEY = "format";
- public static final String FORMAT_SESSION_DATA_SETS_HANDLER_VALUE =
"SessionDataSetsHandler";
- public static final String FORMAT_TS_FILE_HANDLER_VALUE = "TsFileHandler";
- public static final String FORMAT_DEFAULT_VALUE =
FORMAT_SESSION_DATA_SETS_HANDLER_VALUE;
+ public static final String FORMAT_RECORD_HANDLER_VALUE =
"SubscriptionRecordHandler";
+ public static final String FORMAT_TS_FILE_VALUE =
"SubscriptionTsFileHandler";
+ public static final String FORMAT_DEFAULT_VALUE =
FORMAT_RECORD_HANDLER_VALUE;
+
+ @Deprecated
+ public static final String FORMAT_SESSION_DATA_SETS_HANDLER_VALUE =
FORMAT_RECORD_HANDLER_VALUE;
+
+ @Deprecated public static final String FORMAT_TS_FILE_HANDLER_VALUE =
FORMAT_TS_FILE_VALUE;
Review Comment:
The `@Deprecated` constants have had their values silently changed.
`FORMAT_TS_FILE_HANDLER_VALUE` was previously `"TsFileHandler"` and is now
`"SubscriptionTsFileHandler"`. Similarly,
`FORMAT_SESSION_DATA_SETS_HANDLER_VALUE` was `"SessionDataSetsHandler"` and is
now `"SubscriptionRecordHandler"`.
Any external code that was using these deprecated constants as string values
(e.g., stored in topic configurations or compared against persisted strings)
will break silently. The `isTsFileFormat()` method in `TopicConfig` correctly
handles the legacy `"TsFileHandler"` string, but there's no equivalent backward
compat for the old `"SessionDataSetsHandler"` string in the non-TsFile path. If
existing topics were created with `format=SessionDataSetsHandler`, the behavior
may change.
Consider preserving the original string values in the deprecated constants,
or adding a legacy check for `"SessionDataSetsHandler"` similar to
`LEGACY_FORMAT_TS_FILE_HANDLER_VALUE`.
--
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]
