Skip to content

Always send an explicit LIMIT on BanyanDB queries - #13961

Merged
wu-sheng merged 1 commit into
masterfrom
fix/banyandb-implicit-query-limit-truncation
Jul 28, 2026
Merged

Always send an explicit LIMIT on BanyanDB queries#13961
wu-sheng merged 1 commit into
masterfrom
fix/banyandb-implicit-query-limit-truncation

Conversation

@wu-sheng

Copy link
Copy Markdown
Member

Fix silent truncation of BanyanDB query results at the engine's implicit row cap — #13960

  • Add a unit test to verify that the fix works.
  • 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.

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

BanyanDB applies its own default limit to any query that carries none --
100 rows for measures, 20 for streams/traces -- and applies it after
GROUP BY, so an over-long result set is silently truncated rather than
rejected.

OAP never sent a limit on several read paths, so an entity-scoped metrics
query returned at most 100 data points regardless of the requested range:
a 4-hour minute-step read rendered only its first 100 minutes and the rest
showed as empty, even though DurationUtils.MAX_TIME_RANGE allows up to 500
steps. The same cap silently shortened topology relation maps, instance
and process metadata lists, profiling thread snapshots and eBPF task lists.

Every BydbQL query now leaves OAP with an explicit LIMIT:

- the entity-scoped metrics read sends the exact number of assembled
  duration points, the same row set the ES/JDBC DAOs fetch by explicit id;
- ad-hoc SELECT TOP sends its own N;
- anything that does not paginate itself falls back to the configured
  resultWindowMaxSize (default 10000) via Conditions#limitIfAbsent, applied
  in the stream/measure/trace query helpers that every DAO funnels through.

The fallback is spliced in at the start of the pagination tail rather than
appended, so it lands ahead of an OFFSET that was set first and keeps
WITH QUERY_TRACE positioned as the grammar requires.

ES and JDBC storage were never affected -- both fetch metrics rows by
explicit document id.
@wu-sheng wu-sheng added bug Something isn't working and you are sure it's a bug! backend OAP backend related. labels Jul 28, 2026
@wu-sheng wu-sheng added this to the 11.0.0 milestone Jul 28, 2026
@wu-sheng
wu-sheng merged commit be0383a into master Jul 28, 2026
443 of 447 checks passed
@wu-sheng
wu-sheng deleted the fix/banyandb-implicit-query-limit-truncation branch July 28, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend OAP backend related. bug Something isn't working and you are sure it's a bug!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants