Skip to content

Avoid eager aggregation type coverage checks on first query - #18915

Merged
jt2594838 merged 2 commits into
apache:masterfrom
hongzhi-gao:fix/aggregation-eager-type-validation
Oct 20, 2026
Merged

jt2594838 merged 2 commits into
apache:masterfrom
hongzhi-gao:fix/aggregation-eager-type-validation

Conversation

@hongzhi-gao

@hongzhi-gao hongzhi-gao commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Description

Reduce first-query aggregation latency introduced by #18606 by moving exhaustive type-coverage validation out of TypeServices.Aggregation class initialization.

Problem and fix

A simple SELECT LAST ... initializes TypeServices.Aggregation. Its static initializer calls .check() on six services, traversing all data types and constructing strategies unrelated to the requested aggregation. In particular, checking the max/min-by strategy also initializes the calc type-service registry. max_time queries can hit the same initialization path through aggregation size calculation.

This work is paid on first use. A benchmark that restarts the database before each query workload repeatedly includes that cost in its reported average. In a separate pre/post-#18606 diagnostic, the first LAST query increased from 315 ms to 433 ms, while the other 99 queries averaged 9.95 ms and 9.86 ms respectively. That adjacent-commit comparison also changed the TsFile dependency; the controlled fix comparison below keeps the source revision and dependencies constant.

Move the six existing checks, unchanged and in the same order, into TypeServicesTest.testAggregationServicesCoverAllTypes(). The checks construct and discard strategies; they do not perform required registration. Actual strategy factories, supported-type dispatch, aggregation algorithms and exception behavior at use sites remain unchanged. Real max/min-by queries still initialize the calc services they need. There are no file-format, configuration, persistence or concurrency-mechanism changes.

Performance results

Repository and versions tested: both server distributions were built from Apache IoTDB (apache/iotdb). The controlled comparison uses unmodified a670bce7ec6b9e16ebdd8c9beb09197058785865 (#18606) versus the same revision plus this patch, with the same TsFile 2.4.1-260915-SNAPSHOT and benchmark client libraries. These numbers were measured on that historical revision, not on the latest master base of this PR. The complete unpatched Aggregation class is identical on that revision and the PR base (4c34a253927).

Environment: 8 vCPUs, 31 GiB RAM, Java 17.0.9, VMware, ext4; one ConfigNode and one DataNode, 20 GiB DataNode heap, all three compaction modes disabled. The existing aligned dataset contains 2,402 TsFiles (approximately 16 GiB). Each round restores the copied cluster metadata and starts fresh database JVMs. A private network namespace avoids collisions with another workload on the same host. Host page caches were not explicitly evicted in this matrix.

The two workloads use one benchmark client. Each generated query reads one measurement from one device, selected reproducibly from 100 devices and 300 measurements using query seed 151658:

  1. Latest-value queries: 100 queries per database restart. The first generated SQL is:

    SELECT LAST s_12 FROM root.test.g_0.d_37;
  2. Maximum timestamp within a time range: 20 queries per database restart. The first generated SQL is:

    SELECT max_time(s_124)
    FROM root.test.g_0.d_37
    WHERE time >= 1637424000000 AND time <= 1638424000000;

These are the first statements in the generated workloads, rather than one identical statement repeated throughout each round. Each round includes the first query after database restart and the subsequent queries in the same JVMs. Each workload was tested in three alternating/reversed-order pairs, without the per-query timing agent. The table reports the average latency per query within each round and the mean of those three round averages:

Query workload Unpatched round means (ms) Patched round means (ms) Mean before → after Reduction
Latest value (SELECT LAST) 14.24 / 10.62 / 13.96 12.96 / 9.77 / 10.24 12.94 → 10.99 ms 15.1%
Maximum timestamp in a time range (SELECT max_time) 52.02 / 44.07 / 32.85 27.57 / 26.99 / 27.90 42.98 → 27.49 ms 36.0%

An additional, separately instrumented pair of latest-value workloads measured first-query latency of 388.27 → 308.97 ms. The following 99 queries averaged 10.67 → 11.25 ms, so this is evidence of reducing first-use initialization cost, not a demonstrated steady-state speedup. The host was shared with another benchmark, and the round-to-round variation above limits broader performance claims.

Across the 12 ordinary rounds and two diagnostic rounds, 920 benchmark queries completed with zero failures. After every round, all 300 measurements returned by a SELECT LAST * sanity query matched exactly across versions, as did an additional seven-function query covering max_time, last_value, first_value, max_by, min_by, extreme and mode. These are targeted result checks rather than exhaustive dataset validation.

Validation

  • The same patch on the historical benchmark revision passed the focused reactor tests (46 tests: 32 DataNode, 12 calc and 2 subscription; no failures/errors/skips) and a distribution package build.
  • On the latest master base (4c34a253927) plus this commit, spotless:apply and the focused reactor test command below completed successfully: 46 tests, zero failures/errors/skips (32 DataNode, 12 calc and 2 subscription). git diff --check also passed.
  • Independent source review found no actionable issues; all six original coverage checks are retained in the unit test.

Commands:

mvn spotless:apply -o -pl iotdb-core/datanode -Ddevelocity.off=true
mvn test -o -pl iotdb-core/datanode -am \
  -Dtest=TypeServicesTest,AccumulatorTest \
  -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false \
  -Ddevelocity.off=true

This PR has:

  • been self-reviewed.
  • moved the existing coverage checks into a unit test.
  • been tested with real queries against existing persisted aligned data in a 1 ConfigNode / 1 DataNode deployment.

Key changed classes: TypeServices.Aggregation, TypeServicesTest.

@Caideyipi Caideyipi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the code and PR description. No actionable code issues found; the deferred TypeService checks are retained in TypeServicesTest, and the English/Markdown formatting is acceptable. The remaining dual-table manual CI failures should be rerun or triaged separately.

@jt2594838
jt2594838 merged commit f709ef5 into apache:master Oct 20, 2026
37 of 39 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.

3 participants