This is an automated email from the ASF dual-hosted git repository.
wu-sheng pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/skywalking.git
The following commit(s) were added to refs/heads/master by this push:
new 4fae3c6a65 Some enhancement check for bydbQL (#13949)
4fae3c6a65 is described below
commit 4fae3c6a65ce51d0c66d983244fc3b28de7ba311
Author: mrproliu <[email protected]>
AuthorDate: Wed Jul 15 20:49:50 2026 +0800
Some enhancement check for bydbQL (#13949)
---
.../banyandb/BanyanDBAggregationQueryDAO.java | 5 +-
.../banyandb/stream/AbstractBanyanDBDAO.java | 25 +--------
.../storage/plugin/banyandb/stream/Conditions.java | 64 +++++++++++++++++-----
.../plugin/banyandb/stream/ConditionsTest.java | 64 ++++++++++++++++++++--
4 files changed, 117 insertions(+), 41 deletions(-)
diff --git
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBAggregationQueryDAO.java
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBAggregationQueryDAO.java
index 0f24223a44..5b9d8c9afe 100644
---
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBAggregationQueryDAO.java
+++
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBAggregationQueryDAO.java
@@ -147,8 +147,9 @@ public class BanyanDBAggregationQueryDAO extends
AbstractBanyanDBDAO implements
}
// Ad-hoc TopN over the raw measure: MEAN aggregation + TOP N + GROUP
BY entity_id, expressed as a
// BydbQL `SELECT TOP ? "value" <dir>, MEAN("value"), entity_id ...
GROUP BY entity_id, "value"`. The
- // trailing GROUP BY field is inert server-side (grouping uses the tag
projection, aggregation the Agg
- // field), so this matches the former typed meanBy(value,{entity_id})
+ topN/bottomN(N,value) path.
+ // trailing GROUP BY field is validated and carried on the request but
does not change the grouping
+ // result (grouping keys come from the tag projection = entity_id; the
field feeds the MEAN aggregate),
+ // so this matches the former typed meanBy(value,{entity_id}) +
topN/bottomN(N,value) path.
final Conditions where = Conditions.create();
if (CollectionUtils.isNotEmpty(additionalConditions)) {
additionalConditions.forEach(c -> where.eq(qualify(c.getKey()),
c.getValue()));
diff --git
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/AbstractBanyanDBDAO.java
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/AbstractBanyanDBDAO.java
index 245cb6bcdb..cffa0eb5ab 100644
---
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/AbstractBanyanDBDAO.java
+++
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/AbstractBanyanDBDAO.java
@@ -97,7 +97,7 @@ public abstract class AbstractBanyanDBDAO extends
AbstractDAO<BanyanDBStorageCli
ql.append(" ON
").append(BanyanDBStorageConfig.StageName.cold.name()).append(" STAGES");
}
ql.append(" TIME BETWEEN ? AND ?");
- appendBodyWithTrace(ql, where.buildQl(), debug);
+ ql.append(where.buildQl(debug));
final List<Serializable<BanyandbModel.TagValue>> params =
timeBoundedParams(timestampRange, where.params());
final StreamQueryResponse response =
getClient().queryStream(ql.toString(), params.toArray(new
Serializable[0]));
@@ -306,7 +306,7 @@ public abstract class AbstractBanyanDBDAO extends
AbstractDAO<BanyanDBStorageCli
ql.append(" ON
").append(BanyanDBStorageConfig.StageName.cold.name()).append(" STAGES");
}
ql.append(" TIME BETWEEN ? AND ?");
- appendBodyWithTrace(ql, where.buildQl(), debug);
+ ql.append(where.buildQl(debug));
final List<Serializable<BanyandbModel.TagValue>> params =
timeBoundedParams(timestampRange, where.params());
final MeasureQueryResponse response =
getClient().queryMeasure(ql.toString(), params.toArray(new
Serializable[0]));
@@ -361,25 +361,6 @@ public abstract class AbstractBanyanDBDAO extends
AbstractDAO<BanyanDBStorageCli
return params;
}
- /**
- * Append the clause tail, inserting {@code WITH QUERY_TRACE} (when
debugging) before any
- * {@code LIMIT}/{@code OFFSET} — BydbQL requires the trace marker ahead
of LIMIT/OFFSET or the parser
- * rejects it with {@code unexpected token "WITH"}.
- */
- private static void appendBodyWithTrace(StringBuilder ql, String body,
boolean debug) {
- final String tail = body == null ? "" : body;
- if (!debug) {
- ql.append(tail);
- return;
- }
- final int limitIdx = tail.indexOf(" LIMIT ");
- if (limitIdx >= 0) {
- ql.append(tail, 0, limitIdx).append(" WITH
QUERY_TRACE").append(tail, limitIdx, tail.length());
- } else {
- ql.append(tail).append(" WITH QUERY_TRACE");
- }
- }
-
/**
* Double-quote a column identifier for BydbQL. Quoting unconditionally
sidesteps the reserved-keyword
* check (e.g. a column named {@code count}) without tracking BanyanDB's
keyword list.
@@ -437,7 +418,7 @@ public abstract class AbstractBanyanDBDAO extends
AbstractDAO<BanyanDBStorageCli
ql.append(" ON
").append(BanyanDBStorageConfig.StageName.cold.name()).append(" STAGES");
}
ql.append(" TIME BETWEEN ? AND ?");
- appendBodyWithTrace(ql, where.buildQl(), debug);
+ ql.append(where.buildQl(debug));
final List<Serializable<BanyandbModel.TagValue>> params =
timeBoundedParams(timestampRange, where.params());
final TraceQueryResponse response =
getClient().queryTrace(ql.toString(), params.toArray(new
Serializable[0]));
diff --git
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/Conditions.java
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/Conditions.java
index bee77a96df..fe7a8beaa1 100644
---
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/Conditions.java
+++
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/Conditions.java
@@ -36,6 +36,12 @@ public final class Conditions {
private final StringBuilder ql = new StringBuilder();
private final List<Serializable<BanyandbModel.TagValue>> params = new
ArrayList<>();
private final boolean groupMode;
+ /**
+ * Offset in {@link #ql} where the {@code LIMIT}/{@code OFFSET} tail
begins, or {@code -1} if there is none.
+ * Recorded when the first of {@code limit()}/{@code offset()} is appended
so {@link #buildQl(boolean)} can
+ * position {@code WITH QUERY_TRACE} ahead of pagination without scanning
the generated text.
+ */
+ private int paginationStart = -1;
private Conditions(boolean groupMode) {
this.groupMode = groupMode;
@@ -84,6 +90,9 @@ public final class Conditions {
}
public Conditions in(String column, List<String> values) {
+ if (values == null || values.isEmpty()) {
+ throw new IllegalArgumentException("IN values must not be empty
for column: " + column);
+ }
return condition(column, " IN (?)", Value.stringArrayTagValue(values));
}
@@ -106,6 +115,9 @@ public final class Conditions {
}
public Conditions having(String column, List<String> values) {
+ if (values == null || values.isEmpty()) {
+ throw new IllegalArgumentException("HAVING values must not be
empty for column: " + column);
+ }
return condition(column, " HAVING (?)",
Value.stringArrayTagValue(values));
}
@@ -149,12 +161,18 @@ public final class Conditions {
}
public Conditions limit(long value) {
+ if (paginationStart < 0) {
+ paginationStart = ql.length();
+ }
ql.append(" LIMIT ?");
params.add(Value.longTagValue(value));
return this;
}
public Conditions offset(long value) {
+ if (paginationStart < 0) {
+ paginationStart = ql.length();
+ }
ql.append(" OFFSET ?");
params.add(Value.longTagValue(value));
return this;
@@ -168,6 +186,23 @@ public final class Conditions {
return ql.toString();
}
+ /**
+ * The QL body with the {@code WITH QUERY_TRACE} debug marker optionally
injected at the grammar-mandated
+ * position — after {@code ORDER BY}, before any {@code LIMIT}/{@code
OFFSET}. The position comes from the
+ * recorded {@link #paginationStart}, so no scanning of the generated QL
text is required (robust even if a
+ * column name or projection contained {@code " LIMIT "}, and correct for
an {@code OFFSET}-only body).
+ *
+ * @param withQueryTrace whether to inject {@code WITH QUERY_TRACE}
+ * @return the QL body, with the trace marker inserted ahead of pagination
when requested
+ */
+ public String buildQl(boolean withQueryTrace) {
+ if (!withQueryTrace) {
+ return ql.toString();
+ }
+ final int pos = paginationStart < 0 ? ql.length() : paginationStart;
+ return ql.substring(0, pos) + " WITH QUERY_TRACE" + ql.substring(pos);
+ }
+
/**
* @return the bound parameters, in the order their {@code ?} placeholders
appear in {@link #buildQl()}.
*/
@@ -176,9 +211,11 @@ public final class Conditions {
}
/**
- * Combine condition {@link #group() groups} with OR: {@code WHERE (g1) OR
(g2) OR ...}. A single non-empty
- * group is emitted without parentheses ({@code WHERE g1}); empty groups
are skipped; if every group is empty
- * no clause is emitted. Params are appended in group order.
+ * Combine condition {@link #group() groups} with OR, wrapping the whole
disjunction in an outer paren so it
+ * composes correctly with any surrounding predicate: {@code WHERE ((g1)
OR (g2) OR ...)}, or {@code WHERE (g1)}
+ * for a single group. The outer paren is what makes {@code
.eq(x).or(groups)} bind as
+ * {@code x AND ((g1) OR (g2))} rather than the precedence-wrong {@code x
AND g1 OR g2}. Empty groups are
+ * skipped; if every group is empty no clause is emitted. Params are
appended in group order.
*
* @param groups the condition groups, each built via {@link #group()}
* @return this builder
@@ -193,19 +230,20 @@ public final class Conditions {
if (nonEmpty.isEmpty()) {
return this;
}
- ql.append(ql.length() == 0 ? " WHERE " : " AND ");
- if (nonEmpty.size() == 1) {
- ql.append(nonEmpty.get(0).ql);
- params.addAll(nonEmpty.get(0).params);
- } else {
- for (int i = 0; i < nonEmpty.size(); i++) {
- if (i > 0) {
- ql.append(" OR ");
- }
+ final boolean multi = nonEmpty.size() > 1;
+ ql.append(ql.length() == 0 ? " WHERE " : " AND ").append("(");
+ for (int i = 0; i < nonEmpty.size(); i++) {
+ if (i > 0) {
+ ql.append(" OR ");
+ }
+ if (multi) {
ql.append("(").append(nonEmpty.get(i).ql).append(")");
- params.addAll(nonEmpty.get(i).params);
+ } else {
+ ql.append(nonEmpty.get(i).ql);
}
+ params.addAll(nonEmpty.get(i).params);
}
+ ql.append(")");
return this;
}
diff --git
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/ConditionsTest.java
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/ConditionsTest.java
index 25f71ab1ff..f8ec24a82c 100644
---
a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/ConditionsTest.java
+++
b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/ConditionsTest.java
@@ -24,6 +24,7 @@ import
org.apache.skywalking.library.banyandb.v1.client.metadata.Serializable;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
public class ConditionsTest {
@@ -129,10 +130,10 @@ public class ConditionsTest {
}
@Test
- public void orWithSingleNonEmptyGroupHasNoParentheses() {
+ public void orWithSingleGroupIsParenthesized() {
final Conditions where = Conditions.create().or(List.of(
Conditions.group().in("a", List.of("x"))));
- assertEquals(" WHERE a IN (?)", where.buildQl());
+ assertEquals(" WHERE (a IN (?))", where.buildQl());
assertEquals(1, where.params().size());
}
@@ -141,7 +142,7 @@ public class ConditionsTest {
final Conditions where = Conditions.create().or(List.of(
Conditions.group().eq("a", "x"),
Conditions.group().eq("b", "y")));
- assertEquals(" WHERE (a = ?) OR (b = ?)", where.buildQl());
+ assertEquals(" WHERE ((a = ?) OR (b = ?))", where.buildQl());
final List<Serializable<BanyandbModel.TagValue>> params =
where.params();
assertEquals(2, params.size());
assertEquals("x", str(params.get(0)));
@@ -154,7 +155,62 @@ public class ConditionsTest {
Conditions.group(),
Conditions.group().eq("a", "x"),
Conditions.group()));
- assertEquals(" WHERE a = ?", where.buildQl());
+ assertEquals(" WHERE (a = ?)", where.buildQl());
assertEquals(1, where.params().size());
}
+
+ @Test
+ public void orAfterExistingPredicateIsParenthesizedForCorrectPrecedence() {
+ final Conditions where = Conditions.create()
+ .eq("tenant", "t")
+ .or(List.of(
+ Conditions.group().eq("a", "x"),
+ Conditions.group().eq("b", "y")));
+ // The outer parens are what keep AND from binding tighter than OR:
+ // tenant = ? AND ((a = ?) OR (b = ?)), not (tenant = ? AND a = ?) OR
b = ?.
+ assertEquals(" WHERE tenant = ? AND ((a = ?) OR (b = ?))",
where.buildQl());
+ assertEquals(3, where.params().size());
+ }
+
+ @Test
+ public void buildQlWithTraceInsertsBeforeLimitAndOffset() {
+ final Conditions where = Conditions.create().eq("a",
"x").orderByDesc("t").limit(10).offset(20);
+ assertEquals(" WHERE a = ? ORDER BY t DESC WITH QUERY_TRACE LIMIT ?
OFFSET ?", where.buildQl(true));
+ }
+
+ @Test
+ public void buildQlWithTraceInsertsBeforeOffsetOnly() {
+ final Conditions where = Conditions.create().eq("a", "x").offset(20);
+ assertEquals(" WHERE a = ? WITH QUERY_TRACE OFFSET ?",
where.buildQl(true));
+ }
+
+ @Test
+ public void buildQlWithTraceAppendsAtEndWhenNoPagination() {
+ final Conditions where = Conditions.create().eq("a",
"x").orderByDesc("t");
+ assertEquals(" WHERE a = ? ORDER BY t DESC WITH QUERY_TRACE",
where.buildQl(true));
+ }
+
+ @Test
+ public void buildQlWithTraceFalseEqualsPlainBuildQl() {
+ final Conditions where = Conditions.create().eq("a", "x").limit(5);
+ assertEquals(where.buildQl(), where.buildQl(false));
+ }
+
+ @Test
+ public void emptyInValuesAreRejectedLocally() {
+ assertThrows(IllegalArgumentException.class, () ->
Conditions.create().in("a", List.of()));
+ }
+
+ @Test
+ public void emptyHavingValuesAreRejectedLocally() {
+ assertThrows(IllegalArgumentException.class, () ->
Conditions.create().having("a", List.of()));
+ }
+
+ @Test
+ public void valuesAreBoundAsParamsNotInterpolatedIntoQl() {
+ // An injection-looking value stays a bound parameter (data); it never
becomes part of the QL text.
+ final Conditions where = Conditions.create().eq("a", "' OR 1=1 --");
+ assertEquals(" WHERE a = ?", where.buildQl());
+ assertEquals("' OR 1=1 --", str(where.params().get(0)));
+ }
}
