Skip to content

Add date_bin aggregation and telemetry key discovery to the Table Mode DAOs#115

Merged
CritasWang merged 2 commits into
apache:masterfrom
PDGGK:feature/pr3-aggregation-keydiscovery
Jul 24, 2026
Merged

Add date_bin aggregation and telemetry key discovery to the Table Mode DAOs#115
CritasWang merged 2 commits into
apache:masterfrom
PDGGK:feature/pr3-aggregation-keydiscovery

Conversation

@PDGGK

@PDGGK PDGGK commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the aggregation and telemetry key-discovery parts of the ThingsBoard Table Mode integration, on top of the merged BaseDao / TimeseriesDao / LatestDao (#110, #113).

Changes

  • Aggregated reads (IoTDBTableTimeseriesDao): date_bin bucketed aggregation covering fixed-width (millisecond) buckets and calendar buckets (WEEK / MONTH / QUARTER). Calendar bucket boundaries are computed on the Java side (via TimeUtils) to avoid time-zone drift.
  • Key discovery (IoTDBTableLatestDao): findAllKeysByEntityIds / findAllKeysByTenant now union the telemetry and telemetry_latest sources, so keys that only ever appear in the latest overlay are also discovered.

Tests

  • Unit tests: IoTDBTableTimeseriesDaoTest (74) and IoTDBTableLatestDaoTest (40) cover the aggregation and key-discovery logic.
  • Container integration tests (testcontainers, apache/iotdb:2.0.8-standalone): IoTDBTableTimeseriesAggregationIT (11), IoTDBTableLatestDaoIT (19), IoTDBTableTimeseriesDaoIT (10). All green.

…e DAOs

Implements the aggregation and key-discovery parts of the ThingsBoard
Table Mode integration on top of the merged BaseDao / TimeseriesDao /
LatestDao:

- IoTDBTableTimeseriesDao: date_bin bucketed aggregation for the
  aggregated read path, covering fixed-width (millisecond) buckets and
  calendar buckets (WEEK / MONTH / QUARTER) with time-zone-aware
  boundaries computed on the Java side to avoid time-zone drift.
- IoTDBTableLatestDao: telemetry key discovery
  (findAllKeysByEntityIds / findAllKeysByTenant) now unions the telemetry
  and telemetry_latest sources so latest-only keys are also discovered.

Adds unit tests and container integration tests.

Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>
@CritasWang

Copy link
Copy Markdown
Contributor

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:

// 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.

exactLongSum re-queried the bucket's raw long_v values on the same
ITableSession that was iterating the outer aggregate result set. IoTDB
Table Mode does not guarantee two concurrently open result sets on one
session, so the re-sum fallback now checks out its own pooled session
and SumReSumContext no longer carries the session.

Addresses review feedback on apache#115.

Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>
@PDGGK

PDGGK commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review, @CritasWang.

1. Nested result set in exactLongSum — good catch, adopted. exactLongSum now checks out its own session from the pool instead of reusing the one that is iterating the outer aggregate, so it never opens a second result set on that session; SumReSumContext no longer carries the session. This fallback path is exercised end-to-end by IoTDBTableTimeseriesAggregationIT: the > 2^53 precision case re-sums the first of three buckets while the outer aggregate result set still has the remaining buckets pending, and it stays green against the real apache/iotdb:2.0.8 container with the separate session. Pushed as a follow-up commit.

2. Empty entity list in buildFindAllKeysByEntityIdsSql — this case is already guarded one level up: doFindAllKeysByEntityIds early-returns List.of() when entityIds.isEmpty(), so the SQL builder's AND (...) clause is never reached with an empty list. It's covered by the findAllKeysByEntityIds_emptyListReturnsEmptyAndSkipsQuery unit test. Happy to move the guard directly into buildFindAllKeysByEntityIdsSql for defense-in-depth if you'd prefer it at the builder.

@CritasWang

Copy link
Copy Markdown
Contributor

Both resolved — thanks.

1. Nested result set: the fix is correct. exactLongSum now checks out its own pooled session in its own try-with-resources, so the re-sum never opens a second result set on the session iterating the outer aggregate, and SumReSumContext dropped the session field. Good that IoTDBTableTimeseriesAggregationIT exercises the fallback where the re-sum runs while the outer aggregate still has pending buckets — that's exactly the case the bug would have corrupted, and it's green against the real 2.0.8 container.

2. Empty entity list: my mistake — you're right, doFindAllKeysByEntityIds already early-returns List.of() on an empty list (covered by findAllKeysByEntityIds_emptyListReturnsEmptyAndSkipsQuery), so the builder never sees it. No need to move the guard down; the current placement is fine.

CI is fully green on 970feca (CodeQL, code-analyze java/go/ts, compile-check 8/11/17/21, dependency-check, todo-check). LGTM — ready to merge from my side.

@CritasWang
CritasWang merged commit f3a0fe3 into apache:master Jul 24, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants