feat(editor): rank numeric columns first inside a numeric aggregate

Reported from the SLO form: inside approx_percentile_cont( on a metrics stream
the dropdown offered twenty string labels and buried `value` -- the one column
the function can take -- below the visible list. The list was correct and
useless at the same time.

The original report was that the fields were "not of the selected stream". They
were: I fetched cache_hit_ratio's schema from the running backend and it matches
the dropdown exactly. What made it look wrong is that every metrics stream in
that dataset has an IDENTICAL schema (verified across four of the 28), so
switching streams changes nothing visible, and metric labels like cost_center
and build_number read as generic infra tags rather than anything about a cache
hit ratio. Driving the live form showed metrics/cache_hit_ratio -> 21 fields and
logs/logs_default -> a different 31, so the wiring was never the problem. The
real complaint was the ranking, and that is what this fixes.

RANKS, does not filter. A declared type is a strong hint, not a rule -- a
quantity stored as Utf8 is still a legal argument -- and hiding a column the
user knows exists is worse than ordering it late. Numeric fields get one extra
sort-lane prefix on a COPY of the entry; the lists handed to the provider are
the composable's live refs, so mutating them would make a contextual ranking
permanent.

Scoped deliberately narrow: the first argument only, of fifteen aggregates that
take a numeric column. approx_percentile_cont(value, 0.95) takes a fraction
second and percentile_cont takes one first, so ranking columns past argument 0
would be noise -- and a per-function argument-type table is exactly the thing
nobody would keep up to date. min/max are included despite accepting strings:
ranking is a hint and the numeric case dominates in a metrics query.

Verified in the running app, both directions: inside approx_percentile_cont(
`value` is now first, and with an empty editor the list returns to plain
alphabetical order with `value` back where it was. The integration test fails
with only the provider wiring stashed (1 failure, and the control test that
proves the ranking is not firing for a trivial alphabetical reason still
passes).

1182 passing across the touched specs; type-check, eslint and prettier clean.
This commit is contained in:
Prabhat Sharma 2026-08-02 16:36:52 -07:00
parent e7f23cc342
commit c83bef5f55
4 changed files with 304 additions and 2 deletions

View File

@ -788,3 +788,84 @@ describe("Phase 3 — C5: one provider per language, not per editor", () => {
},
);
});
// Reported from the SLO form: inside approx_percentile_cont( on a metrics
// stream, twenty string labels sorted above `value` — the one column the
// function can take — and it fell below the visible list.
describe("numeric columns rank first inside a numeric aggregate", () => {
const store6 = createStore({ state: { theme: "light" } });
let spy: ReturnType<typeof vi.spyOn>;
afterAll(() => spy?.mockRestore());
const FIELDS = [
{
name: "availability_zone",
label: "availability_zone",
kind: "Field",
insertText: "availability_zone",
detail: "Utf8",
sortText: "\u0000availability_zone",
},
{
name: "value",
label: "value",
kind: "Field",
insertText: "value",
detail: "Float64",
sortText: "\u0000value",
},
];
/** Mount an editor and ask its provider for the list at `text`. */
const completionsAt = async (editorId: string, text: string) => {
const api = await import("monaco-editor/esm/vs/editor/editor.api");
const createFn = vi.mocked(api.editor.create);
const registerFn = vi.mocked(api.languages.registerCompletionItemProvider);
const before = createFn.mock.calls.length;
spy = vi
.spyOn(document, "getElementById")
.mockImplementation(() => document.createElement("div"));
mount(CodeQueryEditor, {
props: { editorId, language: "sql", query: "", keywords: FIELDS, suggestions: [] },
global: { plugins: [store6] },
});
await vi.waitFor(() => expect(createFn.mock.calls.length).toBe(before + 1), {
timeout: 15000,
interval: 25,
});
const model = createdEditors.at(-1)!.getModel();
const provider = registerFn.mock.calls.filter((c) => c[0] === "sql").at(-1)![1];
model.getValueInRange.mockReturnValue(text);
model.getWordUntilPosition.mockReturnValue({
word: "",
startColumn: text.length + 1,
endColumn: text.length + 1,
});
const result = await provider.provideCompletionItems(
model,
{ lineNumber: 1, column: text.length + 1 },
{},
{},
);
// The order monaco applies to equally-scoring items.
return result.suggestions
.slice()
.sort((a: any, b: any) => (String(a.sortText) < String(b.sortText) ? -1 : 1))
.map((s: any) => s.label);
};
it("puts the numeric column above the string ones", { timeout: 30000 }, async () => {
const labels = await completionsAt("numeric-rank-a", "SELECT approx_percentile_cont(");
expect(labels.indexOf("value")).toBeLessThan(labels.indexOf("availability_zone"));
});
it("leaves the order alone outside such a call", { timeout: 30000 }, async () => {
// Without this the first test passes for a trivial reason -- alphabetical
// order would put `availability_zone` first either way, so a ranking that
// never fires must be observably different here.
const labels = await completionsAt("numeric-rank-b", "SELECT ");
expect(labels.indexOf("availability_zone")).toBeLessThan(labels.indexOf("value"));
});
});

