wu-sheng opened a new pull request, #13961: URL: https://github.com/apache/skywalking/pull/13961
### Fix silent truncation of BanyanDB query results at the engine's implicit row cap — https://github.com/apache/skywalking/discussions/13960 - [x] Add a unit test to verify that the fix works. - [x] Explain briefly why the bug exists and how to fix it. **Why the bug exists.** BanyanDB applies its own default limit to any query that carries none — `defaultLimit uint32 = 100` for measures (`pkg/query/logical/measure/measure_analyzer.go`, mirrored in the vectorized planner) and `20` for streams/traces. The analyzer applies it *after* any `GROUP BY` (`plan = limit(plan, offset, limitParameter)` wraps the whole plan), so an over-long result set is **silently truncated rather than rejected**. OAP never sent a `LIMIT` on several read paths. `BanyanDBMetricsQueryDAO.queryByEntityID` built `Conditions.create().eq(ENTITY_ID, id)` with no limit, so an entity-scoped metrics query returned at most 100 data points regardless of the requested range — a 4-hour minute-step read (240 points) rendered only its first 100 minutes, and the remaining 140 fell into the `emptyValue = true` branch. That is well inside what the query layer considers legal: `DurationUtils.MAX_TIME_RANGE = 500`, which throws a descriptive error rather than truncating. The same cap silently shortened topology relation maps (6 limit-less queries, including the whole-range ones), instance/process metadata lists, profiling thread snapshots, eBPF task lists, and the `multiGet` read-before-write in the metrics aggregation path. This is not a regression from the BydbQL migration (#13947) — `git show 5f6496e72c^` has the same limit-less query, and `defaultLimit = 100` has been in BanyanDB since apache/skywalking-banyandb#234. **How it is fixed.** Every BydbQL statement now leaves OAP with an explicit `LIMIT`, so nothing depends on the engine's default: - **Parameter-derived, where the request has a bound.** The entity-scoped metrics read (`readMetricsValues` / `readLabeledMetricsValues` / `readHeatMap`) sends `LIMIT = duration.assembleDurationPoints().size()` — the exact row set the ES and JDBC DAOs fetch by explicit document id, so the only remaining ceiling is the existing `DurationUtils.MAX_TIME_RANGE` gate. Ad-hoc `SELECT TOP` sends its own `N`. Paginated reads (traces, logs, alarms, events, browser logs) already passed the caller's `limit`/`from` and are unchanged. - **Config-derived, where it has none.** Reads with no user-supplied bound fall back to the existing `resultWindowMaxSize` knob (default `10000`, `SW_STORAGE_BANYANDB_QUERY_MAX_WINDOW_SIZE`) via a new `Conditions#limitIfAbsent`, applied in the stream / measure / trace query helpers in `AbstractBanyanDBDAO` that every DAO funnels through. The knob's documented meaning is widened accordingly in `bydb.yml` and the storage doc. It is read off `BanyanDBStorageClient` so the config does not have to be threaded through ~30 DAO constructors. The fallback is *spliced in* at the start of the pagination tail rather than appended, so it still lands ahead of an `OFFSET` that was set first (`... LIMIT ? OFFSET ?`) and keeps `WITH QUERY_TRACE` at the position the grammar mandates. It is a no-op when the caller set a limit, and idempotent. `SHOW TOP` needs no change — it maps to a `TopNRequest`, which has no `LIMIT` clause in the grammar and does not go through the measure analyzer's limit path. **Behaviour note for reviewers.** These paths now return more rows than before — that is the fix, but a very large deployment may see bigger topology / metadata payloads than it used to, where the previous size was only "correct-looking" because it had been cut at 100. Unlike ES's `index.max_result_window`, BanyanDB does not reject an over-large window, so `resultWindowMaxSize` is the operative bound on those reads. **Tests.** `ConditionsTest` gains 8 cases covering `limitIfAbsent` (appends when absent, keeps a caller limit, idempotent, splices ahead of an existing `OFFSET` with parameters re-aligned, composes after `GROUP BY`/`ORDER BY`, and keeps `WITH QUERY_TRACE` ahead of pagination in both layouts). New `QueryLimitTest` pins the invariant at the DAO funnel — that a measure query with no caller limit, with no conditions at all, and with a `GROUP BY` all leave carrying a `LIMIT`, and that an explicit caller limit is not doubled. 41 tests pass in the module. **Storage scope.** ES and JDBC were never affected: `MetricsQueryEsDAO` issues a multi-get over the exact document ids and `JDBCMetricsQueryDAO` uses `id IN (...)` over the assembled points, so neither has a size cap on this path. - [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>. - [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md). -- 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]
