diff --git a/web/src/components/CodeQueryEditor.completion.spec.ts b/web/src/components/CodeQueryEditor.completion.spec.ts index c5bb1d2f24..55a0ffaabc 100644 --- a/web/src/components/CodeQueryEditor.completion.spec.ts +++ b/web/src/components/CodeQueryEditor.completion.spec.ts @@ -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; + 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")); + }); +}); diff --git a/web/src/components/CodeQueryEditor.vue b/web/src/components/CodeQueryEditor.vue index 0bc6867a4f..5164c63a4e 100644 --- a/web/src/components/CodeQueryEditor.vue +++ b/web/src/components/CodeQueryEditor.vue @@ -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, diff --git a/web/src/utils/query/editorProviders.spec.ts b/web/src/utils/query/editorProviders.spec.ts index 2a2e903da0..81bd1e1050 100644 --- a/web/src/utils/query/editorProviders.spec.ts +++ b/web/src/utils/query/editorProviders.spec.ts @@ -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"]); + }); +}); diff --git a/web/src/utils/query/editorProviders.ts b/web/src/utils/query/editorProviders.ts index 66dfc61661..856d1abccf 100644 --- a/web/src/utils/query/editorProviders.ts +++ b/web/src/utils/query/editorProviders.ts @@ -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. *