opensearch-project / opensearch-project/sql
[RFC] Integration tests should cover the command x data-type matrix
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 176
- Forks
- 229
- Avg merge
- 2d 21h
- Merged PRs (30d)
- 43
Description
Problem Statement
Our integration tests verify that each command works on the data types its author happened to use — almost always a keyword, a number, or a date. Nothing verifies what a command does when it meets an object, a nested/multi-value field, an ip, a binary, a geo_point, an unmapped field, or a field whose type differs across the indices of a wildcard.
The result is a recurring class of production bug: a command that has been green in CI for months returns a 500 the first time a user points it at an object field. These are not deep engine bugs — each is one missing type check — and every one of them is a coverage gap rather than a logic gap.
Recent examples, all found from user reports rather than CI:
| Issue | Command | Type it was never tested against |
|---|---|---|
| #5750 | timechart / chart split, cast |
object, multi-value |
| #5685 | dedup |
object vs scalar across a wildcard |
| #5618 | value decoding | geo_point, scalar-under-object |
| #5752 | mapping merge | object vs scalar across a wildcard |
Current State
- Each command gets its own IT class with its own fixture index, sized to that command's happy path.
CalciteTimechartCommandITuseshost(keyword) andcpu_usage(double);CalciteChartCommandITsimilar. Neither index has anobject,nested,ip,binary, orgeo_pointfield, so no chart test could ever have caught #5750. - The few type-focused fixtures we do have (
nested,geopoint,datatypes) are consumed by the tests written specifically for them, not by the command suites. - Nothing enumerates the command × type space, so coverage is whatever the union of individual authors happened to pick, and no one can tell which cells are untested versus deliberately unsupported.
- There is no assertion anywhere that says "unsupported input must be a 4xx, never a 5xx". A command that 500s on an object field looks identical to CI as one that returns a clean error.
Measured example of what the space actually looks like — one object field, one index, current main:
| Query | Result |
|---|---|
stats count() by obj |
200, but a single bucket with obj = null — silently wrong, no error |
top 2 obj / rare 2 obj |
200, single null bucket |
eventstats count() by obj |
200, rows retain the JSON object |
fields obj |
200, object rendered as JSON |
dedup obj |
200 |
sort obj |
500 — shard-level No mapping found for [obj] in order to sort on |
timechart span=1m count() by obj |
500 → 400 after #5751 |
chart count() over m by obj |
500 → 400 after #5751 |
eval x = cast(obj as string) |
500 → 400 after #5751 |
eval x = concat(obj, 'x') |
500 — plan dump (#5753) |
eval x = upper(obj) |
400 — correct, function signature check |
xyseries m obj c |
400 — correct, explicit guard |
Twelve queries, one type, five distinct behaviours — three of them wrong. None of this was visible before someone went looking by hand.
Long-Term Goals
- Any command that cannot support a data type says so with a 4xx and a message naming the field, deterministically, in every engine configuration. A 5xx from a type mismatch is a bug by definition.
- The command × type matrix is explicit and version-controlled, so an unsupported cell is a recorded decision rather than an unknown.
- Adding a command or a supported type forces the matrix to be extended, so coverage cannot silently rot as the language grows.
- Silent-wrong-answer cells (like
stats ... by <object>returning anullbucket) are visible and tracked, not hidden behind a 200.
Proposal
A type conformance matrix in integ-test, driven by two lists and one expectation file.
-
A canonical "type zoo" fixture. One index with one field per supported mapping type, plus the awkward cases:
keyword,text,textwith a.keywordsub-field,match_only_text,long/integer/short/byte,double/float/half_float/scaled_float,boolean,date,date_nanos,ip,binary,geo_point,object(with sub-fields),nested, array-of-scalar,alias,flat_object, a dynamically-mapped field, and a field absent from the mapping.Plus a small set of two-index fixtures for cross-index conflicts:
text/keyword,object/scalar,integer/long, and field-present/field-absent. -
A command template list, one entry per command, parameterized on a field name — e.g.
source=$IDX | timechart span=1m count() by $FIELD,source=$IDX | sort $FIELD,source=$IDX | stats count() by $FIELD. -
A generated matrix run that executes every template against every field and classifies the outcome, asserting against a checked-in expectation file:
OK— succeeded, with the row count asserted.UNSUPPORTED— 4xx and the message matches a recorded pattern. Legitimate, and the message is pinned so it cannot silently regress into a 500.KNOWN_GAP— recorded with a linked issue number. Fails the build if the cell starts passing (so fixes get promoted) or changes shape.- Anything else, in particular any 5xx, fails the build.
The expectation file is the deliverable that does not exist today: a single reviewable artifact showing what the engine does with every type, per command.
Approach
Phased so each phase is independently useful and reviewable:
- Fixture + harness. Type-zoo index, template list, and a runner covering ~10 commands. Snapshot current behaviour into the expectation file, marking each non-OK cell
UNSUPPORTEDorKNOWN_GAP. No behaviour changes — this phase only makes the truth visible. - Triage the snapshot. Every 5xx and every silent-wrong-answer cell becomes an issue. The chart/cast 500s (#5750) would have been found here; so would
sort <object>andstats ... by <object>returning anullbucket. - CI gate. Run the matrix in CI; a new 5xx cell or a changed error message fails the build.
- Extend to all commands, then to functions grouped by signature family, and to the cross-index conflict fixtures.
- Optional per-route runs. The matrix is a natural fit for running against each engine configuration (Calcite with pushdown, without pushdown, other backends), which is where type handling diverges most.
Alternative
- Keep adding type cases to individual command ITs as bugs are reported. This is the status quo. It is reactive by construction: coverage only ever grows where a user already got a 500, and nothing prevents the next command from repeating the same omission.
- A checklist item in the new-command doc ("test your command against an object field"). Cheap, but unenforced and easy to skip — the existing new-command checklist has not prevented any of the issues above.
- Property-based/fuzz testing over generated queries. Broader in principle, but the oracle problem is hard: for a fuzzed query we usually cannot say what the right answer is. The matrix sidesteps this by only asserting the class of outcome, which is exactly the property we care about (never 5xx, stable error messages).
Implementation Discussion
- Where it lives.
yamlRestTestis declarative and readable but awkward to generate a matrix in; a parameterized Java IT gives us the loop and the expectation file. Leaning Java IT with a resource-file expectation snapshot. - Runtime cost. ~25 types × ~40 commands is ~1000 queries. On a shared fixture with no re-indexing per case this should be a single-digit-minutes suite, but it needs measuring before it goes into the default CI path — it may belong in a nightly job with a smaller smoke subset per PR.
- How to record a known gap. We already have a per-route skip mechanism (capability annotations) for tests that cannot pass on a given backend; the matrix needs something similar but data-driven, since the unit of exemption is a cell rather than a test method.
- Error-message pinning granularity. Pinning full messages makes the suite noisy on wording changes (we have just reworded two in #5751). Probably pin an error code plus a loose message pattern, which is also an argument for populating
ErrorCodeon these client errors — several currently reportUNKNOWN. - What counts as a silent wrong answer.
stats ... by <object>returning anullbucket is arguably worse than an error, but deciding that anullis wrong requires knowing the intended semantics per command. Suggest starting with a narrow rule — a group key that is non-null in_sourcemust not aggregate tonull— and expanding from there. - Interaction with #5610. The cross-index conflict fixtures overlap with the schema-conflict policy discussion; the matrix can encode whatever policy that lands on rather than pre-judging it.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading the existing integ-test command suites, especially CalciteTimechartCommandIT and CalciteChartCommandIT, and compare them with the yamlRestTest approach. Define the shared type-zoo fixture, command templates, Java parameterized runner, and resource-file expectation snapshot described in the proposal. Done means the initial command subset records OK, UNSUPPORTED, and KNOWN_GAP outcomes and fails on unexpected results such as 5xx responses.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- testing-qa
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100