View File

@ -110,6 +110,8 @@ import {
buildHoverContents,
findCatalogEntry,
findFunctionEntry,
wantsNumericColumn,
rankNumericFieldsFirst,
} from "@/utils/query/editorProviders";
import { loadPromqlLanguage } from "@/utils/query/promqlLanguageDefinition";
@ -830,8 +832,18 @@ export default defineComponent({
}
}
const keywordList = config.keywords();
const suggestionList = config.suggestions();
// Inside avg( or approx_percentile_cont(, lift the numeric columns to
// the top. On a metrics stream every label sorts above `value` the
// one column the function can take which is a correct list and a
// useless one. Applied to both lists so it does not depend on which
// one a given host puts its fields in.
const numericFirst = wantsNumericColumn(parseCallContext(textUntilPosition));
const keywordList = numericFirst
? rankNumericFieldsFirst(config.keywords())
: config.keywords();
const suggestionList = numericFirst
? rankNumericFieldsFirst(config.suggestions())
: config.suggestions();
return {
suggestions: buildCompletionItems({
keywords: keywordList,

View File

@ -29,6 +29,9 @@ import {
buildHoverContents,
findCatalogEntry,
findFunctionEntry,
isNumericField,
wantsNumericColumn,
rankNumericFieldsFirst,
} from "./editorProviders";
const fn = (name: string) => SQL_FUNCTIONS.find((f) => f.name === name)!;
@ -428,3 +431,145 @@ describe("parseCallContext ignores SQL comments", () => {
});
});
});
// ───────────────────────────────────────────────────────────────────────────
// Numeric-column ranking
//
// Reported from the SLO form: inside approx_percentile_cont( on a metrics
// stream the dropdown offered twenty string labels and buried `value` — the
// only column the function can take — below the fold. The list was correct and
// useless at the same time.
//
// This RANKS, it does not filter. A declared type is a strong hint, not a rule
// (a quantity stored as Utf8 is still a legal argument), and hiding a column
// the user knows is there is worse than ordering it late.
// ───────────────────────────────────────────────────────────────────────────
// The field lane prefix, written as an ESCAPE: a raw control character in a
// source file is invisible in review and easy to mangle in an edit.
const FIELD_LANE = "\u0000";
describe("isNumericField — what counts as a numeric column", () => {
const field = (detail?: string) => ({ label: "c", kind: "Field", detail }) as any;
it("accepts the arrow types a stream schema actually reports", () => {
for (const t of ["Int64", "Int32", "UInt8", "Float64", "Float32", "Decimal128(10, 2)"]) {
expect(isNumericField(field(t)), t).toBe(true);
}
});
it("rejects the non-numeric ones", () => {
for (const t of ["Utf8", "Boolean", "Binary", "Timestamp(Nanosecond, None)"]) {
expect(isNumericField(field(t)), t).toBe(false);
}
});
it("is case-insensitive, since the type key varies by API", () => {
expect(isNumericField(field("float64"))).toBe(true);
});
it("treats an unknown type as non-numeric rather than guessing", () => {
expect(isNumericField(field(undefined))).toBe(false);
expect(isNumericField(field(""))).toBe(false);
});
it("never promotes a non-Field entry, whatever its detail says", () => {
// A function whose detail mentions a numeric type is still a function and
// must not be ranked in among the columns.
expect(isNumericField({ label: "abs", kind: "Function", detail: "(Int64)" } as any)).toBe(
false,
);
});
});
describe("wantsNumericColumn — when the ranking applies", () => {
it("applies to the first argument of a numeric aggregate", () => {
expect(wantsNumericColumn(parseCallContext("SELECT approx_percentile_cont("))).toBe(true);
expect(wantsNumericColumn(parseCallContext("SELECT avg("))).toBe(true);
expect(wantsNumericColumn(parseCallContext("SELECT sum(x"))).toBe(true);
});
it("is case-insensitive", () => {
expect(wantsNumericColumn(parseCallContext("SELECT AVG("))).toBe(true);
});
it("stops applying past the first argument", () => {
// approx_percentile_cont(value, 0.95) — argument 1 is a fraction, not a
// column, so there is nothing to rank.
expect(wantsNumericColumn(parseCallContext("SELECT approx_percentile_cont(value, "))).toBe(
false,
);
});
it("does not apply to functions that take any type", () => {
expect(wantsNumericColumn(parseCallContext("SELECT count("))).toBe(false);
expect(wantsNumericColumn(parseCallContext("SELECT str_match("))).toBe(false);
});
it("does not apply outside a call", () => {
expect(wantsNumericColumn(null)).toBe(false);
expect(wantsNumericColumn(parseCallContext("SELECT "))).toBe(false);
});
it("does not apply to a WHERE group, which parses as a call with no function", () => {
expect(wantsNumericColumn(parseCallContext("SELECT * FROM t WHERE ("))).toBe(false);
});
});
describe("rankNumericFieldsFirst", () => {
const entries = [
{
label: "availability_zone",
kind: "Field",
detail: "Utf8",
sortText: `${FIELD_LANE}availability_zone`,
},
{ label: "value", kind: "Field", detail: "Float64", sortText: `${FIELD_LANE}value` },
{ label: "duration_ms", kind: "Field", detail: "Int64", sortText: `${FIELD_LANE}duration_ms` },
{ label: "avg", kind: "Function", detail: "(field)", sortText: "avg" },
] as any[];
// localeCompare ignores the control characters the lanes are built from, so
// compare the raw code units — the same order monaco applies.
const order = (list: any[]) =>
[...list]
.sort((a, b) => (String(a.sortText) < String(b.sortText) ? -1 : 1))
.map((e) => e.label);
it("sorts every numeric column above every string one", () => {
expect(order(rankNumericFieldsFirst(entries))).toEqual([
"duration_ms",
"value",
"availability_zone",
"avg",
]);
});
it("leaves the functions below the columns", () => {
const ranked = order(rankNumericFieldsFirst(entries));
expect(ranked.indexOf("avg")).toBeGreaterThan(ranked.indexOf("availability_zone"));
});
it("does not drop, add or rewrite any entry", () => {
const ranked = rankNumericFieldsFirst(entries);
expect(ranked).toHaveLength(entries.length);
expect(ranked.map((e: any) => e.label).sort()).toEqual(entries.map((e) => e.label).sort());
expect(ranked.find((e: any) => e.label === "value")!.detail).toBe("Float64");
});
it("does not mutate the caller's entries", () => {
// These are the composable's live refs. Mutating them would make the
// ranking permanent instead of contextual.
const before = entries.map((e) => e.sortText);
rankNumericFieldsFirst(entries);
expect(entries.map((e) => e.sortText)).toEqual(before);
});
it("orders a field with no sortText by its own name", () => {
const ranked = rankNumericFieldsFirst([
{ label: "b_num", kind: "Field", detail: "Int64" },
{ label: "a_num", kind: "Field", detail: "Int64" },
] as any[]);
expect(order(ranked)).toEqual(["a_num", "b_num"]);
});
});

