CritasWang commented on PR #115:
URL: https://github.com/apache/iotdb-extras/pull/115#issuecomment-5066705193
The aggregation implementation reads well overall — the `date_bin`
anchoring, calendar boundary walk, SUM precision guard (`sumIsProvablyExact` +
Java re-sum fallback), long-only MIN/MAX channel, and empty-bucket detection
via `MAX(time) IS NULL` are all correct and well-documented. Test coverage is
substantial. Two things to address before merge:
**1. Nested result set in `exactLongSum` — correctness concern**
In both the milliseconds and calendar aggregation paths, `exactLongSum`
opens a second `SessionDataSet` on the **same** `ITableSession` while the outer
aggregate result set is still open inside its `try-with-resources` block:
```java
// milliseconds path — outer dataSet still open:
try (ITableSession session = tableSessionPool.getSession();
SessionDataSet dataSet = session.executeQueryStatement(sql)) {
while (row.next()) {
KvEntry value = aggregatedEntry(aggregation, row,
new SumReSumContext(session, ...)); // may call exactLongSum
// exactLongSum: session.executeQueryStatement(reSumSql) — second
open RS
}
}
```
Whether IoTDB's `ITableSession` supports two concurrently open result sets
on the same connection is not obvious from the client-go / Java client docs. If
it does not (the common case for most DB clients), the re-sum query would
either throw or silently close the outer result set, corrupting the remaining
bucket rows. The fast path (`sumIsProvablyExact`) avoids this, but the fallback
is reachable for any long-only bucket where `count * maxAbs > 2^53`.
The fix is straightforward: acquire a **separate** session from the pool
inside `exactLongSum` rather than reusing the one passed through
`SumReSumContext`. The `SumReSumContext` record can drop the `session` field
entirely (or keep it for the calendar path where the outer RS is a single-row
aggregate and is fully consumed before `aggregatedEntry` is called — but the
milliseconds path iterates while calling it, so a separate session is the safe
choice for both).
**2. Empty entity list in `buildFindAllKeysByEntityIdsSql`**
If `entityIds` is empty the method emits `WHERE tenant_id=... AND ()`, which
is invalid SQL and will throw at the server. ThingsBoard likely never calls
`findAllKeysByEntityIds` with an empty list in practice, but the SPI contract
does not forbid it and a defensive early-return (`if (entityIds.isEmpty())
return List.of()`) in `doFindAllKeysByEntityIds` costs nothing.
CI is still running on this commit — waiting for `code-analyze (java)` and
the compile-check matrix before the merge decision.
--
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]