View File

@ -203,6 +203,70 @@ export const buildHoverContents = (entry: LooseEntry | null): { value: string }[
return contents;
};
/**
* Aggregates whose FIRST argument is a numeric column.
*
* Only the first argument: approx_percentile_cont(value, 0.95) takes a fraction
* second, and percentile_cont takes one first ranking columns there would be
* noise. Restricting to argument 0 covers the reported case and every common
* one without a per-function argument table nobody would keep up to date.
*
* min/max are here even though they accept strings: ranking is a hint, and the
* numeric case is overwhelmingly the intent in a metrics query.
*/
const NUMERIC_COLUMN_FUNCTIONS = new Set([
"avg",
"sum",
"min",
"max",
"median",
"approx_median",
"approx_percentile_cont",
"approx_percentile_cont_with_weight",
"stddev",
"stddev_pop",
"stddev_samp",
"var",
"var_pop",
"var_samp",
"variance",
]);
/** Arrow types that name a number. Decimal carries a precision suffix. */
const NUMERIC_TYPE = /^(u?int(8|16|32|64)|float(16|32|64)|decimal)/i;
/**
* Is this entry a column holding numbers?
*
* Kind is checked first: a FUNCTION whose detail happens to mention a numeric
* type is still a function and must not be ranked in among the columns.
*/
export const isNumericField = (entry: LooseEntry): boolean =>
entry.kind === "Field" && NUMERIC_TYPE.test(entry.detail ?? "");
/** Does the cursor sit where a numeric column is wanted? */
export const wantsNumericColumn = (call: CallContext | null): boolean =>
!!call && call.activeParameter === 0 && NUMERIC_COLUMN_FUNCTIONS.has(call.name.toLowerCase());
/**
* Lift the numeric columns to the top of the list.
*
* RANKS, does not filter. A declared type is a strong hint, not a rule a
* quantity stored as Utf8 is still a legal argument and hiding a column the
* user knows exists is worse than ordering it late.
*
* Prefixing the existing sortText rather than rebuilding it keeps the relative
* order inside each group and works whatever lane scheme the host used. Copies
* are returned because the lists handed in are the composable's live refs:
* mutating them would make a contextual ranking permanent.
*/
export const rankNumericFieldsFirst = (entries: LooseEntry[]): LooseEntry[] =>
entries.map((entry) =>
isNumericField(entry)
? { ...entry, sortText: `\u0000${entry.sortText ?? entryName(entry)}` }
: entry,
);
/**
* Detect that the cursor sits where a field VALUE belongs.
*