From 9a35e76fea098b08788d1130f20e7e4e7672d107 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Sun, 19 Jul 2026 16:54:27 -0700 Subject: [PATCH 01/10] feat(ci): add PPL lint rule validation check (eventstats PoC) Add a cross-repository CI check that keeps the OpenSearch-Dashboards PPL lint rule 'unsupported-window-function-in-eventstats' and the SQL backend in agreement. Frontend half: a SQL-owned Node script loads the compiled OSD analyzer from an OSD checkout and asserts the rule's diagnostic counts. Backend half: a Gradle integration test sends the same queries to the live /_plugins/_ppl endpoint of the SQL plugin built from the checkout. Both halves consume one shared contract file. - integ-test/.../ppl-lint/unsupported-window-function-in-eventstats.spec.json - scripts/ppl-lint/run-frontend-contract.mjs - integ-test/.../calcite/remote/PplLintRuleValidationIT.java - .github/workflows/ppl-lint-rule-validation.yml - scripts/ppl-lint-rule-validation.sh Signed-off-by: Hanyu Wei --- .../workflows/ppl-lint-rule-validation.yml | 148 ++++++++++++++++ .../remote/PplLintRuleValidationIT.java | 130 ++++++++++++++ ...ed-window-function-in-eventstats.spec.json | 29 ++++ scripts/ppl-lint-rule-validation.sh | 105 +++++++++++ scripts/ppl-lint/run-frontend-contract.mjs | 164 ++++++++++++++++++ 5 files changed, 576 insertions(+) create mode 100644 .github/workflows/ppl-lint-rule-validation.yml create mode 100644 integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java create mode 100644 integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json create mode 100755 scripts/ppl-lint-rule-validation.sh create mode 100644 scripts/ppl-lint/run-frontend-contract.mjs diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml new file mode 100644 index 00000000000..61ee0be6fb8 --- /dev/null +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -0,0 +1,148 @@ +name: PPL lint rule validation + +# Cross-repository check: the OpenSearch-Dashboards (OSD) PPL lint rule +# `unsupported-window-function-in-eventstats` and the SQL backend must agree. +# +# Frontend half: a SQL-owned Node script loads the compiled OSD analyzer from an +# OSD checkout and asserts the rule's diagnostic counts. +# Backend half: a Gradle integration test sends the same queries to the live +# `/_plugins/_ppl` endpoint of the SQL plugin built from this checkout. +# +# The OSD detector is loaded from `main` by default (PR and nightly runs), so a +# removed or changed detector is detected. `workflow_dispatch` can target a +# specific OSD ref to reproduce a run or test an unmerged OSD branch. + +on: + pull_request: + schedule: + - cron: '0 10 * * *' + workflow_dispatch: + inputs: + osd_ref: + description: OSD commit or branch to test instead of main + required: false + type: string + +jobs: + Get-CI-Image-Tag: + uses: opensearch-project/opensearch-build/.github/workflows/get-ci-image-tag.yml@761e093b8c1349cc07f21c1d681d3b30bf9e1999 # main + with: + product: opensearch + + ppl-lint-rule-validation: + name: PPL lint rule validation + needs: Get-CI-Image-Tag + runs-on: ubuntu-latest + container: + image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} + options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} + + steps: + - name: Run start commands + run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }} + + - name: Checkout SQL pull request + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - name: Resolve OSD ref + id: osd-ref + env: + REQUESTED_REF: ${{ inputs.osd_ref }} + run: echo "ref=${REQUESTED_REF:-main}" >> "$GITHUB_OUTPUT" + + - name: Checkout OpenSearch-Dashboards + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + repository: opensearch-project/OpenSearch-Dashboards + ref: ${{ steps.osd-ref.outputs.ref }} + path: .ci/OpenSearch-Dashboards + + - name: Record OSD revision + id: osd-sha + run: | + sha=$(git -C .ci/OpenSearch-Dashboards rev-parse HEAD) + echo "sha=$sha" >> "$GITHUB_OUTPUT" + echo "OSD revision: \`$sha\` (ref: ${{ steps.osd-ref.outputs.ref }})" >> "$GITHUB_STEP_SUMMARY" + + - name: Set up JDK 21 + uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4 + with: + distribution: 'temurin' + java-version: 21 + + # Read the Node/Yarn toolchain from the OSD checkout rather than hardcoding + # it, so an OSD toolchain bump does not silently drift this job. + - name: Read OSD Node version + id: osd-node + run: echo "version=$(cat .ci/OpenSearch-Dashboards/.nvmrc)" >> "$GITHUB_OUTPUT" + + - name: Set up Node ${{ steps.osd-node.outputs.version }} + uses: actions/setup-node@1e60f620b9541d16bece96c5465dc8ee9832be0b # v4 + with: + node-version: ${{ steps.osd-node.outputs.version }} + + - name: Read OSD Yarn version + id: osd-yarn + working-directory: .ci/OpenSearch-Dashboards + run: | + yarn_range=$(node -e "process.stdout.write(require('./package.json').engines.yarn)") + # Take the lower bound of the engines.yarn range (e.g. "^1.22.10" -> "1.22.10"). + yarn_version=$(echo "$yarn_range" | sed -E 's/[^0-9.]//g') + echo "version=$yarn_version" >> "$GITHUB_OUTPUT" + + - name: Pin Yarn ${{ steps.osd-yarn.outputs.version }} + run: npm install -g "yarn@${{ steps.osd-yarn.outputs.version }}" + + - name: Cache OSD Yarn dependencies + uses: actions/cache@0c907a75c2c80ebcb7f088228285e798b750cf8f # v4 + with: + path: | + ~/.cache/yarn + key: ${{ runner.os }}-osd-yarn-node${{ steps.osd-node.outputs.version }}-${{ hashFiles('.ci/OpenSearch-Dashboards/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-osd-yarn-node${{ steps.osd-node.outputs.version }}- + + - name: Bootstrap OpenSearch-Dashboards + working-directory: .ci/OpenSearch-Dashboards + run: yarn osd bootstrap + + # The Gradle test cluster runs the version from build.gradle's + # `opensearch.version` default (e.g. 3.8.0-SNAPSHOT). Export the release + # portion (3.8.0) as PPL_SQL_VERSION so the frontend applies the same + # version filtering the backend does, without maintaining a second string. + - name: Resolve OpenSearch version + id: os-version + run: | + raw=$(grep -oE '"opensearch.version", "[^"]+"' build.gradle | head -1 | sed -E 's/.*"opensearch.version", "([^"]+)"/\1/') + version="${raw%%-*}" + echo "version=$version" >> "$GITHUB_OUTPUT" + echo "OpenSearch version: \`$version\` (from \`$raw\`)" >> "$GITHUB_STEP_SUMMARY" + + # The frontend contract only runs Node (no OpenSearch cluster), so it runs + # as the default container user. Only the Gradle integTest below must run + # as a non-root user, because OpenSearch refuses to start as root. + - name: Run frontend contract (OSD analyzer) + working-directory: .ci/OpenSearch-Dashboards + env: + PPL_LINT_CONTRACT_FILE: ${{ github.workspace }}/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json + PPL_SQL_VERSION: ${{ steps.os-version.outputs.version }} + run: | + node -r ./src/setup_node_env \ + "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" \ + | tee "$GITHUB_WORKSPACE/frontend-contract.log" + + - name: Run backend integration test (live /_plugins/_ppl) + run: | + chown -R 1000:1000 `pwd` + su `id -un 1000` -c "./gradlew :integ-test:integTest --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" + + - name: Upload failure artifacts + if: ${{ failure() }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + continue-on-error: true + with: + name: ppl-lint-rule-validation-artifacts + path: | + frontend-contract.log + integ-test/build/reports/** + integ-test/build/testclusters/*/logs/* diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java new file mode 100644 index 00000000000..ac92f979595 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java @@ -0,0 +1,130 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.calcite.remote; + +import static org.opensearch.sql.legacy.TestUtils.getResponseBody; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +import org.json.JSONArray; +import org.json.JSONObject; +import org.junit.jupiter.api.Test; +import org.opensearch.client.ResponseException; +import org.opensearch.sql.legacy.TestUtils; +import org.opensearch.sql.ppl.PPLIntegTestCase; + +/** + * Backend half of the PPL lint rule validation contract. + * + *

This test drives the live {@code POST /_plugins/_ppl} endpoint on the SQL plugin built from + * the current checkout and asserts, per contract case, that: + * + *

+ * + *

The contract file is shared verbatim with the SQL-owned OSD frontend adapter ({@code + * scripts/ppl-lint/run-frontend-contract.mjs}) so the same reviewed cases pin both the OSD analyzer + * diagnostic and the SQL backend behavior. The rejection-body parsing mirrors the existing {@link + * org.opensearch.sql.calcite.remote.CalciteErrorReportStageIT} pattern; the live fixture and + * Calcite setup follow {@link org.opensearch.sql.calcite.remote.CalcitePPLEventstatsIT}. + */ +public class PplLintRuleValidationIT extends PPLIntegTestCase { + + private static final String CONTRACT_RESOURCE = + "src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json"; + + @Override + public void init() throws Exception { + super.init(); + // eventstats via the Calcite path. Disallow fallback so an unsupported window function is + // rejected rather than silently degrading to the V2 engine. + enableCalcite(); + disallowCalciteFallback(); + loadIndex(Index.ACCOUNT); + } + + @Test + public void testValidatesUnsupportedWindowFunctionContract() throws IOException { + JSONObject contract = loadContract(); + String index = contract.getString("index"); + JSONArray cases = contract.getJSONArray("cases"); + + for (int i = 0; i < cases.length(); i++) { + JSONObject testCase = cases.getJSONObject(i); + String caseId = testCase.getString("id"); + String query = testCase.getString("query").replace("{{index}}", index); + JSONObject backendExpected = testCase.getJSONObject("backendExpected"); + int expectedStatus = backendExpected.getInt("httpStatus"); + + if (expectedStatus == 200) { + verifyAcceptedCase(caseId, query); + } else { + verifyRejectedCase(caseId, query, expectedStatus, backendExpected.getJSONObject("body")); + } + } + } + + /** A valid control query must return HTTP 200. executeQuery already asserts the 200 status. */ + private void verifyAcceptedCase(String caseId, String query) throws IOException { + JSONObject response = executeQuery(query); + assertTrue( + "case \"" + + caseId + + "\": expected a datarows array in the 200 response for query: " + + query, + response.has("datarows")); + } + + /** + * A rejected query must throw a {@link ResponseException} whose response carries the contracted + * HTTP status and structured error fields. executeQuery internally asserts 200, so a non-200 + * response surfaces as a ResponseException before it can return. + */ + private void verifyRejectedCase( + String caseId, String query, int expectedStatus, JSONObject expectedBody) { + ResponseException exception = assertThrows(ResponseException.class, () -> executeQuery(query)); + + int actualStatus = exception.getResponse().getStatusLine().getStatusCode(); + assertEquals( + "case \"" + caseId + "\": unexpected HTTP status for query: " + query, + expectedStatus, + actualStatus); + + JSONObject body; + try { + body = new JSONObject(getResponseBody(exception.getResponse(), true)); + } catch (IOException e) { + throw new RuntimeException( + "case \"" + caseId + "\": failed to read rejection response body for query: " + query, e); + } + + assertEquals( + "case \"" + caseId + "\": unexpected top-level status field for query: " + query, + expectedBody.getInt("status"), + body.getInt("status")); + + JSONObject expectedError = expectedBody.getJSONObject("error"); + JSONObject actualError = body.getJSONObject("error"); + + assertEquals( + "case \"" + caseId + "\": unexpected error.type for query: " + query, + expectedError.getString("type"), + actualError.getString("type")); + assertEquals( + "case \"" + caseId + "\": unexpected error.reason for query: " + query, + expectedError.getString("reason"), + actualError.getString("reason")); + } + + private JSONObject loadContract() throws IOException { + String path = TestUtils.getResourceFilePath(CONTRACT_RESOURCE); + return new JSONObject(new String(Files.readAllBytes(Paths.get(path)))); + } +} diff --git a/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json b/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json new file mode 100644 index 00000000000..02fbab79a3c --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json @@ -0,0 +1,29 @@ +{ + "ruleId": "unsupported-window-function-in-eventstats", + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "eventstats-rank", + "query": "source={{index}} | eventstats rank() as rank_value", + "frontendDiagnosticCount": 1, + "backendExpected": { + "httpStatus": 400, + "body": { + "status": 400, + "error": { + "type": "CalciteUnsupportedException", + "reason": "Unexpected window function: rank" + } + } + } + }, + { + "id": "eventstats-avg-control", + "query": "source={{index}} | eventstats avg(age) as avg_age", + "frontendDiagnosticCount": 0, + "backendExpected": { + "httpStatus": 200 + } + } + ] +} diff --git a/scripts/ppl-lint-rule-validation.sh b/scripts/ppl-lint-rule-validation.sh new file mode 100755 index 00000000000..1552353d052 --- /dev/null +++ b/scripts/ppl-lint-rule-validation.sh @@ -0,0 +1,105 @@ +#!/usr/bin/env bash +# +# Copyright OpenSearch Contributors +# SPDX-License-Identifier: Apache-2.0 +# +# Local developer entry point for the PPL lint rule validation contract. +# +# Runs both halves of the cross-repository check from a SQL checkout: +# 1. Frontend: loads the compiled OpenSearch-Dashboards (OSD) PPL analyzer and +# asserts the rule's diagnostic counts against the shared contract. +# 2. Backend: runs the Gradle integration test against a live /_plugins/_ppl +# endpoint on the SQL plugin built from this checkout. +# +# Usage: +# # OSD main frontend check plus SQL backend IT (fetches OSD into .ci/) +# ./scripts/ppl-lint-rule-validation.sh +# +# # Reuse an existing OSD checkout (skips clone + bootstrap if node_modules present) +# OSD_SOURCE_PATH=../OpenSearch-Dashboards ./scripts/ppl-lint-rule-validation.sh +# +# # Reproduce a CI run against a specific OSD revision +# OSD_REF= ./scripts/ppl-lint-rule-validation.sh +# +# # Skip one half +# SKIP_BACKEND=1 ./scripts/ppl-lint-rule-validation.sh +# SKIP_FRONTEND=1 ./scripts/ppl-lint-rule-validation.sh + +set -euo pipefail + +SQL_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$SQL_ROOT" + +OSD_REPO_URL="${OSD_REPO_URL:-https://github.com/opensearch-project/OpenSearch-Dashboards.git}" +OSD_REF="${OSD_REF:-main}" +DEFAULT_OSD_CHECKOUT="$SQL_ROOT/.ci/OpenSearch-Dashboards" +CONTRACT_FILE="$SQL_ROOT/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json" +FRONTEND_SCRIPT="$SQL_ROOT/scripts/ppl-lint/run-frontend-contract.mjs" +IT_CLASS="org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" + +log() { echo "[ppl-lint-rule-validation] $*"; } + +resolve_opensearch_version() { + local raw + raw=$(grep -oE '"opensearch.version", "[^"]+"' build.gradle | head -1 | + sed -E 's/.*"opensearch.version", "([^"]+)"/\1/') + echo "${raw%%-*}" +} + +run_frontend() { + local osd_checkout="$1" + + if [[ ! -d "$osd_checkout/node_modules" ]]; then + log "Bootstrapping OSD at $osd_checkout (this can take a while)..." + (cd "$osd_checkout" && yarn osd bootstrap) + else + log "Reusing bootstrapped OSD at $osd_checkout (node_modules present)." + fi + + local os_version + os_version="$(resolve_opensearch_version)" + log "Running frontend contract against OSD analyzer (PPL_SQL_VERSION=$os_version)..." + ( + cd "$osd_checkout" + PPL_LINT_CONTRACT_FILE="$CONTRACT_FILE" \ + PPL_SQL_VERSION="$os_version" \ + node -r ./src/setup_node_env "$FRONTEND_SCRIPT" + ) +} + +if [[ "${SKIP_FRONTEND:-0}" != "1" ]]; then + if [[ -n "${OSD_SOURCE_PATH:-}" ]]; then + OSD_CHECKOUT="$(cd "$OSD_SOURCE_PATH" && pwd)" + log "Using existing OSD checkout: $OSD_CHECKOUT" + else + OSD_CHECKOUT="$DEFAULT_OSD_CHECKOUT" + if [[ ! -d "$OSD_CHECKOUT/.git" ]]; then + log "Cloning OSD ($OSD_REF) into $OSD_CHECKOUT ..." + mkdir -p "$(dirname "$OSD_CHECKOUT")" + git clone --depth 1 --branch "$OSD_REF" "$OSD_REPO_URL" "$OSD_CHECKOUT" 2>/dev/null || + git clone "$OSD_REPO_URL" "$OSD_CHECKOUT" + fi + log "Checking out OSD ref: $OSD_REF" + git -C "$OSD_CHECKOUT" fetch --depth 1 origin "$OSD_REF" 2>/dev/null || true + git -C "$OSD_CHECKOUT" checkout "$OSD_REF" 2>/dev/null || + git -C "$OSD_CHECKOUT" checkout FETCH_HEAD + fi + + OSD_SHA="$(git -C "$OSD_CHECKOUT" rev-parse HEAD)" + log "OSD revision under test: $OSD_SHA" + + run_frontend "$OSD_CHECKOUT" + log "Frontend contract passed." +else + log "SKIP_FRONTEND=1 — skipping the OSD frontend contract." +fi + +if [[ "${SKIP_BACKEND:-0}" != "1" ]]; then + log "Running backend integration test: $IT_CLASS" + ./gradlew :integ-test:integTest --tests "$IT_CLASS" + log "Backend integration test passed." +else + log "SKIP_BACKEND=1 — skipping the SQL backend integration test." +fi + +log "Done." diff --git a/scripts/ppl-lint/run-frontend-contract.mjs b/scripts/ppl-lint/run-frontend-contract.mjs new file mode 100644 index 00000000000..603e3feab6c --- /dev/null +++ b/scripts/ppl-lint/run-frontend-contract.mjs @@ -0,0 +1,164 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * SQL-owned frontend contract adapter for the PPL lint rule validation CI. + * + * This script is executed from inside an OpenSearch-Dashboards (OSD) checkout, + * for example: + * + * cd .ci/OpenSearch-Dashboards + * PPL_LINT_CONTRACT_FILE= \ + * PPL_SQL_VERSION= \ + * node -r ./src/setup_node_env \ + * "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" + * + * `node -r ./src/setup_node_env` installs OSD's process-wide auto-transpilation + * hook (`@osd/optimizer`'s `registerNodeAutoTranspilation`), which transpiles + * `packages/osd-monaco/src/**` TypeScript on `require()` regardless of where the + * entry script lives. That is what lets this SQL-owned `.mjs` load the compiled + * OSD analyzer without OSD's own Jest. + * + * The analyzer, catalog and detector registry are NOT re-exported from the + * `@osd/monaco` package barrel, so they are loaded via their deep module paths. + * Because this is an ES module, `require` is obtained with `createRequire`, and + * the modules are resolved against the OSD checkout root (process.cwd()) rather + * than the location of this script (which lives in the SQL repo, not OSD). + */ + +import assert from 'assert'; +import fs from 'fs'; +import path from 'path'; +import { createRequire } from 'module'; + +const RULE_MODULE = 'packages/osd-monaco/src/ppl/ppl_language_analyzer'; +const CATALOG_MODULE = 'packages/osd-monaco/src/ppl/lint/catalog'; +const DETECTOR_REGISTRY_MODULE = 'packages/osd-monaco/src/ppl/lint/detector_registry'; + +function fail(message) { + // eslint-disable-next-line no-console + console.error(`[ppl-lint-frontend-contract] FAIL: ${message}`); + process.exit(1); +} + +function loadContract() { + const contractFile = process.env.PPL_LINT_CONTRACT_FILE; + if (!contractFile) { + fail('PPL_LINT_CONTRACT_FILE is not set.'); + } + if (!fs.existsSync(contractFile)) { + fail(`Contract file not found: ${contractFile}`); + } + try { + return JSON.parse(fs.readFileSync(contractFile, 'utf8')); + } catch (error) { + fail(`Could not parse contract file ${contractFile}: ${error.message}`); + return undefined; // unreachable + } +} + +function loadOsdAnalyzer() { + // Resolve the OSD compiled analyzer from the checkout root. In CI this script + // runs after `cd .ci/OpenSearch-Dashboards`, so process.cwd() is that root. + const osdRoot = process.cwd(); + const require = createRequire(path.join(osdRoot, 'noop.js')); + + const resolveOsd = (relativeModule) => { + const absolute = path.join(osdRoot, relativeModule); + if (!fs.existsSync(`${absolute}.ts`) && !fs.existsSync(`${absolute}.js`)) { + fail( + `Expected OSD module not found under the checkout root: ${relativeModule}\n` + + `Resolved OSD root: ${osdRoot}\n` + + `Run this script from the OSD checkout (e.g. cd .ci/OpenSearch-Dashboards) after bootstrap.` + ); + } + return require(absolute); + }; + + const { PPLLanguageAnalyzer } = resolveOsd(RULE_MODULE); + const { getBundledCatalog } = resolveOsd(CATALOG_MODULE); + const { getDetector } = resolveOsd(DETECTOR_REGISTRY_MODULE); + + if (typeof PPLLanguageAnalyzer !== 'function') { + fail(`PPLLanguageAnalyzer was not a constructor when loaded from ${RULE_MODULE}.`); + } + return { PPLLanguageAnalyzer, getBundledCatalog, getDetector, osdRoot }; +} + +function assertRuleIsWiredUp(ruleId, getBundledCatalog, getDetector) { + const entry = getBundledCatalog().find((candidate) => candidate.id === ruleId); + if (!entry) { + fail(`Rule "${ruleId}" is not present in the OSD bundled catalog.`); + } + if (!entry.enabled) { + fail(`Rule "${ruleId}" is present but disabled in the OSD bundled catalog.`); + } + if (entry.severity !== 'error') { + fail(`Rule "${ruleId}" severity is "${entry.severity}", expected "error".`); + } + if (typeof getDetector(entry.detector) !== 'function') { + fail(`Rule "${ruleId}" has no registered detector "${entry.detector}".`); + } + return entry; +} + +function main() { + const contract = loadContract(); + const ruleId = contract.ruleId; + const index = contract.index; + const sqlVersion = process.env.PPL_SQL_VERSION; + + const { PPLLanguageAnalyzer, getBundledCatalog, getDetector, osdRoot } = loadOsdAnalyzer(); + + const entry = assertRuleIsWiredUp(ruleId, getBundledCatalog, getDetector); + + // eslint-disable-next-line no-console + console.log( + `[ppl-lint-frontend-contract] OSD root: ${osdRoot}\n` + + `[ppl-lint-frontend-contract] rule "${ruleId}" enabled=${entry.enabled} severity=${entry.severity} detector="${entry.detector}"\n` + + `[ppl-lint-frontend-contract] PPL_SQL_VERSION=${sqlVersion || '(unset)'}\n` + + `[ppl-lint-frontend-contract] running ${contract.cases.length} case(s) against index "${index}"` + ); + + const analyzer = new PPLLanguageAnalyzer(); + const failures = []; + + for (const testCase of contract.cases) { + const query = testCase.query.split('{{index}}').join(index); + const result = analyzer.lint(query, { + dataSourceVersion: sqlVersion, + isCalcite: true, + }); + const matches = result.diagnostics.filter((diagnostic) => diagnostic.ruleId === ruleId); + + // eslint-disable-next-line no-console + console.log( + `[ppl-lint-frontend-contract] ${testCase.id}: expected ${testCase.frontendDiagnosticCount}, ` + + `got ${matches.length} — ${query}` + ); + + try { + assert.strictEqual( + matches.length, + testCase.frontendDiagnosticCount, + `case "${testCase.id}": expected ${testCase.frontendDiagnosticCount} "${ruleId}" ` + + `diagnostic(s) but received ${matches.length} for query: ${query}` + ); + } catch (error) { + failures.push(error.message); + } + } + + if (failures.length > 0) { + fail(`${failures.length} case(s) failed:\n- ${failures.join('\n- ')}`); + } + + // eslint-disable-next-line no-console + console.log( + `[ppl-lint-frontend-contract] PASS: all ${contract.cases.length} case(s) matched for "${ruleId}".` + ); +} + +main(); From 738672e1d09254e4ca53c9668e5fecb58ee599e9 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Sun, 19 Jul 2026 17:41:10 -0700 Subject: [PATCH 02/10] style(ci): use $(...) instead of legacy backticks in ppl-lint workflow Addresses shellcheck SC2006/SC2046 on the chown/su lines so actionlint runs clean. Behavior is unchanged. Signed-off-by: Hanyu Wei --- .github/workflows/ppl-lint-rule-validation.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index 61ee0be6fb8..4d545ea677b 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -133,8 +133,8 @@ jobs: - name: Run backend integration test (live /_plugins/_ppl) run: | - chown -R 1000:1000 `pwd` - su `id -un 1000` -c "./gradlew :integ-test:integTest --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" + chown -R 1000:1000 "$(pwd)" + su "$(id -un 1000)" -c "./gradlew :integ-test:integTest --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" - name: Upload failure artifacts if: ${{ failure() }} From 74fcfaa6a5e6ece5c87752f02b767a428a436936 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Sun, 19 Jul 2026 20:59:33 -0700 Subject: [PATCH 03/10] ci: re-trigger PPL lint rule validation after Actions recovery Signed-off-by: Hanyu Wei From 9aca44c40e296278a8fda1afffe6a2bc04c6018c Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Sun, 19 Jul 2026 22:01:59 -0700 Subject: [PATCH 04/10] fix(ci): split ppl-lint validation into frontend/backend jobs The OpenSearch CI container is Amazon Linux 2 (glibc 2.26), but OSD requires Node 22 whose prebuilt binary needs glibc >= 2.27. Running the Node frontend contract inside that container failed with 'GLIBC_2.27 not found'. Split into two required jobs: 'frontend' runs the OSD analyzer contract on a bare ubuntu-latest runner (modern glibc, actions/setup-node works), and 'backend' keeps the Gradle integration test in the CI container where the OpenSearch test cluster needs it. Signed-off-by: Hanyu Wei --- .../workflows/ppl-lint-rule-validation.yml | 105 ++++++++++-------- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index 4d545ea677b..9e9d5e11dc8 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -3,10 +3,17 @@ name: PPL lint rule validation # Cross-repository check: the OpenSearch-Dashboards (OSD) PPL lint rule # `unsupported-window-function-in-eventstats` and the SQL backend must agree. # -# Frontend half: a SQL-owned Node script loads the compiled OSD analyzer from an -# OSD checkout and asserts the rule's diagnostic counts. -# Backend half: a Gradle integration test sends the same queries to the live -# `/_plugins/_ppl` endpoint of the SQL plugin built from this checkout. +# Frontend half (frontend job): a SQL-owned Node script loads the compiled OSD +# analyzer from an OSD checkout and asserts the rule's diagnostic counts. This +# runs on a bare ubuntu-latest runner because OSD requires a modern Node whose +# prebuilt binary needs a newer glibc than the OpenSearch CI container (Amazon +# Linux 2) provides. +# Backend half (backend job): a Gradle integration test sends the same queries +# to the live `/_plugins/_ppl` endpoint of the SQL plugin built from this +# checkout. This runs inside the OpenSearch CI container because the Gradle +# test cluster needs it. +# +# Both jobs are required; a failure on either side fails the SQL PR check. # # The OSD detector is loaded from `main` by default (PR and nightly runs), so a # removed or changed detector is detected. `workflow_dispatch` can target a @@ -24,23 +31,10 @@ on: type: string jobs: - Get-CI-Image-Tag: - uses: opensearch-project/opensearch-build/.github/workflows/get-ci-image-tag.yml@761e093b8c1349cc07f21c1d681d3b30bf9e1999 # main - with: - product: opensearch - - ppl-lint-rule-validation: - name: PPL lint rule validation - needs: Get-CI-Image-Tag + frontend: + name: Frontend contract (OSD analyzer) runs-on: ubuntu-latest - container: - image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} - options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} - steps: - - name: Run start commands - run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }} - - name: Checkout SQL pull request uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 @@ -58,49 +52,33 @@ jobs: path: .ci/OpenSearch-Dashboards - name: Record OSD revision - id: osd-sha run: | sha=$(git -C .ci/OpenSearch-Dashboards rev-parse HEAD) - echo "sha=$sha" >> "$GITHUB_OUTPUT" echo "OSD revision: \`$sha\` (ref: ${{ steps.osd-ref.outputs.ref }})" >> "$GITHUB_STEP_SUMMARY" - - name: Set up JDK 21 - uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4 - with: - distribution: 'temurin' - java-version: 21 - # Read the Node/Yarn toolchain from the OSD checkout rather than hardcoding # it, so an OSD toolchain bump does not silently drift this job. - - name: Read OSD Node version - id: osd-node - run: echo "version=$(cat .ci/OpenSearch-Dashboards/.nvmrc)" >> "$GITHUB_OUTPUT" - - - name: Set up Node ${{ steps.osd-node.outputs.version }} + - name: Set up Node from OSD .nvmrc uses: actions/setup-node@1e60f620b9541d16bece96c5465dc8ee9832be0b # v4 with: - node-version: ${{ steps.osd-node.outputs.version }} + node-version-file: .ci/OpenSearch-Dashboards/.nvmrc - - name: Read OSD Yarn version - id: osd-yarn + - name: Pin Yarn from OSD engines working-directory: .ci/OpenSearch-Dashboards run: | yarn_range=$(node -e "process.stdout.write(require('./package.json').engines.yarn)") # Take the lower bound of the engines.yarn range (e.g. "^1.22.10" -> "1.22.10"). yarn_version=$(echo "$yarn_range" | sed -E 's/[^0-9.]//g') - echo "version=$yarn_version" >> "$GITHUB_OUTPUT" - - - name: Pin Yarn ${{ steps.osd-yarn.outputs.version }} - run: npm install -g "yarn@${{ steps.osd-yarn.outputs.version }}" + npm install -g "yarn@${yarn_version}" - name: Cache OSD Yarn dependencies uses: actions/cache@0c907a75c2c80ebcb7f088228285e798b750cf8f # v4 with: path: | ~/.cache/yarn - key: ${{ runner.os }}-osd-yarn-node${{ steps.osd-node.outputs.version }}-${{ hashFiles('.ci/OpenSearch-Dashboards/yarn.lock') }} + key: ${{ runner.os }}-osd-yarn-${{ hashFiles('.ci/OpenSearch-Dashboards/yarn.lock') }} restore-keys: | - ${{ runner.os }}-osd-yarn-node${{ steps.osd-node.outputs.version }}- + ${{ runner.os }}-osd-yarn- - name: Bootstrap OpenSearch-Dashboards working-directory: .ci/OpenSearch-Dashboards @@ -118,10 +96,7 @@ jobs: echo "version=$version" >> "$GITHUB_OUTPUT" echo "OpenSearch version: \`$version\` (from \`$raw\`)" >> "$GITHUB_STEP_SUMMARY" - # The frontend contract only runs Node (no OpenSearch cluster), so it runs - # as the default container user. Only the Gradle integTest below must run - # as a non-root user, because OpenSearch refuses to start as root. - - name: Run frontend contract (OSD analyzer) + - name: Run frontend contract working-directory: .ci/OpenSearch-Dashboards env: PPL_LINT_CONTRACT_FILE: ${{ github.workspace }}/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json @@ -131,7 +106,42 @@ jobs: "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" \ | tee "$GITHUB_WORKSPACE/frontend-contract.log" - - name: Run backend integration test (live /_plugins/_ppl) + - name: Upload frontend log + if: ${{ failure() }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + continue-on-error: true + with: + name: ppl-lint-frontend-contract-log + path: frontend-contract.log + + Get-CI-Image-Tag: + uses: opensearch-project/opensearch-build/.github/workflows/get-ci-image-tag.yml@761e093b8c1349cc07f21c1d681d3b30bf9e1999 # main + with: + product: opensearch + + backend: + name: Backend integration test (live /_plugins/_ppl) + needs: Get-CI-Image-Tag + runs-on: ubuntu-latest + container: + image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} + options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} + + steps: + - name: Run start commands + run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }} + + - name: Checkout SQL pull request + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - name: Set up JDK 21 + uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4 + with: + distribution: 'temurin' + java-version: 21 + + # OpenSearch refuses to start as root, so run Gradle as a non-root user. + - name: Run backend integration test run: | chown -R 1000:1000 "$(pwd)" su "$(id -un 1000)" -c "./gradlew :integ-test:integTest --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" @@ -141,8 +151,7 @@ jobs: uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 continue-on-error: true with: - name: ppl-lint-rule-validation-artifacts + name: ppl-lint-backend-artifacts path: | - frontend-contract.log integ-test/build/reports/** integ-test/build/testclusters/*/logs/* From 1745ed90d2aed8c3c586c052c5a02c52c4905dc1 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Sun, 19 Jul 2026 23:38:44 -0700 Subject: [PATCH 05/10] feat(ci): extend PPL lint contract to all reachable rules (schema v2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalize the single-rule PPL lint validation contract (eventstats PoC) into a schema-v2 corpus that pins every reachable OSD analyzer rule to live /_plugins/_ppl behavior. Both halves read the same reviewed contract files so neither the analyzer diagnostic nor the engine behavior can drift without a red build. Verified live end-to-end: frontend 13/13 against the OSD main analyzer; backend 13/13 (pr) and 21/21 (nightly) against a live test cluster. Contract schema v2 (integ-test/src/test/resources/ppl-lint/contracts/*.spec.json + manifest.json): - backend.kind discriminator: rejection | result-shape | advisory (explain reserved for the nightly-only explain rule class once it lands on OSD main). - per-contract backendFixture.clusterSettings so contracts that disagree on fallback/join settings each set what they need (eventstats needs calciteFallback=false; dedup-consecutive needs true) — validated in one run. - per-case minVersionRequired/engineRequired so both halves skip identically. - wiring block asserted deep-equal against the OSD catalog (drift tripwire). - frontendContext.deriveFromMapping single-sources fields/typeMap for the field-validation existence pass. - error.reason values snapshotted from the observed engine response, not hand-typed (join/multisearch AST-build-time throws yield generic "Invalid Query"; union/replace carry the specific message). Frontend adapter (run-frontend-contract.mjs): contract discovery via manifest, schedule + version/engine gating, catalog wiring assertion, compiled-simplified and runtime-bundle grammar surfaces (runtime-only rules whose parser rules are absent on the checkout's grammar assert wiring then skip cleanly), collect-all failures, and a frontend-report.json for disagreement diffing. Nightly adds a coverage assertion that every enabled catalog rule has a contract. Backend IT: parameterized over the contract corpus with per-kind verifiers (verifyRejectedCase / verifyResultShape / verifyAdvisory200), per-contract cluster-setting apply+reset, GET / cluster-version gating, and a backend-report.json recording observed status/type/reason per rejection. Sends queries with a JSON-escaped body so contract queries containing quotes (grok field=body "...") reach the engine faithfully instead of tripping a core request-payload parse error. Honors -Dppl.lint.schedule=pr|nightly, forwarded to the forked test JVM via integ-test/build.gradle. Workflow + repro script: derive schedule (PR -> pr, cron -> nightly), read the contract dir, upload frontend/backend reports + corpus artifacts. Rules covered: eventstats window fn, division-by-zero, head-without-sort, disabled-join-type, field-validation (shape + existence) on PR; plus dedup-consecutive and the runtime-only union/multisearch/replace on nightly. Signed-off-by: Hanyu Wei --- .../workflows/ppl-lint-rule-validation.yml | 95 +++- .gitignore | 7 +- integ-test/build.gradle | 11 + .../remote/PplLintRuleValidationIT.java | 495 ++++++++++++++++-- .../dedup-consecutive-unsupported.spec.json | 42 ++ .../contracts/disabled-join-type.spec.json | 52 ++ .../contracts/division-by-zero.spec.json | 44 ++ .../contracts/field-validation.spec.json | 66 +++ .../contracts/head-without-sort.spec.json | 38 ++ .../ppl-lint/contracts/manifest.json | 15 + .../multisearch-min-subsearch.spec.json | 47 ++ .../replace-wildcard-asymmetry.spec.json | 53 ++ .../contracts/union-min-datasets.spec.json | 47 ++ ...ed-window-function-in-eventstats.spec.json | 49 ++ ...ed-window-function-in-eventstats.spec.json | 29 - scripts/ppl-lint-rule-validation.sh | 19 +- scripts/ppl-lint/run-frontend-contract.mjs | 470 ++++++++++++++--- 17 files changed, 1399 insertions(+), 180 deletions(-) create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/manifest.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json create mode 100644 integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json delete mode 100644 integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index 9e9d5e11dc8..53f4d14ca74 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -1,23 +1,30 @@ name: PPL lint rule validation -# Cross-repository check: the OpenSearch-Dashboards (OSD) PPL lint rule -# `unsupported-window-function-in-eventstats` and the SQL backend must agree. +# Cross-repository check: the OpenSearch-Dashboards (OSD) PPL lint rules and the +# SQL backend must agree. A shared, reviewed corpus of contract files pins each +# rule's OSD analyzer diagnostic count to the live SQL engine's behavior, so +# neither side can drift unilaterally without a red build. # # Frontend half (frontend job): a SQL-owned Node script loads the compiled OSD -# analyzer from an OSD checkout and asserts the rule's diagnostic counts. This -# runs on a bare ubuntu-latest runner because OSD requires a modern Node whose -# prebuilt binary needs a newer glibc than the OpenSearch CI container (Amazon -# Linux 2) provides. -# Backend half (backend job): a Gradle integration test sends the same queries -# to the live `/_plugins/_ppl` endpoint of the SQL plugin built from this -# checkout. This runs inside the OpenSearch CI container because the Gradle -# test cluster needs it. +# analyzer from an OSD checkout and asserts each rule's diagnostic counts and +# catalog wiring. This runs on a bare ubuntu-latest runner because OSD requires a +# modern Node whose prebuilt binary needs a newer glibc than the OpenSearch CI +# container (Amazon Linux 2) provides. +# Backend half (backend job): a Gradle integration test sends the same queries to +# the live `/_plugins/_ppl` endpoint of the SQL plugin built from this checkout. +# This runs inside the OpenSearch CI container because the Gradle test cluster +# needs it. # # Both jobs are required; a failure on either side fails the SQL PR check. # -# The OSD detector is loaded from `main` by default (PR and nightly runs), so a -# removed or changed detector is detected. `workflow_dispatch` can target a -# specific OSD ref to reproduce a run or test an unmerged OSD branch. +# Schedule: pull_request and workflow_dispatch run the fast, deterministic +# `schedule:pr` subset. The nightly cron runs the full corpus — runtime-only +# rules, advisory/soft-oracle rules, and a coverage assertion that every enabled +# OSD catalog rule has a contract file. +# +# The OSD detector is loaded from `main` by default, so a removed or changed +# detector is caught. `workflow_dispatch` can target a specific OSD ref to +# reproduce a run or pre-validate an unmerged OSD branch. on: pull_request: @@ -29,6 +36,11 @@ on: description: OSD commit or branch to test instead of main required: false type: string + schedule: + description: Contract schedule to run (pr or nightly) + required: false + default: pr + type: string jobs: frontend: @@ -44,6 +56,22 @@ jobs: REQUESTED_REF: ${{ inputs.osd_ref }} run: echo "ref=${REQUESTED_REF:-main}" >> "$GITHUB_OUTPUT" + - name: Resolve contract schedule + id: schedule + env: + REQUESTED_SCHEDULE: ${{ inputs.schedule }} + EVENT_NAME: ${{ github.event_name }} + run: | + if [ -n "$REQUESTED_SCHEDULE" ]; then + value="$REQUESTED_SCHEDULE" + elif [ "$EVENT_NAME" = "schedule" ]; then + value="nightly" + else + value="pr" + fi + echo "value=$value" >> "$GITHUB_OUTPUT" + echo "Contract schedule: \`$value\`" >> "$GITHUB_STEP_SUMMARY" + - name: Checkout OpenSearch-Dashboards uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: @@ -99,20 +127,25 @@ jobs: - name: Run frontend contract working-directory: .ci/OpenSearch-Dashboards env: - PPL_LINT_CONTRACT_FILE: ${{ github.workspace }}/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json + PPL_LINT_CONTRACT_DIR: ${{ github.workspace }}/integ-test/src/test/resources/ppl-lint/contracts + PPL_LINT_SCHEDULE: ${{ steps.schedule.outputs.value }} PPL_SQL_VERSION: ${{ steps.os-version.outputs.version }} + PPL_LINT_REPORT: ${{ github.workspace }}/frontend-report.json run: | node -r ./src/setup_node_env \ "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" \ | tee "$GITHUB_WORKSPACE/frontend-contract.log" - - name: Upload frontend log - if: ${{ failure() }} + - name: Upload frontend report and corpus + if: ${{ always() }} uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 continue-on-error: true with: - name: ppl-lint-frontend-contract-log - path: frontend-contract.log + name: ppl-lint-frontend-report + path: | + frontend-contract.log + frontend-report.json + integ-test/src/test/resources/ppl-lint/contracts Get-CI-Image-Tag: uses: opensearch-project/opensearch-build/.github/workflows/get-ci-image-tag.yml@761e093b8c1349cc07f21c1d681d3b30bf9e1999 # main @@ -134,6 +167,21 @@ jobs: - name: Checkout SQL pull request uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - name: Resolve contract schedule + id: schedule + env: + REQUESTED_SCHEDULE: ${{ inputs.schedule }} + EVENT_NAME: ${{ github.event_name }} + run: | + if [ -n "$REQUESTED_SCHEDULE" ]; then + value="$REQUESTED_SCHEDULE" + elif [ "$EVENT_NAME" = "schedule" ]; then + value="nightly" + else + value="pr" + fi + echo "value=$value" >> "$GITHUB_OUTPUT" + - name: Set up JDK 21 uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4 with: @@ -144,7 +192,16 @@ jobs: - name: Run backend integration test run: | chown -R 1000:1000 "$(pwd)" - su "$(id -un 1000)" -c "./gradlew :integ-test:integTest --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" + su "$(id -un 1000)" -c "./gradlew :integ-test:integTest --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT -Dppl.lint.schedule=${{ steps.schedule.outputs.value }} -Dppl.lint.report=$(pwd)/backend-report.json" + + - name: Upload backend report + if: ${{ always() }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + continue-on-error: true + with: + name: ppl-lint-backend-report + path: | + backend-report.json - name: Upload failure artifacts if: ${{ failure() }} diff --git a/.gitignore b/.gitignore index bf9002f999d..0f4dc3f32e2 100644 --- a/.gitignore +++ b/.gitignore @@ -59,4 +59,9 @@ http-client.env.json !.claude/harness/ .claude/settings.local.json .clinerules -memory-bank \ No newline at end of file +memory-bank +# PPL lint rule validation contract run artifacts (uploaded in CI, not committed) +frontend-report.json +backend-report.json +backend-report-nightly.json +frontend-contract.log diff --git a/integ-test/build.gradle b/integ-test/build.gradle index c18fa6e37f6..3a25caf99df 100644 --- a/integ-test/build.gradle +++ b/integ-test/build.gradle @@ -168,6 +168,17 @@ tasks.withType(licenseHeaders.class) { additionalLicense 'AL ', 'Apache', 'Licensed under the Apache License, Version 2.0 (the "License")' } +// Forward the PPL lint rule validation contract knobs to every integ test JVM +// (PplLintRuleValidationIT reads them): which schedule to run (pr|nightly) and +// an optional path to write the observed-vs-expected report. Applied globally so +// every RestIntegTestTask that runs the class picks it up without per-task edits. +tasks.withType(Test).configureEach { + systemProperty "ppl.lint.schedule", System.getProperty("ppl.lint.schedule", "pr") + if (System.getProperty("ppl.lint.report") != null) { + systemProperty "ppl.lint.report", System.getProperty("ppl.lint.report") + } +} + validateNebulaPom.enabled = false loggerUsageCheck.enabled = false diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java index ac92f979595..211ad341293 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java @@ -6,96 +6,188 @@ package org.opensearch.sql.calcite.remote; import static org.opensearch.sql.legacy.TestUtils.getResponseBody; +import static org.opensearch.sql.plugin.rest.RestPPLQueryAction.QUERY_API_ENDPOINT; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Locale; +import java.util.Set; import org.json.JSONArray; import org.json.JSONObject; import org.junit.jupiter.api.Test; +import org.opensearch.client.Request; +import org.opensearch.client.RequestOptions; +import org.opensearch.client.Response; import org.opensearch.client.ResponseException; +import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.legacy.TestUtils; import org.opensearch.sql.ppl.PPLIntegTestCase; /** - * Backend half of the PPL lint rule validation contract. + * Backend half of the schema-v2 PPL lint rule validation contract. * *

This test drives the live {@code POST /_plugins/_ppl} endpoint on the SQL plugin built from - * the current checkout and asserts, per contract case, that: + * the current checkout. For every contract case (see {@code + * src/test/resources/ppl-lint/contracts/*.spec.json}) it applies the case's cluster settings and + * asserts, per {@code backend.kind}: * *

* - *

The contract file is shared verbatim with the SQL-owned OSD frontend adapter ({@code + *

The contract files are shared verbatim with the SQL-owned OSD frontend adapter ({@code * scripts/ppl-lint/run-frontend-contract.mjs}) so the same reviewed cases pin both the OSD analyzer - * diagnostic and the SQL backend behavior. The rejection-body parsing mirrors the existing {@link - * org.opensearch.sql.calcite.remote.CalciteErrorReportStageIT} pattern; the live fixture and - * Calcite setup follow {@link org.opensearch.sql.calcite.remote.CalcitePPLEventstatsIT}. + * diagnostic count and the SQL backend behavior; neither side can drift without a red build. The + * rejection-body parsing mirrors {@link + * org.opensearch.sql.calcite.remote.CalciteErrorReportStageIT}; the Calcite setup follows {@link + * org.opensearch.sql.calcite.remote.CalcitePPLEventstatsIT}. + * + *

The suite honors {@code -Dppl.lint.schedule=pr|nightly} (default {@code pr}): PR runs only the + * fast, deterministic {@code schedule:pr} contracts; nightly runs the full corpus including the + * runtime-only and softer-oracle rules. */ public class PplLintRuleValidationIT extends PPLIntegTestCase { - private static final String CONTRACT_RESOURCE = - "src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json"; + private static final String CONTRACT_DIR = "src/test/resources/ppl-lint/contracts"; + private static final String MANIFEST = CONTRACT_DIR + "/manifest.json"; + + /** Which contracts to run this session; PR is the fast blocking subset. */ + private final String schedule = System.getProperty("ppl.lint.schedule", "pr"); + + private int[] clusterVersion; @Override public void init() throws Exception { super.init(); - // eventstats via the Calcite path. Disallow fallback so an unsupported window function is - // rejected rather than silently degrading to the V2 engine. enableCalcite(); - disallowCalciteFallback(); - loadIndex(Index.ACCOUNT); + // Seed the union of every index every scheduled contract needs, once. + for (String indexEnum : requiredIndexEnums()) { + loadIndex(Index.valueOf(indexEnum)); + } + clusterVersion = fetchClusterVersion(); } @Test - public void testValidatesUnsupportedWindowFunctionContract() throws IOException { - JSONObject contract = loadContract(); + public void testValidatesLintRuleContracts() throws IOException { + List contracts = loadScheduledContracts(); + List failures = new ArrayList<>(); + JSONArray report = new JSONArray(); + + for (JSONObject contract : contracts) { + String ruleId = contract.getString("ruleId"); + runContract(contract, ruleId, failures, report); + } + + writeReport(report); + + if (!failures.isEmpty()) { + fail( + "PPL lint backend contract failures (" + + failures.size() + + "):\n- " + + String.join("\n- ", failures)); + } + } + + private void runContract( + JSONObject contract, String ruleId, List failures, JSONArray report) + throws IOException { String index = contract.getString("index"); JSONArray cases = contract.getJSONArray("cases"); + JSONObject fixture = contract.optJSONObject("backendFixture"); - for (int i = 0; i < cases.length(); i++) { - JSONObject testCase = cases.getJSONObject(i); - String caseId = testCase.getString("id"); - String query = testCase.getString("query").replace("{{index}}", index); - JSONObject backendExpected = testCase.getJSONObject("backendExpected"); - int expectedStatus = backendExpected.getInt("httpStatus"); + List applied = applyClusterSettings(fixture); + try { + for (int i = 0; i < cases.length(); i++) { + JSONObject testCase = cases.getJSONObject(i); + String caseId = testCase.getString("id"); + String query = testCase.getString("query").replace("{{index}}", index); - if (expectedStatus == 200) { - verifyAcceptedCase(caseId, query); - } else { - verifyRejectedCase(caseId, query, expectedStatus, backendExpected.getJSONObject("body")); + String minVersion = testCase.optString("minVersionRequired", null); + if (minVersion != null && !versionAtLeast(minVersion)) { + log(ruleId, caseId, "SKIP (needs >= " + minVersion + ")"); + continue; + } + + JSONObject backend = resolveBackend(testCase); + String kind = backend.getString("kind"); + JSONObject entry = reportEntry(ruleId, caseId, query, kind); + try { + verifyCase(kind, caseId, query, backend, entry); + entry.put("outcome", "pass"); + log(ruleId, caseId, "PASS (" + kind + ")"); + } catch (AssertionError | RuntimeException e) { + entry.put("outcome", "fail").put("error", String.valueOf(e.getMessage())); + failures.add("[" + ruleId + "/" + caseId + "] " + e.getMessage()); + log(ruleId, caseId, "FAIL (" + kind + "): " + e.getMessage()); + } + report.put(entry); } + } finally { + resetClusterSettings(applied); } } - /** A valid control query must return HTTP 200. executeQuery already asserts the 200 status. */ - private void verifyAcceptedCase(String caseId, String query) throws IOException { - JSONObject response = executeQuery(query); - assertTrue( - "case \"" - + caseId - + "\": expected a datarows array in the 200 response for query: " - + query, - response.has("datarows")); + private void verifyCase( + String kind, String caseId, String query, JSONObject backend, JSONObject reportEntry) + throws IOException { + switch (kind) { + case "rejection": + verifyRejectedCase( + caseId, + query, + backend.getInt("httpStatus"), + backend.getJSONObject("body"), + reportEntry); + break; + case "result-shape": + verifyResultShape(caseId, query, backend.optJSONObject("expect")); + break; + case "advisory": + verifyAdvisory200(caseId, query); + break; + default: + throw new IllegalArgumentException( + "case \"" + caseId + "\": unknown backend.kind \"" + kind + "\""); + } } - /** - * A rejected query must throw a {@link ResponseException} whose response carries the contracted - * HTTP status and structured error fields. executeQuery internally asserts 200, so a non-200 - * response surfaces as a ResponseException before it can return. - */ + /** Back-compat: accept both v2 {@code backend} and the legacy {@code backendExpected} shape. */ + private JSONObject resolveBackend(JSONObject testCase) { + if (testCase.has("backend")) { + return testCase.getJSONObject("backend"); + } + JSONObject legacy = testCase.getJSONObject("backendExpected"); + int status = legacy.getInt("httpStatus"); + JSONObject backend = new JSONObject(); + if (status == 200) { + return backend.put("kind", "result-shape").put("httpStatus", 200); + } + return backend + .put("kind", "rejection") + .put("httpStatus", status) + .put("body", legacy.getJSONObject("body")); + } + + /** A rejected query must throw with the contracted status and structured error fields. */ private void verifyRejectedCase( - String caseId, String query, int expectedStatus, JSONObject expectedBody) { - ResponseException exception = assertThrows(ResponseException.class, () -> executeQuery(query)); + String caseId, + String query, + int expectedStatus, + JSONObject expectedBody, + JSONObject reportEntry) { + ResponseException exception = assertThrows(ResponseException.class, () -> runPplQuery(query)); int actualStatus = exception.getResponse().getStatusLine().getStatusCode(); - assertEquals( - "case \"" + caseId + "\": unexpected HTTP status for query: " + query, - expectedStatus, - actualStatus); JSONObject body; try { @@ -105,26 +197,321 @@ private void verifyRejectedCase( "case \"" + caseId + "\": failed to read rejection response body for query: " + query, e); } + // Record the observed status/type/reason before asserting so backend-report.json + // carries the byte-exact engine wording even for a failing case — this is what + // the snapshot should be updated to when the contract is deliberately changed. + JSONObject observed = new JSONObject().put("httpStatus", actualStatus); + JSONObject actualError = body.optJSONObject("error"); + if (actualError != null) { + observed.put("type", actualError.opt("type")).put("reason", actualError.opt("reason")); + } + reportEntry.put("observed", observed); + + assertEquals( + "case \"" + caseId + "\": unexpected HTTP status for query: " + query, + expectedStatus, + actualStatus); + assertEquals( "case \"" + caseId + "\": unexpected top-level status field for query: " + query, expectedBody.getInt("status"), body.getInt("status")); JSONObject expectedError = expectedBody.getJSONObject("error"); - JSONObject actualError = body.getJSONObject("error"); assertEquals( "case \"" + caseId + "\": unexpected error.type for query: " + query, expectedError.getString("type"), actualError.getString("type")); - assertEquals( - "case \"" + caseId + "\": unexpected error.reason for query: " + query, - expectedError.getString("reason"), - actualError.getString("reason")); + if (expectedError.has("reason")) { + assertEquals( + "case \"" + caseId + "\": unexpected error.reason for query: " + query, + expectedError.getString("reason"), + actualError.getString("reason")); + } } - private JSONObject loadContract() throws IOException { - String path = TestUtils.getResourceFilePath(CONTRACT_RESOURCE); + /** A result-shape case returns 200 whose datarows match the declared expectations. */ + private void verifyResultShape(String caseId, String query, JSONObject expect) + throws IOException { + JSONObject response = runPplQuery(query); + assertTrue( + "case \"" + + caseId + + "\": expected a datarows array in the 200 response for query: " + + query, + response.has("datarows")); + if (expect == null) { + return; + } + JSONArray datarows = response.getJSONArray("datarows"); + + if (expect.optBoolean("datarowsNonEmpty", false)) { + assertTrue( + "case \"" + caseId + "\": expected non-empty datarows for query: " + query, + datarows.length() > 0); + } + if (expect.has("datarowsCount")) { + assertEquals( + "case \"" + caseId + "\": unexpected datarows count for query: " + query, + expect.getInt("datarowsCount"), + datarows.length()); + } + if (expect.has("columnAllNull")) { + String column = expect.getString("columnAllNull"); + int columnIndex = schemaColumnIndex(response, column); + assertTrue( + "case \"" + + caseId + + "\": column \"" + + column + + "\" not found in schema for query: " + + query, + columnIndex >= 0); + assertTrue( + "case \"" + + caseId + + "\": expected non-empty datarows to check null column for query: " + + query, + datarows.length() > 0); + for (int r = 0; r < datarows.length(); r++) { + JSONArray row = datarows.getJSONArray(r); + assertTrue( + "case \"" + + caseId + + "\": expected column \"" + + column + + "\" to be null in every row but row " + + r + + " was " + + row.get(columnIndex) + + " for query: " + + query, + row.isNull(columnIndex)); + } + } + } + + /** An advisory case only requires the query to be accepted (HTTP 200 with data). */ + private void verifyAdvisory200(String caseId, String query) throws IOException { + JSONObject response = runPplQuery(query); + assertTrue( + "case \"" + + caseId + + "\": expected a datarows array in the 200 response for query: " + + query, + response.has("datarows")); + } + + /** + * POST a PPL query to {@code /_plugins/_ppl} with a JSON-escaped body. The inherited {@code + * executeQuery} raw-interpolates the query into {@code {"query":"%s"}}, so a contract query that + * contains a double quote (e.g. {@code grok field=body "%{WORD:w}"}) would break the request + * payload and surface a spurious core-REST parse error instead of the real engine behavior. Build + * the body with a JSON serializer so any query is sent faithfully. Asserts HTTP 200 (a non-200 + * surfaces as a ResponseException, which the rejection path expects). + */ + private JSONObject runPplQuery(String query) throws IOException { + Request request = new Request("POST", QUERY_API_ENDPOINT); + request.setJsonEntity(new JSONObject().put("query", query).toString()); + RequestOptions.Builder options = RequestOptions.DEFAULT.toBuilder(); + options.addHeader("Content-Type", "application/json"); + request.setOptions(options); + + Response response = client().performRequest(request); + assertEquals(200, response.getStatusLine().getStatusCode()); + return new JSONObject(getResponseBody(response, true)); + } + + private int schemaColumnIndex(JSONObject response, String column) { + if (!response.has("schema")) { + return -1; + } + JSONArray schema = response.getJSONArray("schema"); + for (int i = 0; i < schema.length(); i++) { + JSONObject col = schema.getJSONObject(i); + String name = col.optString("alias", col.optString("name", "")); + if (column.equals(name) || column.equals(col.optString("name", ""))) { + return i; + } + } + return -1; + } + + // --- cluster settings ------------------------------------------------------ + + /** + * Apply the contract's cluster settings and return the list of settings changed so the caller can + * reset them afterwards. Grouped per-contract (not global) because contracts disagree: eventstats + * needs {@code calciteFallback=false} to force rejection, while dedup-consecutive needs it {@code + * true} to succeed via V2 fallback. + */ + private List applyClusterSettings(JSONObject fixture) throws IOException { + List applied = new ArrayList<>(); + if (fixture == null) { + return applied; + } + JSONObject settings = fixture.optJSONObject("clusterSettings"); + if (settings == null) { + return applied; + } + if (settings.has("calcite")) { + if (settings.getBoolean("calcite")) { + enableCalcite(); + } else { + disableCalcite(); + } + } + if (settings.has("calciteFallback")) { + if (settings.getBoolean("calciteFallback")) { + allowCalciteFallback(); + } else { + disallowCalciteFallback(); + } + } + if (settings.has("allJoinTypesAllowed")) { + String key = Settings.Key.CALCITE_SUPPORT_ALL_JOIN_TYPES.getKeyValue(); + String value = Boolean.toString(settings.getBoolean("allJoinTypesAllowed")); + updateClusterSettings(new PPLIntegTestCase.ClusterSetting("persistent", key, value)); + applied.add(key); + } + return applied; + } + + /** + * Reset each explicitly-applied dynamic setting to its cluster default by writing a null value. + * (calcite/calciteFallback are toggled via the inherited helpers and re-set explicitly by each + * contract, so only the persistent settings applied here are reset.) + */ + private void resetClusterSettings(List appliedKeys) { + for (String key : appliedKeys) { + try { + updateClusterSettings(new PPLIntegTestCase.ClusterSetting("persistent", key, null)); + } catch (IOException e) { + // Best-effort reset; the next contract sets what it needs explicitly, so + // keep the failure visible without failing the suite. + System.err.println("[ppl-lint] failed to reset a cluster setting: " + e.getMessage()); + } + } + } + + // --- version gating -------------------------------------------------------- + + private int[] fetchClusterVersion() { + try { + Response response = client().performRequest(new Request("GET", "/")); + JSONObject body = new JSONObject(getResponseBody(response, false)); + String number = body.getJSONObject("version").getString("number"); + return parseVersion(number); + } catch (Exception e) { + // Unknown version → do not skip anything. + return null; + } + } + + private boolean versionAtLeast(String required) { + if (clusterVersion == null) { + return true; + } + int[] want = parseVersion(required); + for (int i = 0; i < 3; i++) { + if (clusterVersion[i] > want[i]) return true; + if (clusterVersion[i] < want[i]) return false; + } + return true; + } + + private int[] parseVersion(String raw) { + String cleaned = raw.split("-")[0]; + String[] parts = cleaned.split("\\."); + int[] v = new int[] {0, 0, 0}; + for (int i = 0; i < 3 && i < parts.length; i++) { + try { + v[i] = Integer.parseInt(parts[i]); + } catch (NumberFormatException ignored) { + v[i] = 0; + } + } + return v; + } + + // --- contract loading ------------------------------------------------------ + + private List loadScheduledContracts() throws IOException { + List result = new ArrayList<>(); + for (String fileName : manifestContractNames()) { + JSONObject contract = loadContractFile(CONTRACT_DIR + "/" + fileName); + String contractSchedule = contract.optString("schedule", "pr"); + if ("pr".equals(schedule) && !"pr".equals(contractSchedule)) { + continue; // PR runs only PR-scheduled contracts; nightly runs all. + } + result.add(contract); + } + return result; + } + + private List manifestContractNames() throws IOException { + JSONObject manifest = loadContractFile(MANIFEST); + JSONArray contracts = manifest.getJSONArray("contracts"); + List names = new ArrayList<>(); + for (int i = 0; i < contracts.length(); i++) { + names.add(contracts.getString(i)); + } + return names; + } + + /** Union of index enums required by the contracts scheduled to run this session. */ + private Set requiredIndexEnums() throws IOException { + Set indices = new LinkedHashSet<>(); + for (JSONObject contract : loadScheduledContracts()) { + JSONObject fixture = contract.optJSONObject("backendFixture"); + if (fixture == null) { + continue; + } + JSONArray declared = fixture.optJSONArray("indices"); + if (declared == null) { + continue; + } + for (int i = 0; i < declared.length(); i++) { + indices.add(declared.getString(i)); + } + } + if (indices.isEmpty()) { + indices.add("ACCOUNT"); + } + return indices; + } + + private JSONObject loadContractFile(String resourcePath) throws IOException { + String path = TestUtils.getResourceFilePath(resourcePath); return new JSONObject(new String(Files.readAllBytes(Paths.get(path)))); } + + // --- reporting ------------------------------------------------------------- + + private JSONObject reportEntry(String ruleId, String caseId, String query, String kind) { + return new JSONObject() + .put("ruleId", ruleId) + .put("caseId", caseId) + .put("query", query) + .put("kind", kind); + } + + private void writeReport(JSONArray report) { + String target = System.getProperty("ppl.lint.report"); + if (target == null || target.isEmpty()) { + return; + } + try { + Files.write(Paths.get(target), report.toString(2).getBytes()); + } catch (IOException e) { + System.err.println("[ppl-lint] could not write backend report to " + target + ": " + e); + } + } + + private void log(String ruleId, String caseId, String message) { + System.out.println( + String.format( + Locale.ROOT, "[ppl-lint-backend-contract] %s/%s: %s", ruleId, caseId, message)); + } } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json new file mode 100644 index 00000000000..4c292860f6c --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json @@ -0,0 +1,42 @@ +{ + "schemaVersion": 2, + "ruleId": "dedup-consecutive-unsupported", + "oracleClass": "advisory", + "grammarSurface": "compiled-simplified", + "schedule": "nightly", + "wiring": { + "detector": "dedup-consecutive-unsupported", + "enabled": true, + "severity": "warning", + "runtimeOnly": false, + "needsContext": false, + "needsExplain": false, + "appliesTo": { "minVersion": "3.3.0", "engine": "calcite" } + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": true } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "dedup-consecutive-true", + "query": "source={{index}} | dedup firstname consecutive=true", + "minVersionRequired": "3.3.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 1, "severity": "warning" }, + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + }, + { + "id": "dedup-plain-control", + "query": "source={{index}} | dedup firstname", + "minVersionRequired": "3.3.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json new file mode 100644 index 00000000000..3e7c7302f77 --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json @@ -0,0 +1,52 @@ +{ + "schemaVersion": 2, + "ruleId": "disabled-join-type", + "oracleClass": "rejection", + "grammarSurface": "compiled-simplified", + "schedule": "pr", + "wiring": { + "detector": "disabled-join-type", + "enabled": true, + "severity": "warning", + "runtimeOnly": false, + "needsContext": false, + "needsExplain": false, + "appliesTo": {} + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false, "allJoinTypesAllowed": false } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "right-join-disabled", + "query": "source={{index}} | right join left=l right=r on l.account_number=r.account_number {{index}}", + "frontend": { "diagnosticCount": 1, "severity": "warning" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "SemanticCheckException", "reason": "Invalid Query" } } + } + }, + { + "id": "cross-join-disabled", + "query": "source={{index}} | cross join left=l right=r on l.account_number=r.account_number {{index}}", + "frontend": { "diagnosticCount": 1, "severity": "warning" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "SemanticCheckException", "reason": "Invalid Query" } } + } + }, + { + "id": "inner-join-control", + "query": "source={{index}} | join left=l right=r on l.account_number=r.account_number {{index}} | head 1", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json new file mode 100644 index 00000000000..514476d746a --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json @@ -0,0 +1,44 @@ +{ + "schemaVersion": 2, + "ruleId": "division-by-zero", + "oracleClass": "result-shape", + "grammarSurface": "compiled-simplified", + "schedule": "pr", + "wiring": { + "detector": "division-by-zero", + "enabled": true, + "severity": "warning", + "runtimeOnly": false, + "needsContext": false, + "needsExplain": false, + "appliesTo": {} + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "divide-by-zero-literal", + "query": "source={{index}} | eval ratio = balance / 0 | fields ratio | head 1", + "frontend": { "diagnosticCount": 1, "severity": "warning" }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "columnAllNull": "ratio" } } + }, + { + "id": "divide-by-nonzero-control", + "query": "source={{index}} | eval ratio = balance / 2 | fields ratio | head 1", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + }, + { + "id": "modulo-by-zero-flagged", + "query": "source={{index}} | eval m = balance % 0 | fields m | head 1", + "frontend": { "diagnosticCount": 1, "severity": "warning" }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "columnAllNull": "m" } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json new file mode 100644 index 00000000000..e383f773bb9 --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json @@ -0,0 +1,66 @@ +{ + "schemaVersion": 2, + "ruleId": "field-validation", + "oracleClass": "rejection", + "grammarSurface": "compiled-simplified", + "schedule": "pr", + "wiring": { + "detector": "field-validation", + "enabled": true, + "severity": "error", + "runtimeOnly": false, + "needsContext": false, + "needsExplain": false, + "appliesTo": {} + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false } + }, + "frontendContext": { + "isCalcite": true, + "visibleIndices": ["{{index}}"], + "deriveFromMapping": { + "account_number": "long", + "balance": "long", + "age": "long", + "firstname": "text", + "lastname": "text", + "gender": "text", + "address": "text", + "employer": "text", + "email": "text", + "city": "text", + "state": "text" + } + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "unknown-field-existence", + "query": "source={{index}} | where nonexistent_field > 3", + "frontend": { "diagnosticCount": 1, "severity": "error", "matchMessage": "nonexistent_field" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Field [nonexistent_field] not found." } } + } + }, + { + "id": "grok-field-slot-shape-typo", + "query": "source={{index}} | grok field=firstname \"%{WORD:w}\"", + "frontend": { "diagnosticCount": 1, "severity": "error" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Field [field] not found." } } + } + }, + { + "id": "known-field-control", + "query": "source={{index}} | where age > 30 | head 1", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json new file mode 100644 index 00000000000..1d800dfcd6d --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json @@ -0,0 +1,38 @@ +{ + "schemaVersion": 2, + "ruleId": "head-without-sort", + "oracleClass": "advisory", + "grammarSurface": "compiled-simplified", + "schedule": "pr", + "wiring": { + "detector": "head-without-sort", + "enabled": true, + "severity": "info", + "runtimeOnly": false, + "needsContext": false, + "needsExplain": false, + "appliesTo": {} + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "head-without-sort", + "query": "source={{index}} | head 5", + "frontend": { "diagnosticCount": 1, "severity": "info" }, + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + }, + { + "id": "head-with-sort-control", + "query": "source={{index}} | sort age | head 5", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/manifest.json b/integ-test/src/test/resources/ppl-lint/contracts/manifest.json new file mode 100644 index 00000000000..31b54ecf18b --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/manifest.json @@ -0,0 +1,15 @@ +{ + "schemaVersion": 2, + "description": "Index of PPL lint rule validation contracts. Each entry pins one OSD analyzer rule to live SQL /_plugins/_ppl behavior. The frontend adapter (scripts/ppl-lint/run-frontend-contract.mjs) and backend IT (PplLintRuleValidationIT) both read these files.", + "contracts": [ + "unsupported-window-function-in-eventstats.spec.json", + "division-by-zero.spec.json", + "head-without-sort.spec.json", + "disabled-join-type.spec.json", + "field-validation.spec.json", + "dedup-consecutive-unsupported.spec.json", + "multisearch-min-subsearch.spec.json", + "union-min-datasets.spec.json", + "replace-wildcard-asymmetry.spec.json" + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json new file mode 100644 index 00000000000..5d64a7ba674 --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json @@ -0,0 +1,47 @@ +{ + "schemaVersion": 2, + "ruleId": "multisearch-min-subsearch", + "oracleClass": "rejection", + "grammarSurface": "runtime-bundle", + "schedule": "nightly", + "requiredParserRules": ["multisearchCommand", "subSearch"], + "wiring": { + "detector": "multisearch-min-subsearch", + "enabled": true, + "severity": "error", + "runtimeOnly": true, + "needsContext": false, + "needsExplain": false, + "appliesTo": { "minVersion": "3.4.0" } + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "multisearch-single-subsearch", + "query": "| multisearch [ search source={{index}} ]", + "minVersionRequired": "3.4.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 1, "severity": "error" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "SyntaxCheckException", "reason": "Invalid Query" } } + } + }, + { + "id": "multisearch-two-subsearches-control", + "query": "| multisearch [ search source={{index}} ] [ search source={{index}} ]", + "minVersionRequired": "3.4.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json new file mode 100644 index 00000000000..36a0231c423 --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json @@ -0,0 +1,53 @@ +{ + "schemaVersion": 2, + "ruleId": "replace-wildcard-asymmetry", + "oracleClass": "rejection", + "grammarSurface": "runtime-bundle", + "schedule": "nightly", + "requiredParserRules": ["replacePair", "stringLiteral"], + "wiring": { + "detector": "replace-wildcard-asymmetry", + "enabled": true, + "severity": "error", + "runtimeOnly": true, + "needsContext": false, + "needsExplain": false, + "appliesTo": { "minVersion": "3.4.0", "engine": "calcite" } + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "replace-wildcard-count-mismatch", + "query": "source={{index}} | replace \"*_a\" with \"b_*_*\" in firstname", + "minVersionRequired": "3.4.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 1, "severity": "error" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { + "status": 400, + "error": { + "type": "IllegalArgumentException", + "reason": "Error in 'replace' command: Wildcard count mismatch - pattern has 1 wildcard(s), replacement has 2. Replacement must have same number of wildcards or none." + } + } + } + }, + { + "id": "replace-symmetric-control", + "query": "source={{index}} | replace \"*_a\" with \"b_*\" in firstname | head 1", + "minVersionRequired": "3.4.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json new file mode 100644 index 00000000000..eb13103a37d --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json @@ -0,0 +1,47 @@ +{ + "schemaVersion": 2, + "ruleId": "union-min-datasets", + "oracleClass": "rejection", + "grammarSurface": "runtime-bundle", + "schedule": "nightly", + "requiredParserRules": ["unionCommand", "unionDataset", "pplCommands"], + "wiring": { + "detector": "union-min-datasets", + "enabled": true, + "severity": "error", + "runtimeOnly": true, + "needsContext": false, + "needsExplain": false, + "appliesTo": { "minVersion": "3.7.0", "engine": "calcite" } + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "union-single-dataset", + "query": "| union [ source={{index}} ]", + "minVersionRequired": "3.7.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 1, "severity": "error" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Union command requires at least two datasets. Provided: 1" } } + } + }, + { + "id": "union-two-datasets-control", + "query": "| union [ source={{index}} ] [ source={{index}} ]", + "minVersionRequired": "3.7.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json new file mode 100644 index 00000000000..8ad00479144 --- /dev/null +++ b/integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json @@ -0,0 +1,49 @@ +{ + "schemaVersion": 2, + "ruleId": "unsupported-window-function-in-eventstats", + "oracleClass": "rejection", + "grammarSurface": "compiled-simplified", + "schedule": "pr", + "wiring": { + "detector": "unsupported-window-function-in-eventstats", + "enabled": true, + "severity": "error", + "runtimeOnly": false, + "needsContext": false, + "needsExplain": false, + "appliesTo": { "minVersion": "3.4.0" } + }, + "backendFixture": { + "indices": ["ACCOUNT"], + "clusterSettings": { "calcite": true, "calciteFallback": false } + }, + "frontendContext": { + "isCalcite": true + }, + "index": "opensearch-sql_test_index_account", + "cases": [ + { + "id": "eventstats-rank", + "query": "source={{index}} | eventstats rank() as rank_value", + "minVersionRequired": "3.4.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 1, "severity": "error" }, + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { + "status": 400, + "error": { "type": "CalciteUnsupportedException", "reason": "Unexpected window function: rank" } + } + } + }, + { + "id": "eventstats-avg-control", + "query": "source={{index}} | eventstats avg(age) as avg_age", + "minVersionRequired": "3.4.0", + "engineRequired": "calcite", + "frontend": { "diagnosticCount": 0 }, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + ] +} diff --git a/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json b/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json deleted file mode 100644 index 02fbab79a3c..00000000000 --- a/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json +++ /dev/null @@ -1,29 +0,0 @@ -{ - "ruleId": "unsupported-window-function-in-eventstats", - "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "eventstats-rank", - "query": "source={{index}} | eventstats rank() as rank_value", - "frontendDiagnosticCount": 1, - "backendExpected": { - "httpStatus": 400, - "body": { - "status": 400, - "error": { - "type": "CalciteUnsupportedException", - "reason": "Unexpected window function: rank" - } - } - } - }, - { - "id": "eventstats-avg-control", - "query": "source={{index}} | eventstats avg(age) as avg_age", - "frontendDiagnosticCount": 0, - "backendExpected": { - "httpStatus": 200 - } - } - ] -} diff --git a/scripts/ppl-lint-rule-validation.sh b/scripts/ppl-lint-rule-validation.sh index 1552353d052..515dda9c3bf 100755 --- a/scripts/ppl-lint-rule-validation.sh +++ b/scripts/ppl-lint-rule-validation.sh @@ -24,6 +24,9 @@ # # Skip one half # SKIP_BACKEND=1 ./scripts/ppl-lint-rule-validation.sh # SKIP_FRONTEND=1 ./scripts/ppl-lint-rule-validation.sh +# +# # Run the full nightly corpus (runtime-only + advisory rules + coverage) +# PPL_LINT_SCHEDULE=nightly ./scripts/ppl-lint-rule-validation.sh set -euo pipefail @@ -33,9 +36,11 @@ cd "$SQL_ROOT" OSD_REPO_URL="${OSD_REPO_URL:-https://github.com/opensearch-project/OpenSearch-Dashboards.git}" OSD_REF="${OSD_REF:-main}" DEFAULT_OSD_CHECKOUT="$SQL_ROOT/.ci/OpenSearch-Dashboards" -CONTRACT_FILE="$SQL_ROOT/integ-test/src/test/resources/ppl-lint/unsupported-window-function-in-eventstats.spec.json" +CONTRACT_DIR="$SQL_ROOT/integ-test/src/test/resources/ppl-lint/contracts" FRONTEND_SCRIPT="$SQL_ROOT/scripts/ppl-lint/run-frontend-contract.mjs" IT_CLASS="org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" +# pr (fast, blocking subset) or nightly (full corpus + coverage assertion). +PPL_LINT_SCHEDULE="${PPL_LINT_SCHEDULE:-pr}" log() { echo "[ppl-lint-rule-validation] $*"; } @@ -58,11 +63,13 @@ run_frontend() { local os_version os_version="$(resolve_opensearch_version)" - log "Running frontend contract against OSD analyzer (PPL_SQL_VERSION=$os_version)..." + log "Running frontend contract against OSD analyzer (PPL_SQL_VERSION=$os_version, schedule=$PPL_LINT_SCHEDULE)..." ( cd "$osd_checkout" - PPL_LINT_CONTRACT_FILE="$CONTRACT_FILE" \ + PPL_LINT_CONTRACT_DIR="$CONTRACT_DIR" \ + PPL_LINT_SCHEDULE="$PPL_LINT_SCHEDULE" \ PPL_SQL_VERSION="$os_version" \ + PPL_LINT_REPORT="$SQL_ROOT/frontend-report.json" \ node -r ./src/setup_node_env "$FRONTEND_SCRIPT" ) } @@ -95,8 +102,10 @@ else fi if [[ "${SKIP_BACKEND:-0}" != "1" ]]; then - log "Running backend integration test: $IT_CLASS" - ./gradlew :integ-test:integTest --tests "$IT_CLASS" + log "Running backend integration test: $IT_CLASS (schedule=$PPL_LINT_SCHEDULE)" + ./gradlew :integ-test:integTest --tests "$IT_CLASS" \ + -Dppl.lint.schedule="$PPL_LINT_SCHEDULE" \ + -Dppl.lint.report="$SQL_ROOT/backend-report.json" log "Backend integration test passed." else log "SKIP_BACKEND=1 — skipping the SQL backend integration test." diff --git a/scripts/ppl-lint/run-frontend-contract.mjs b/scripts/ppl-lint/run-frontend-contract.mjs index 603e3feab6c..e3163e2ad44 100644 --- a/scripts/ppl-lint/run-frontend-contract.mjs +++ b/scripts/ppl-lint/run-frontend-contract.mjs @@ -10,8 +10,10 @@ * for example: * * cd .ci/OpenSearch-Dashboards - * PPL_LINT_CONTRACT_FILE= \ + * PPL_LINT_CONTRACT_DIR= \ + * PPL_LINT_SCHEDULE=pr \ * PPL_SQL_VERSION= \ + * PPL_LINT_REPORT= \ * node -r ./src/setup_node_env \ * "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" * @@ -26,9 +28,18 @@ * Because this is an ES module, `require` is obtained with `createRequire`, and * the modules are resolved against the OSD checkout root (process.cwd()) rather * than the location of this script (which lives in the SQL repo, not OSD). + * + * This adapter is the frontend half of a schema-v2 cross-repository differential + * contract (see integ-test/src/test/resources/ppl-lint/contracts/*.spec.json). + * It asserts three things per rule: + * 1. Wiring: the OSD catalog entry deep-equals the contract's `wiring` block, + * so a silently removed/retyped/regated detector reds the build. + * 2. Diagnostics: for each case the analyzer emits exactly the contracted + * number of `ruleId` diagnostics (the differential the backend half pins to + * live-engine behavior). + * 3. Coverage (nightly only): every enabled catalog rule has a contract file. */ -import assert from 'assert'; import fs from 'fs'; import path from 'path'; import { createRequire } from 'module'; @@ -36,129 +47,444 @@ import { createRequire } from 'module'; const RULE_MODULE = 'packages/osd-monaco/src/ppl/ppl_language_analyzer'; const CATALOG_MODULE = 'packages/osd-monaco/src/ppl/lint/catalog'; const DETECTOR_REGISTRY_MODULE = 'packages/osd-monaco/src/ppl/lint/detector_registry'; +const LINT_RUNNER_MODULE = 'packages/osd-monaco/src/ppl/lint/lint_runner'; +const RULE_INDEX_MODULE = 'packages/osd-monaco/src/ppl/lint/rule_index'; +const GRAMMAR_MODULE = 'packages/osd-antlr-grammar/target/index.js'; +// Explain lint lives only on OSD branches that ship the explain rule class; the +// adapter feature-detects it and skips explain cases when it is absent. +const RUN_EXPLAIN_MODULE = 'packages/osd-monaco/src/ppl/lint/explain/run_explain_lint'; + +function log(message) { + // eslint-disable-next-line no-console + console.log(`[ppl-lint-frontend-contract] ${message}`); +} -function fail(message) { +function fatal(message) { // eslint-disable-next-line no-console - console.error(`[ppl-lint-frontend-contract] FAIL: ${message}`); - process.exit(1); + console.error(`[ppl-lint-frontend-contract] FATAL: ${message}`); + process.exit(2); } -function loadContract() { - const contractFile = process.env.PPL_LINT_CONTRACT_FILE; - if (!contractFile) { - fail('PPL_LINT_CONTRACT_FILE is not set.'); +/** Load every *.spec.json under the contract dir, honoring manifest.json if present. */ +function loadContracts() { + const dir = process.env.PPL_LINT_CONTRACT_DIR; + const single = process.env.PPL_LINT_CONTRACT_FILE; + + if (single) { + if (!fs.existsSync(single)) { + fatal(`Contract file not found: ${single}`); + } + return [{ file: single, spec: JSON.parse(fs.readFileSync(single, 'utf8')) }]; } - if (!fs.existsSync(contractFile)) { - fail(`Contract file not found: ${contractFile}`); + + if (!dir) { + fatal('Set PPL_LINT_CONTRACT_DIR (a directory of *.spec.json) or PPL_LINT_CONTRACT_FILE.'); } - try { - return JSON.parse(fs.readFileSync(contractFile, 'utf8')); - } catch (error) { - fail(`Could not parse contract file ${contractFile}: ${error.message}`); - return undefined; // unreachable + if (!fs.existsSync(dir)) { + fatal(`Contract directory not found: ${dir}`); + } + + const manifestPath = path.join(dir, 'manifest.json'); + let files; + if (fs.existsSync(manifestPath)) { + const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); + if (!Array.isArray(manifest.contracts)) { + fatal(`manifest.json must have a "contracts" array of file names.`); + } + files = manifest.contracts.map((name) => path.join(dir, name)); + } else { + files = fs + .readdirSync(dir) + .filter((f) => f.endsWith('.spec.json')) + .sort() + .map((f) => path.join(dir, f)); } + + return files.map((file) => { + if (!fs.existsSync(file)) { + fatal(`Contract referenced by manifest not found: ${file}`); + } + return { file, spec: JSON.parse(fs.readFileSync(file, 'utf8')) }; + }); } -function loadOsdAnalyzer() { - // Resolve the OSD compiled analyzer from the checkout root. In CI this script - // runs after `cd .ci/OpenSearch-Dashboards`, so process.cwd() is that root. +function loadOsd() { const osdRoot = process.cwd(); const require = createRequire(path.join(osdRoot, 'noop.js')); - const resolveOsd = (relativeModule) => { + const resolveOsd = (relativeModule, { optional = false } = {}) => { const absolute = path.join(osdRoot, relativeModule); - if (!fs.existsSync(`${absolute}.ts`) && !fs.existsSync(`${absolute}.js`)) { - fail( + const exists = + fs.existsSync(absolute) || + fs.existsSync(`${absolute}.ts`) || + fs.existsSync(`${absolute}.js`); + if (!exists) { + if (optional) { + return undefined; + } + fatal( `Expected OSD module not found under the checkout root: ${relativeModule}\n` + `Resolved OSD root: ${osdRoot}\n` + `Run this script from the OSD checkout (e.g. cd .ci/OpenSearch-Dashboards) after bootstrap.` ); } - return require(absolute); + try { + return require(absolute); + } catch (error) { + if (optional) { + return undefined; + } + throw error; + } }; const { PPLLanguageAnalyzer } = resolveOsd(RULE_MODULE); const { getBundledCatalog } = resolveOsd(CATALOG_MODULE); const { getDetector } = resolveOsd(DETECTOR_REGISTRY_MODULE); + const { runLint } = resolveOsd(LINT_RUNNER_MODULE); + const ruleIndex = resolveOsd(RULE_INDEX_MODULE); + const grammar = resolveOsd(GRAMMAR_MODULE, { optional: true }); + const explain = resolveOsd(RUN_EXPLAIN_MODULE, { optional: true }); if (typeof PPLLanguageAnalyzer !== 'function') { - fail(`PPLLanguageAnalyzer was not a constructor when loaded from ${RULE_MODULE}.`); + fatal(`PPLLanguageAnalyzer was not a constructor when loaded from ${RULE_MODULE}.`); + } + + return { PPLLanguageAnalyzer, getBundledCatalog, getDetector, runLint, ruleIndex, grammar, explain, osdRoot }; +} + +/** Coerce "3.8.0-SNAPSHOT" / "3.8" to a comparable [major, minor, patch]. */ +function parseVersion(v) { + if (!v) return undefined; + const m = /^(\d+)(?:\.(\d+))?(?:\.(\d+))?/.exec(String(v)); + if (!m) return undefined; + return [Number(m[1]), Number(m[2] || 0), Number(m[3] || 0)]; +} + +function versionGte(a, b) { + const pa = parseVersion(a); + const pb = parseVersion(b); + if (!pa || !pb) return true; // unknown → do not skip + for (let i = 0; i < 3; i++) { + if (pa[i] > pb[i]) return true; + if (pa[i] < pb[i]) return false; } - return { PPLLanguageAnalyzer, getBundledCatalog, getDetector, osdRoot }; + return true; } -function assertRuleIsWiredUp(ruleId, getBundledCatalog, getDetector) { - const entry = getBundledCatalog().find((candidate) => candidate.id === ruleId); +/** + * Assert the OSD catalog entry deep-equals the contract's `wiring` block. This is + * the primary OSD-drift tripwire: if a detector is removed, retyped, re-gated or + * its severity changed, this fails before any query runs. + */ +function checkWiring(spec, catalog, getDetector, failures) { + const { ruleId, wiring } = spec; + const entry = catalog.find((c) => c.id === ruleId); if (!entry) { - fail(`Rule "${ruleId}" is not present in the OSD bundled catalog.`); + failures.push(`[${ruleId}] not present in the OSD bundled catalog.`); + return undefined; } - if (!entry.enabled) { - fail(`Rule "${ruleId}" is present but disabled in the OSD bundled catalog.`); + if (!wiring) { + return entry; // no wiring block to assert } - if (entry.severity !== 'error') { - fail(`Rule "${ruleId}" severity is "${entry.severity}", expected "error".`); + + const checks = [ + ['detector', wiring.detector, entry.detector], + ['enabled', wiring.enabled, entry.enabled], + ['severity', wiring.severity, entry.severity], + ['runtimeOnly', !!wiring.runtimeOnly, !!entry.runtimeOnly], + ['needsContext', !!wiring.needsContext, !!entry.needsContext], + ['needsExplain', !!wiring.needsExplain, !!entry.needsExplain], + ]; + for (const [name, expected, actual] of checks) { + if (expected !== undefined && expected !== actual) { + failures.push(`[${ruleId}] wiring.${name} expected ${JSON.stringify(expected)} but catalog has ${JSON.stringify(actual)}.`); + } + } + + if (wiring.appliesTo) { + const a = entry.appliesTo || {}; + for (const key of ['minVersion', 'maxVersion', 'engine']) { + if (wiring.appliesTo[key] !== undefined && wiring.appliesTo[key] !== a[key]) { + failures.push(`[${ruleId}] wiring.appliesTo.${key} expected ${JSON.stringify(wiring.appliesTo[key])} but catalog has ${JSON.stringify(a[key])}.`); + } + } } - if (typeof getDetector(entry.detector) !== 'function') { - fail(`Rule "${ruleId}" has no registered detector "${entry.detector}".`); + + if (wiring.detector && typeof getDetector(wiring.detector) !== 'function') { + failures.push(`[${ruleId}] has no registered detector "${wiring.detector}".`); } + return entry; } -function main() { - const contract = loadContract(); - const ruleId = contract.ruleId; - const index = contract.index; - const sqlVersion = process.env.PPL_SQL_VERSION; +/** + * Build the per-case lint context. Derives `fields`/`typeMap` from the + * `deriveFromMapping` block (a single source shared with the backend seeding), + * and sets an enable override for default-off rules that declare `forceEnable`. + */ +function buildContext(spec, sqlVersion) { + const fc = spec.frontendContext || {}; + const context = { + isCalcite: fc.isCalcite !== false, + dataSourceVersion: sqlVersion, + grammarSurface: spec.grammarSurface === 'runtime-bundle' ? 'runtime-bundle' : 'compiled-simplified', + }; - const { PPLLanguageAnalyzer, getBundledCatalog, getDetector, osdRoot } = loadOsdAnalyzer(); + const mapping = fc.deriveFromMapping; + if (mapping && typeof mapping === 'object') { + const fields = new Set(); + const typeMap = new Map(); + for (const [name, type] of Object.entries(mapping)) { + fields.add(name); + typeMap.set(name, type); + } + context.fields = fields; + context.typeMap = typeMap; + } + if (Array.isArray(fc.disabledObjectFields) && fc.disabledObjectFields.length > 0) { + context.disabledObjectFields = new Set(fc.disabledObjectFields); + } + if (Array.isArray(fc.visibleIndices) && fc.visibleIndices.length > 0) { + context.visibleIndices = fc.visibleIndices.map((i) => i.split('{{index}}').join(spec.index)); + } + if (fc.settings && typeof fc.settings === 'object') { + context.settings = fc.settings; + } + if (fc.forceEnable) { + context.overrides = { [spec.ruleId]: { enabled: true } }; + } + return context; +} - const entry = assertRuleIsWiredUp(ruleId, getBundledCatalog, getDetector); +/** Count diagnostics for this rule via the compiled-simplified analyzer. */ +function lintCompiled(analyzer, query, context, ruleId) { + const result = analyzer.lint(query, context); + return result.diagnostics.filter((d) => d.ruleId === ruleId); +} - // eslint-disable-next-line no-console - console.log( - `[ppl-lint-frontend-contract] OSD root: ${osdRoot}\n` + - `[ppl-lint-frontend-contract] rule "${ruleId}" enabled=${entry.enabled} severity=${entry.severity} detector="${entry.detector}"\n` + - `[ppl-lint-frontend-contract] PPL_SQL_VERSION=${sqlVersion || '(unset)'}\n` + - `[ppl-lint-frontend-contract] running ${contract.cases.length} case(s) against index "${index}"` - ); +/** + * Count diagnostics for a runtime-only rule by parsing with the exported runtime + * grammar and running the detector registry directly. This exercises OSD-main's + * runtime grammar, NOT the cluster-versioned bundle production fetches, so it is + * a wiring/coverage check rather than a true cluster-grammar fidelity check. + * Returns undefined when the runtime grammar can't reach the rule on this OSD + * checkout (the rule's parser rules are absent) so the caller can skip cleanly. + */ +function lintRuntime(osd, spec, query, context, ruleId) { + const { grammar, runLint, ruleIndex } = osd; + if (!grammar || !grammar.OpenSearchPPLParser || !grammar.OpenSearchPPLLexer) { + return undefined; + } + const antlr = requireAntlr(osd.osdRoot); + if (!antlr) { + return undefined; + } + const { OpenSearchPPLLexer, OpenSearchPPLParser } = grammar; + const runtimeMap = new Map(); + const names = OpenSearchPPLParser.ruleNames || []; + for (let i = 0; i < names.length; i++) { + runtimeMap.set(names[i], i); + } + + // The exported runtime grammar on this OSD checkout may predate the command a + // runtime-only rule keys off (union/multisearch/replace are absent on the + // legacy `opensearch_ppl` grammar). Detecting the absence here lets the caller + // record a clean skip — the wiring assertion already ran — instead of a false + // "0 diagnostics" failure. + const required = spec.requiredParserRules || []; + for (const name of required) { + if (!runtimeMap.has(name)) { + return undefined; + } + } + + const input = antlr.CharStream.fromString(query); + const lexer = new OpenSearchPPLLexer(input); + const tokenStream = new antlr.CommonTokenStream(lexer); + const parser = new OpenSearchPPLParser(tokenStream); + parser.removeErrorListeners(); + const tree = parser.root ? parser.root() : parser.pplStatement && parser.pplStatement(); + if (!tree) { + return undefined; + } + + const ruleNameToIndex = ruleIndex.createRuntimeRuleNameToIndex(runtimeMap); + + const diagnostics = runLint(tree, { + ruleNameToIndex, + dataSourceVersion: context.dataSourceVersion, + context: { ...context, grammarSurface: 'runtime-bundle' }, + }); + return diagnostics.filter((d) => d.ruleId === ruleId); +} + +let cachedAntlr; +function requireAntlr(osdRoot) { + if (cachedAntlr !== undefined) { + return cachedAntlr || undefined; + } + try { + const require = createRequire(path.join(osdRoot, 'noop.js')); + cachedAntlr = require('antlr4ng'); + } catch { + cachedAntlr = null; + } + return cachedAntlr || undefined; +} + +function main() { + const schedule = process.env.PPL_LINT_SCHEDULE || 'pr'; + const sqlVersion = process.env.PPL_SQL_VERSION; + const reportPath = process.env.PPL_LINT_REPORT; + + const osd = loadOsd(); + const { PPLLanguageAnalyzer, getBundledCatalog, getDetector, osdRoot } = osd; + const catalog = getBundledCatalog(); const analyzer = new PPLLanguageAnalyzer(); + + const contracts = loadContracts(); const failures = []; + const report = { osdRoot, schedule, sqlVersion, results: [] }; - for (const testCase of contract.cases) { - const query = testCase.query.split('{{index}}').join(index); - const result = analyzer.lint(query, { - dataSourceVersion: sqlVersion, - isCalcite: true, - }); - const matches = result.diagnostics.filter((diagnostic) => diagnostic.ruleId === ruleId); + log(`OSD root: ${osdRoot}`); + log(`schedule=${schedule} PPL_SQL_VERSION=${sqlVersion || '(unset)'} contracts=${contracts.length}`); - // eslint-disable-next-line no-console - console.log( - `[ppl-lint-frontend-contract] ${testCase.id}: expected ${testCase.frontendDiagnosticCount}, ` + - `got ${matches.length} — ${query}` - ); + for (const { file, spec } of contracts) { + const ruleId = spec.ruleId; + const index = spec.index; + + // A contract runs on PR only when scheduled for PR; nightly runs everything. + const contractSchedule = spec.schedule || 'pr'; + if (schedule === 'pr' && contractSchedule !== 'pr') { + log(`SKIP ${ruleId} (schedule=${contractSchedule}, running ${schedule}) — ${path.basename(file)}`); + continue; + } + + const entry = checkWiring(spec, catalog, getDetector, failures); + if (!entry) { + continue; + } + const context = buildContext(spec, sqlVersion); + const isRuntime = context.grammarSurface === 'runtime-bundle'; + + for (const testCase of spec.cases || []) { + const query = testCase.query.split('{{index}}').join(index); + const fe = testCase.frontend || {}; + const expected = fe.diagnosticCount; + + // Per-case version/engine gate mirrors the backend so both halves skip + // identically instead of disagreeing on a self-suppressed rule. + if (testCase.minVersionRequired && !versionGte(sqlVersion, testCase.minVersionRequired)) { + log(`SKIP ${ruleId}/${testCase.id} (needs >= ${testCase.minVersionRequired}, have ${sqlVersion || 'unknown'})`); + continue; + } + if (testCase.engineRequired === 'calcite' && context.isCalcite !== true) { + log(`SKIP ${ruleId}/${testCase.id} (needs calcite engine)`); + continue; + } + + let matches; + if (testCase.explainFixture) { + matches = lintExplain(osd, spec, testCase, context, ruleId); + if (matches === undefined) { + log(`SKIP ${ruleId}/${testCase.id} (explain lint unavailable on this OSD checkout)`); + continue; + } + } else if (isRuntime) { + matches = lintRuntime(osd, spec, query, context, ruleId); + if (matches === undefined) { + // Runtime grammar can't reach this rule on this OSD checkout: the + // wiring assertion above still ran, so record a skip (not a failure). + log(`SKIP ${ruleId}/${testCase.id} (runtime grammar rule absent on this OSD checkout; wiring asserted)`); + report.results.push({ ruleId, caseId: testCase.id, query, expected, actual: null, skipped: 'runtime-grammar-absent' }); + continue; + } + } else { + matches = lintCompiled(analyzer, query, context, ruleId); + } + + const actual = matches.length; + const ok = actual === expected; + + log(` ${ok ? 'PASS' : 'FAIL'} ${ruleId}/${testCase.id}: expected ${expected}, got ${actual} — ${query}`); + + const severityOk = + !fe.severity || actual === 0 || matches.every((m) => m.severity === fe.severity); + const messageOk = + !fe.matchMessage || matches.some((m) => (m.message || '').includes(fe.matchMessage)); + + report.results.push({ ruleId, caseId: testCase.id, query, expected, actual, severities: matches.map((m) => m.severity) }); + + if (!ok) { + failures.push(`[${ruleId}/${testCase.id}] expected ${expected} "${ruleId}" diagnostic(s), got ${actual} for: ${query}`); + } + if (!severityOk) { + failures.push(`[${ruleId}/${testCase.id}] expected severity "${fe.severity}" for: ${query}`); + } + if (!messageOk) { + failures.push(`[${ruleId}/${testCase.id}] expected message to contain "${fe.matchMessage}" for: ${query}`); + } + } + } + + // Nightly-only coverage: every enabled catalog rule must have a contract file. + if (schedule === 'nightly') { + const covered = new Set(contracts.map(({ spec }) => spec.ruleId)); + for (const rule of catalog) { + if (rule.enabled && !covered.has(rule.id)) { + failures.push(`[coverage] enabled catalog rule "${rule.id}" has no contract file.`); + } + } + } + + if (reportPath) { + report.failures = failures; try { - assert.strictEqual( - matches.length, - testCase.frontendDiagnosticCount, - `case "${testCase.id}": expected ${testCase.frontendDiagnosticCount} "${ruleId}" ` + - `diagnostic(s) but received ${matches.length} for query: ${query}` - ); + fs.writeFileSync(reportPath, JSON.stringify(report, null, 2)); + log(`wrote report to ${reportPath}`); } catch (error) { - failures.push(error.message); + log(`WARN: could not write report to ${reportPath}: ${error.message}`); } } if (failures.length > 0) { - fail(`${failures.length} case(s) failed:\n- ${failures.join('\n- ')}`); + // eslint-disable-next-line no-console + console.error(`[ppl-lint-frontend-contract] FAIL: ${failures.length} problem(s):\n- ${failures.join('\n- ')}`); + process.exit(1); } - // eslint-disable-next-line no-console - console.log( - `[ppl-lint-frontend-contract] PASS: all ${contract.cases.length} case(s) matched for "${ruleId}".` - ); + log(`PASS: all contracts agreed with the OSD analyzer (schedule=${schedule}).`); +} + +/** + * Explain-case handling. Loads the captured plan fixture and runs the OSD explain + * lint over it. Returns undefined when the explain rule class is not present on + * this OSD checkout (feature-detected via the optional module). + */ +function lintExplain(osd, spec, testCase, context, ruleId) { + if (!osd.explain || typeof osd.explain.runExplainLint !== 'function') { + return undefined; + } + const dir = process.env.PPL_LINT_CONTRACT_DIR; + if (!dir) { + return undefined; + } + const fixturePath = path.join(dir, testCase.explainFixture); + if (!fs.existsSync(fixturePath)) { + return undefined; + } + const plan = JSON.parse(fs.readFileSync(fixturePath, 'utf8')); + const query = testCase.query.split('{{index}}').join(spec.index); + const diagnostics = osd.explain.runExplainLint(plan, { + query, + overrides: context.overrides, + dataSourceVersion: context.dataSourceVersion, + isCalcite: context.isCalcite, + }); + return (diagnostics || []).filter((d) => d.ruleId === ruleId); } main(); From 5b03f4fdf58f24c20f2b015b58bf32d73104ff8e Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Mon, 20 Jul 2026 15:46:42 -0700 Subject: [PATCH 06/10] feat(ci): reconnect PPL lint validation to candidate runtime grammar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Take the cross-repository PPL lint check from the PoC to the design in ppl-lint-ci-validation-design.md. The detector half now lints against the *candidate* runtime grammar bundle the SQL PR builds — through OSD's production headless lint API — instead of the compiled analyzer or a hand-rolled reparse of OSD main's checked-in grammar. Both halves validate the same grammar, so a parser/semantic change that invalidates a lint rule reds the build. SQL-side changes (the OSD headless API ships separately): - PplLintRuleValidationIT: export the candidate grammar bundle (GET /_plugins/_ppl/_grammar) + a target manifest {engineVersion, grammarHash, grammarBundle} while the cluster is alive; read schema-v3 specs; select the one expectations[] entry matching the backend version (zero/>1 fails); record the observed backend behavior per query for the differential. - integ-test/build.gradle: forward -Dppl.lint.grammar.bundle / -Dppl.lint.target to the test JVM alongside the existing ppl.lint.* knobs. - run-frontend-contract.mjs: deserialize the candidate bundle via the OSD headless API and lint each query with lintQueryWithBundle (runtime-bundle surface, so the runtime-only arity rules fire); pin dataSourceVersion + knownVersion to the candidate version; assert the detector-vs-backend differential from the backend report; fail loud on a missing bundle. - workflow: linear backend-validation -> detector-validation -> validation-result pipeline; artifacts are the only bridge between jobs. validation-result is the single always() required check (red unless both jobs succeed) and writes the per-rule PR summary; assemble-run-manifest.mjs emits run-manifest.json with the immutable SQL + OSD SHAs, mode, backend version, grammar hash, and enforced set. A workflow_dispatch osd_ref run is pre-merge evidence, not a protection result. - contracts: migrate all 9 specs to schema v3 (named queries{role,query} + version-scoped expectations[]); partition manifest.json into enforced (eventstats, multisearch, union, replace), pendingReview (field-validation), and nonEnforcing. Union/multisearch triggers are query-initial, not pipe-first: OSD prepends a synthetic source prefix to pipe-first queries, which would desync the two halves. - Add scripts/ppl-lint/README.md documenting inputs, local reproduction, the contract format, and the failure table. Verified end to end against a live cluster: the backend IT exports a real candidate bundle and the detector runner agrees on all four enforced rules (triggers rejected + 1 diagnostic, controls accepted + 0); an intentional expectation mismatch reds the runner. Signed-off-by: Hanyu Wei --- .../workflows/ppl-lint-rule-validation.yml | 282 ++++++---- .gitignore | 7 +- integ-test/build.gradle | 14 +- .../remote/PplLintRuleValidationIT.java | 414 ++++++++++---- .../dedup-consecutive-unsupported.spec.json | 40 +- .../contracts/disabled-join-type.spec.json | 66 ++- .../contracts/division-by-zero.spec.json | 50 +- .../contracts/field-validation.spec.json | 67 ++- .../contracts/head-without-sort.spec.json | 37 +- .../ppl-lint/contracts/manifest.json | 26 +- .../multisearch-min-subsearch.spec.json | 50 +- .../replace-wildcard-asymmetry.spec.json | 58 +- .../contracts/union-min-datasets.spec.json | 51 +- ...ed-window-function-in-eventstats.spec.json | 51 +- scripts/ppl-lint-rule-validation.sh | 93 ++-- scripts/ppl-lint/README.md | 195 +++++++ scripts/ppl-lint/assemble-run-manifest.mjs | 157 ++++++ scripts/ppl-lint/run-frontend-contract.mjs | 506 ++++++++++-------- 18 files changed, 1520 insertions(+), 644 deletions(-) create mode 100644 scripts/ppl-lint/README.md create mode 100644 scripts/ppl-lint/assemble-run-manifest.mjs diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index 53f4d14ca74..83fc01d0734 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -1,30 +1,40 @@ name: PPL lint rule validation -# Cross-repository check: the OpenSearch-Dashboards (OSD) PPL lint rules and the -# SQL backend must agree. A shared, reviewed corpus of contract files pins each -# rule's OSD analyzer diagnostic count to the live SQL engine's behavior, so -# neither side can drift unilaterally without a red build. +# Cross-repository check: the OpenSearch-Dashboards (OSD) PPL lint detectors and +# the SQL backend must agree on the SAME candidate runtime grammar. A shared, +# reviewed corpus of contract files pins each rule's OSD detector diagnostic +# count to the live SQL engine's behavior, so neither side can drift unilaterally +# without a red build. # -# Frontend half (frontend job): a SQL-owned Node script loads the compiled OSD -# analyzer from an OSD checkout and asserts each rule's diagnostic counts and -# catalog wiring. This runs on a bare ubuntu-latest runner because OSD requires a -# modern Node whose prebuilt binary needs a newer glibc than the OpenSearch CI -# container (Amazon Linux 2) provides. -# Backend half (backend job): a Gradle integration test sends the same queries to -# the live `/_plugins/_ppl` endpoint of the SQL plugin built from this checkout. -# This runs inside the OpenSearch CI container because the Gradle test cluster -# needs it. +# The workflow is a linear three-job pipeline (design §3.1): # -# Both jobs are required; a failure on either side fails the SQL PR check. +# backend-validation ──(artifacts)──▶ detector-validation ──▶ validation-result # -# Schedule: pull_request and workflow_dispatch run the fast, deterministic -# `schedule:pr` subset. The nightly cron runs the full corpus — runtime-only -# rules, advisory/soft-oracle rules, and a coverage assertion that every enabled -# OSD catalog rule has a contract file. +# 1. backend-validation (Amazon Linux CI container): builds the SQL PR, starts the +# Gradle test cluster, runs the contract trigger/control queries against the +# live /_plugins/_ppl endpoint, and — while the cluster is alive — exports the +# candidate runtime grammar bundle (GET /_plugins/_ppl/_grammar) plus a target +# manifest and the observed backend report. Those three files are the ONLY +# bridge to the next job; the test cluster is never passed between jobs. +# 2. detector-validation (ubuntu-latest): checks out and bootstraps OSD as a Node +# code dependency (no OSD server, no Monaco, no browser), deserializes the +# candidate bundle through OSD's production headless lint API, runs the real +# detectors against the same queries, and asserts the detector-vs-backend +# differential. OSD needs a newer Node/glibc than the CI container provides, +# hence a separate Ubuntu job. +# 3. validation-result: the single stable required check. Fails unless BOTH +# validation jobs succeeded (an always() result job so a skipped detector +# cannot mask a backend failure), writes the compact per-rule PR summary, and +# uploads the run manifest recording the exact SQL SHA, OSD SHA, mode, backend +# version, and grammar hash. # -# The OSD detector is loaded from `main` by default, so a removed or changed -# detector is caught. `workflow_dispatch` can target a specific OSD ref to -# reproduce a run or pre-validate an unmerged OSD branch. +# Modes (design §3.4, §4.1.1): +# - pull_request: SQL PR validation against OSD `main`. The ONLY enforcing mode; +# this is what branch protection pins to. Runs the fast schedule:pr subset. +# - workflow_dispatch (osd_ref): pre-merge evidence for an unmerged OSD branch. +# Records the resolved immutable OSD commit SHA but CANNOT satisfy branch +# protection — only the pull_request run does. +# - schedule (nightly): the full corpus + a coverage assertion. on: pull_request: @@ -33,7 +43,7 @@ on: workflow_dispatch: inputs: osd_ref: - description: OSD commit or branch to test instead of main + description: OSD commit or branch to validate instead of main (pre-merge evidence only) required: false type: string schedule: @@ -43,13 +53,96 @@ on: type: string jobs: - frontend: - name: Frontend contract (OSD analyzer) + Get-CI-Image-Tag: + uses: opensearch-project/opensearch-build/.github/workflows/get-ci-image-tag.yml@761e093b8c1349cc07f21c1d681d3b30bf9e1999 # main + with: + product: opensearch + + backend-validation: + name: Backend validation (live /_plugins/_ppl + grammar export) + needs: Get-CI-Image-Tag + runs-on: ubuntu-latest + container: + image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} + options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} + + steps: + - name: Run start commands + run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }} + + - name: Checkout SQL pull request + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - name: Resolve contract schedule + id: schedule + env: + REQUESTED_SCHEDULE: ${{ inputs.schedule }} + EVENT_NAME: ${{ github.event_name }} + run: | + if [ -n "$REQUESTED_SCHEDULE" ]; then + value="$REQUESTED_SCHEDULE" + elif [ "$EVENT_NAME" = "schedule" ]; then + value="nightly" + else + value="pr" + fi + echo "value=$value" >> "$GITHUB_OUTPUT" + + - name: Set up JDK 21 + uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4 + with: + distribution: 'temurin' + java-version: 21 + + # OpenSearch refuses to start as root, so run Gradle as a non-root user. The + # IT exports the candidate grammar bundle + target manifest while the cluster + # is alive; those become the artifacts the detector job lints against. + - name: Run backend integration test and export candidate grammar + run: | + chown -R 1000:1000 "$(pwd)" + su "$(id -un 1000)" -c "./gradlew :integ-test:integTest \ + --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT \ + -Dppl.lint.schedule=${{ steps.schedule.outputs.value }} \ + -Dppl.lint.report=$(pwd)/backend-report.json \ + -Dppl.lint.grammar.bundle=$(pwd)/ppl-grammar-bundle.json \ + -Dppl.lint.target=$(pwd)/target.json" + + - name: Upload backend artifacts (bundle + target + report) + if: ${{ always() }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + continue-on-error: true + with: + name: ppl-lint-backend + path: | + backend-report.json + ppl-grammar-bundle.json + target.json + + - name: Upload backend failure logs + if: ${{ failure() }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + continue-on-error: true + with: + name: ppl-lint-backend-logs + path: | + integ-test/build/reports/** + integ-test/build/testclusters/*/logs/* + + detector-validation: + name: Detector validation (OSD headless lint on candidate bundle) + needs: backend-validation runs-on: ubuntu-latest + outputs: + osd_ref: ${{ steps.osd-ref.outputs.ref }} + osd_sha: ${{ steps.osd-rev.outputs.sha }} + schedule: ${{ steps.schedule.outputs.value }} steps: - name: Checkout SQL pull request uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + # The required pull_request run always validates against OSD `main`; only a + # manual workflow_dispatch may target an unmerged OSD ref, and that run is + # pre-merge evidence, not a branch-protection result (design §4.1.1). - name: Resolve OSD ref id: osd-ref env: @@ -70,7 +163,12 @@ jobs: value="pr" fi echo "value=$value" >> "$GITHUB_OUTPUT" - echo "Contract schedule: \`$value\`" >> "$GITHUB_STEP_SUMMARY" + + - name: Download backend artifacts + uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 + with: + name: ppl-lint-backend + path: artifacts - name: Checkout OpenSearch-Dashboards uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 @@ -79,9 +177,13 @@ jobs: ref: ${{ steps.osd-ref.outputs.ref }} path: .ci/OpenSearch-Dashboards + # Resolve the (possibly mutable) ref to the immutable commit SHA actually + # tested, so the run manifest pins exactly what ran (design §4.1.1, T11). - name: Record OSD revision + id: osd-rev run: | sha=$(git -C .ci/OpenSearch-Dashboards rev-parse HEAD) + echo "sha=$sha" >> "$GITHUB_OUTPUT" echo "OSD revision: \`$sha\` (ref: ${{ steps.osd-ref.outputs.ref }})" >> "$GITHUB_STEP_SUMMARY" # Read the Node/Yarn toolchain from the OSD checkout rather than hardcoding @@ -112,103 +214,93 @@ jobs: working-directory: .ci/OpenSearch-Dashboards run: yarn osd bootstrap - # The Gradle test cluster runs the version from build.gradle's - # `opensearch.version` default (e.g. 3.8.0-SNAPSHOT). Export the release - # portion (3.8.0) as PPL_SQL_VERSION so the frontend applies the same - # version filtering the backend does, without maintaining a second string. - - name: Resolve OpenSearch version - id: os-version - run: | - raw=$(grep -oE '"opensearch.version", "[^"]+"' build.gradle | head -1 | sed -E 's/.*"opensearch.version", "([^"]+)"/\1/') - version="${raw%%-*}" - echo "version=$version" >> "$GITHUB_OUTPUT" - echo "OpenSearch version: \`$version\` (from \`$raw\`)" >> "$GITHUB_STEP_SUMMARY" - - - name: Run frontend contract + - name: Run detector validation against the candidate bundle working-directory: .ci/OpenSearch-Dashboards env: PPL_LINT_CONTRACT_DIR: ${{ github.workspace }}/integ-test/src/test/resources/ppl-lint/contracts PPL_LINT_SCHEDULE: ${{ steps.schedule.outputs.value }} - PPL_SQL_VERSION: ${{ steps.os-version.outputs.version }} - PPL_LINT_REPORT: ${{ github.workspace }}/frontend-report.json + PPL_LINT_GRAMMAR_BUNDLE: ${{ github.workspace }}/artifacts/ppl-grammar-bundle.json + PPL_LINT_TARGET_MANIFEST: ${{ github.workspace }}/artifacts/target.json + PPL_LINT_BACKEND_REPORT: ${{ github.workspace }}/artifacts/backend-report.json + PPL_LINT_REPORT: ${{ github.workspace }}/detector-report.json run: | node -r ./src/setup_node_env \ "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" \ - | tee "$GITHUB_WORKSPACE/frontend-contract.log" + | tee "$GITHUB_WORKSPACE/detector-contract.log" - - name: Upload frontend report and corpus + - name: Upload detector report and corpus if: ${{ always() }} uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 continue-on-error: true with: - name: ppl-lint-frontend-report + name: ppl-lint-detector path: | - frontend-contract.log - frontend-report.json + detector-contract.log + detector-report.json integ-test/src/test/resources/ppl-lint/contracts - Get-CI-Image-Tag: - uses: opensearch-project/opensearch-build/.github/workflows/get-ci-image-tag.yml@761e093b8c1349cc07f21c1d681d3b30bf9e1999 # main - with: - product: opensearch - - backend: - name: Backend integration test (live /_plugins/_ppl) - needs: Get-CI-Image-Tag + validation-result: + name: validation-result + if: ${{ always() }} + needs: + - backend-validation + - detector-validation runs-on: ubuntu-latest - container: - image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} - options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} - steps: - - name: Run start commands - run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }} - - name: Checkout SQL pull request uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - - name: Resolve contract schedule - id: schedule - env: - REQUESTED_SCHEDULE: ${{ inputs.schedule }} - EVENT_NAME: ${{ github.event_name }} - run: | - if [ -n "$REQUESTED_SCHEDULE" ]; then - value="$REQUESTED_SCHEDULE" - elif [ "$EVENT_NAME" = "schedule" ]; then - value="nightly" - else - value="pr" - fi - echo "value=$value" >> "$GITHUB_OUTPUT" + - name: Download backend artifacts + uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 + continue-on-error: true + with: + name: ppl-lint-backend + path: artifacts - - name: Set up JDK 21 - uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4 + - name: Download detector artifacts + uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 + continue-on-error: true with: - distribution: 'temurin' - java-version: 21 + name: ppl-lint-detector + path: artifacts - # OpenSearch refuses to start as root, so run Gradle as a non-root user. - - name: Run backend integration test - run: | - chown -R 1000:1000 "$(pwd)" - su "$(id -un 1000)" -c "./gradlew :integ-test:integTest --tests org.opensearch.sql.calcite.remote.PplLintRuleValidationIT -Dppl.lint.schedule=${{ steps.schedule.outputs.value }} -Dppl.lint.report=$(pwd)/backend-report.json" + # Assemble the run manifest and the compact per-rule PR summary from the + # reports both jobs uploaded. The manifest records the immutable SQL + OSD + # SHAs so any run is exactly reproducible (design §3.3, §4.4). + - name: Assemble run manifest and summary + env: + SQL_SHA: ${{ github.sha }} + OSD_REF: ${{ needs.detector-validation.outputs.osd_ref }} + OSD_SHA: ${{ needs.detector-validation.outputs.osd_sha }} + EVENT_NAME: ${{ github.event_name }} + SCHEDULE: ${{ needs.detector-validation.outputs.schedule }} + BACKEND_RESULT: ${{ needs.backend-validation.result }} + DETECTOR_RESULT: ${{ needs.detector-validation.result }} + run: node "$GITHUB_WORKSPACE/scripts/ppl-lint/assemble-run-manifest.mjs" - - name: Upload backend report + - name: Upload run manifest if: ${{ always() }} uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 continue-on-error: true with: - name: ppl-lint-backend-report + name: ppl-lint-run-manifest path: | - backend-report.json + run-manifest.json - - name: Upload failure artifacts - if: ${{ failure() }} - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 - continue-on-error: true - with: - name: ppl-lint-backend-artifacts - path: | - integ-test/build/reports/** - integ-test/build/testclusters/*/logs/* + # The sole branch-protection check: red unless BOTH validation jobs + # succeeded. Because this job runs with always(), a skipped detector job + # (e.g. backend failed first) still reds the result instead of appearing + # green (design §4.4). A workflow_dispatch run is pre-merge evidence and is + # intentionally not what repo admins pin to branch protection. + - name: Require both validation jobs to have succeeded + env: + BACKEND_RESULT: ${{ needs.backend-validation.result }} + DETECTOR_RESULT: ${{ needs.detector-validation.result }} + run: | + echo "backend-validation: $BACKEND_RESULT" + echo "detector-validation: $DETECTOR_RESULT" + if [ "$BACKEND_RESULT" != "success" ] || [ "$DETECTOR_RESULT" != "success" ]; then + echo "::error::PPL lint rule validation failed (backend=$BACKEND_RESULT detector=$DETECTOR_RESULT)." + exit 1 + fi + echo "PPL lint rule validation passed: backend and detector agree on the candidate grammar." diff --git a/.gitignore b/.gitignore index 0f4dc3f32e2..00db1869c8d 100644 --- a/.gitignore +++ b/.gitignore @@ -61,7 +61,10 @@ http-client.env.json .clinerules memory-bank # PPL lint rule validation contract run artifacts (uploaded in CI, not committed) -frontend-report.json backend-report.json backend-report-nightly.json -frontend-contract.log +detector-report.json +detector-contract.log +ppl-grammar-bundle.json +target.json +run-manifest.json diff --git a/integ-test/build.gradle b/integ-test/build.gradle index 3a25caf99df..1d8f3af45f4 100644 --- a/integ-test/build.gradle +++ b/integ-test/build.gradle @@ -169,13 +169,17 @@ tasks.withType(licenseHeaders.class) { } // Forward the PPL lint rule validation contract knobs to every integ test JVM -// (PplLintRuleValidationIT reads them): which schedule to run (pr|nightly) and -// an optional path to write the observed-vs-expected report. Applied globally so -// every RestIntegTestTask that runs the class picks it up without per-task edits. +// (PplLintRuleValidationIT reads them): which schedule to run (pr|nightly), an +// optional path to write the observed backend report, and — while the cluster is +// alive — optional paths to export the candidate runtime grammar bundle and its +// target manifest for the detector-validation job. Applied globally so every +// RestIntegTestTask that runs the class picks it up without per-task edits. tasks.withType(Test).configureEach { systemProperty "ppl.lint.schedule", System.getProperty("ppl.lint.schedule", "pr") - if (System.getProperty("ppl.lint.report") != null) { - systemProperty "ppl.lint.report", System.getProperty("ppl.lint.report") + ["ppl.lint.report", "ppl.lint.grammar.bundle", "ppl.lint.target"].each { prop -> + if (System.getProperty(prop) != null) { + systemProperty prop, System.getProperty(prop) + } } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java index 211ad341293..4185511ab04 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/PplLintRuleValidationIT.java @@ -9,6 +9,7 @@ import static org.opensearch.sql.plugin.rest.RestPPLQueryAction.QUERY_API_ENDPOINT; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Paths; import java.util.ArrayList; @@ -28,12 +29,14 @@ import org.opensearch.sql.ppl.PPLIntegTestCase; /** - * Backend half of the schema-v2 PPL lint rule validation contract. + * Backend half of the schema-v3 PPL lint rule validation contract. * *

This test drives the live {@code POST /_plugins/_ppl} endpoint on the SQL plugin built from - * the current checkout. For every contract case (see {@code - * src/test/resources/ppl-lint/contracts/*.spec.json}) it applies the case's cluster settings and - * asserts, per {@code backend.kind}: + * the current checkout. For every contract (see {@code + * src/test/resources/ppl-lint/contracts/*.spec.json}) it selects the single {@code expectations[]} + * entry that matches the candidate backend version (exactly one must match, or the contract fails + * before any query runs), applies the contract's cluster settings, and asserts, per query's {@code + * backend.kind}: * *

* - *

The contract files are shared verbatim with the SQL-owned OSD frontend adapter ({@code + *

The contract files are shared verbatim with the SQL-owned OSD detector runner ({@code * scripts/ppl-lint/run-frontend-contract.mjs}) so the same reviewed cases pin both the OSD analyzer * diagnostic count and the SQL backend behavior; neither side can drift without a red build. The * rejection-body parsing mirrors {@link * org.opensearch.sql.calcite.remote.CalciteErrorReportStageIT}; the Calcite setup follows {@link * org.opensearch.sql.calcite.remote.CalcitePPLEventstatsIT}. * + *

While the ephemeral cluster is alive, the test also exports the candidate runtime grammar + * bundle it built ({@code GET /_plugins/_ppl/_grammar}) and a small target manifest pairing the + * bundle with the backend version and grammar hash. These become workflow artifacts that the + * detector-validation job injects into OSD's headless lint API, so both halves validate against the + * SAME candidate grammar (design §4.2, §4.3). Export runs only when {@code + * -Dppl.lint.grammar.bundle} is set (CI); local runs without it are unaffected. + * *

The suite honors {@code -Dppl.lint.schedule=pr|nightly} (default {@code pr}): PR runs only the - * fast, deterministic {@code schedule:pr} contracts; nightly runs the full corpus including the - * runtime-only and softer-oracle rules. + * fast, deterministic {@code schedule:pr} contracts; nightly runs the full corpus. */ public class PplLintRuleValidationIT extends PPLIntegTestCase { private static final String CONTRACT_DIR = "src/test/resources/ppl-lint/contracts"; private static final String MANIFEST = CONTRACT_DIR + "/manifest.json"; + private static final String GRAMMAR_API_ENDPOINT = "/_plugins/_ppl/_grammar"; /** Which contracts to run this session; PR is the fast blocking subset. */ private final String schedule = System.getProperty("ppl.lint.schedule", "pr"); private int[] clusterVersion; + private String engineVersionRaw; @Override public void init() throws Exception { @@ -82,6 +93,11 @@ public void testValidatesLintRuleContracts() throws IOException { List failures = new ArrayList<>(); JSONArray report = new JSONArray(); + // Export the candidate grammar bundle + target manifest while the cluster is + // alive. Runs before the contract loop so the artifacts are emitted even if a + // contract later fails. + exportGrammarArtifacts(failures); + for (JSONObject contract : contracts) { String ruleId = contract.getString("ruleId"); runContract(contract, ruleId, failures, report); @@ -102,33 +118,44 @@ private void runContract( JSONObject contract, String ruleId, List failures, JSONArray report) throws IOException { String index = contract.getString("index"); - JSONArray cases = contract.getJSONArray("cases"); + JSONObject queries = contract.getJSONObject("queries"); + JSONArray expectations = contract.getJSONArray("expectations"); JSONObject fixture = contract.optJSONObject("backendFixture"); + boolean calciteOn = fixtureCalciteEnabled(fixture); List applied = applyClusterSettings(fixture); try { - for (int i = 0; i < cases.length(); i++) { - JSONObject testCase = cases.getJSONObject(i); - String caseId = testCase.getString("id"); - String query = testCase.getString("query").replace("{{index}}", index); - - String minVersion = testCase.optString("minVersionRequired", null); - if (minVersion != null && !versionAtLeast(minVersion)) { - log(ruleId, caseId, "SKIP (needs >= " + minVersion + ")"); + JSONObject selected = selectExpectation(ruleId, expectations, calciteOn, failures); + if (selected == null) { + return; // no/ambiguous version expectation — failure already recorded. + } + JSONObject expectedQueries = selected.getJSONObject("queries"); + for (String queryName : expectedQueries.keySet()) { + if (!queries.has(queryName)) { + failures.add( + "[" + + ruleId + + "] expectation references unknown query \"" + + queryName + + "\" (not in the top-level queries map)"); continue; } - - JSONObject backend = resolveBackend(testCase); + JSONObject queryDef = queries.getJSONObject(queryName); + String role = queryDef.optString("role", "trigger"); + String query = queryDef.getString("query").replace("{{index}}", index); + JSONObject expected = expectedQueries.getJSONObject(queryName); + JSONObject backend = expected.getJSONObject("backend"); String kind = backend.getString("kind"); - JSONObject entry = reportEntry(ruleId, caseId, query, kind); + + JSONObject entry = reportEntry(ruleId, queryName, role, query, kind); try { - verifyCase(kind, caseId, query, backend, entry); + verifyCase(kind, queryName, query, backend, entry); entry.put("outcome", "pass"); - log(ruleId, caseId, "PASS (" + kind + ")"); + log(ruleId, queryName, "PASS (" + kind + ", " + role + ")"); } catch (AssertionError | RuntimeException e) { entry.put("outcome", "fail").put("error", String.valueOf(e.getMessage())); - failures.add("[" + ruleId + "/" + caseId + "] " + e.getMessage()); - log(ruleId, caseId, "FAIL (" + kind + "): " + e.getMessage()); + failures.add("[" + ruleId + "/" + queryName + "] " + e.getMessage()); + log(ruleId, queryName, "FAIL (" + kind + "): " + e.getMessage()); } report.put(entry); } @@ -137,107 +164,140 @@ private void runContract( } } + /** + * Select the single {@code expectations[]} entry that applies to the candidate backend version + * and engine. Exactly one must match: zero means the rule test does not cover this version + * (design §9), and more than one means overlapping ranges — both fail before execution (§5.3). + */ + private JSONObject selectExpectation( + String ruleId, JSONArray expectations, boolean calciteOn, List failures) { + List matches = new ArrayList<>(); + for (int i = 0; i < expectations.length(); i++) { + JSONObject exp = expectations.getJSONObject(i); + if (!versionMatchesRange(exp.optString("version", null))) { + continue; + } + String engine = exp.optString("engine", ""); + if ("calcite".equals(engine) && !calciteOn) { + continue; + } + matches.add(exp); + } + String versionLabel = engineVersionRaw == null ? "unknown" : engineVersionRaw; + if (matches.size() == 1) { + return matches.get(0); + } + if (matches.isEmpty()) { + failures.add( + "[" + ruleId + "] no version expectation matches backend version " + versionLabel); + } else { + failures.add( + "[" + + ruleId + + "] " + + matches.size() + + " expectations match backend version " + + versionLabel + + " (exactly one required)"); + } + return null; + } + private void verifyCase( - String kind, String caseId, String query, JSONObject backend, JSONObject reportEntry) + String kind, String queryName, String query, JSONObject backend, JSONObject entry) throws IOException { + BackendObservation obs = observeBackend(query); + entry.put("rejected", obs.rejected); + entry.put("observed", obs.toJson()); switch (kind) { case "rejection": - verifyRejectedCase( - caseId, - query, - backend.getInt("httpStatus"), - backend.getJSONObject("body"), - reportEntry); + assertRejection( + queryName, query, obs, backend.getInt("httpStatus"), backend.getJSONObject("body")); break; case "result-shape": - verifyResultShape(caseId, query, backend.optJSONObject("expect")); + assertResultShape(queryName, query, obs, backend.optJSONObject("expect")); break; case "advisory": - verifyAdvisory200(caseId, query); + assertAdvisory(queryName, query, obs); break; default: throw new IllegalArgumentException( - "case \"" + caseId + "\": unknown backend.kind \"" + kind + "\""); + "case \"" + queryName + "\": unknown backend.kind \"" + kind + "\""); } } - /** Back-compat: accept both v2 {@code backend} and the legacy {@code backendExpected} shape. */ - private JSONObject resolveBackend(JSONObject testCase) { - if (testCase.has("backend")) { - return testCase.getJSONObject("backend"); - } - JSONObject legacy = testCase.getJSONObject("backendExpected"); - int status = legacy.getInt("httpStatus"); - JSONObject backend = new JSONObject(); - if (status == 200) { - return backend.put("kind", "result-shape").put("httpStatus", 200); - } - return backend - .put("kind", "rejection") - .put("httpStatus", status) - .put("body", legacy.getJSONObject("body")); + /** + * Run the query once and categorize the observed backend behavior independently of the + * expectation, so the report carries the true behavior even when a case fails (e.g. a trigger the + * backend unexpectedly accepted). A non-2xx surfaces as a {@link ResponseException} from the REST + * client, which is the rejection signal. + */ + private BackendObservation observeBackend(String query) throws IOException { + try { + JSONObject response = runPplQuery(query); + return BackendObservation.accepted(response); + } catch (ResponseException e) { + int status = e.getResponse().getStatusLine().getStatusCode(); + JSONObject body; + try { + body = new JSONObject(getResponseBody(e.getResponse(), true)); + } catch (IOException ioe) { + throw new RuntimeException( + "failed to read rejection response body for query: " + query, ioe); + } + return BackendObservation.rejected(status, body); + } } - /** A rejected query must throw with the contracted status and structured error fields. */ - private void verifyRejectedCase( - String caseId, + /** A rejected query must have thrown with the contracted status and structured error fields. */ + private void assertRejection( + String queryName, String query, + BackendObservation obs, int expectedStatus, - JSONObject expectedBody, - JSONObject reportEntry) { - ResponseException exception = assertThrows(ResponseException.class, () -> runPplQuery(query)); - - int actualStatus = exception.getResponse().getStatusLine().getStatusCode(); - - JSONObject body; - try { - body = new JSONObject(getResponseBody(exception.getResponse(), true)); - } catch (IOException e) { - throw new RuntimeException( - "case \"" + caseId + "\": failed to read rejection response body for query: " + query, e); - } - - // Record the observed status/type/reason before asserting so backend-report.json - // carries the byte-exact engine wording even for a failing case — this is what - // the snapshot should be updated to when the contract is deliberately changed. - JSONObject observed = new JSONObject().put("httpStatus", actualStatus); - JSONObject actualError = body.optJSONObject("error"); - if (actualError != null) { - observed.put("type", actualError.opt("type")).put("reason", actualError.opt("reason")); - } - reportEntry.put("observed", observed); - + JSONObject expectedBody) { + assertTrue( + "case \"" + + queryName + + "\": expected the backend to REJECT the query but it was accepted: " + + query, + obs.rejected); assertEquals( - "case \"" + caseId + "\": unexpected HTTP status for query: " + query, + "case \"" + queryName + "\": unexpected HTTP status for query: " + query, expectedStatus, - actualStatus); - + obs.status); assertEquals( - "case \"" + caseId + "\": unexpected top-level status field for query: " + query, + "case \"" + queryName + "\": unexpected top-level status field for query: " + query, expectedBody.getInt("status"), - body.getInt("status")); + obs.body.getInt("status")); JSONObject expectedError = expectedBody.getJSONObject("error"); - + JSONObject actualError = obs.body.getJSONObject("error"); assertEquals( - "case \"" + caseId + "\": unexpected error.type for query: " + query, + "case \"" + queryName + "\": unexpected error.type for query: " + query, expectedError.getString("type"), actualError.getString("type")); if (expectedError.has("reason")) { assertEquals( - "case \"" + caseId + "\": unexpected error.reason for query: " + query, + "case \"" + queryName + "\": unexpected error.reason for query: " + query, expectedError.getString("reason"), actualError.getString("reason")); } } /** A result-shape case returns 200 whose datarows match the declared expectations. */ - private void verifyResultShape(String caseId, String query, JSONObject expect) - throws IOException { - JSONObject response = runPplQuery(query); + private void assertResultShape( + String queryName, String query, BackendObservation obs, JSONObject expect) { assertTrue( "case \"" - + caseId + + queryName + + "\": expected a 200 result but the backend rejected the query: " + + query, + !obs.rejected); + JSONObject response = obs.response; + assertTrue( + "case \"" + + queryName + "\": expected a datarows array in the 200 response for query: " + query, response.has("datarows")); @@ -248,12 +308,12 @@ private void verifyResultShape(String caseId, String query, JSONObject expect) if (expect.optBoolean("datarowsNonEmpty", false)) { assertTrue( - "case \"" + caseId + "\": expected non-empty datarows for query: " + query, + "case \"" + queryName + "\": expected non-empty datarows for query: " + query, datarows.length() > 0); } if (expect.has("datarowsCount")) { assertEquals( - "case \"" + caseId + "\": unexpected datarows count for query: " + query, + "case \"" + queryName + "\": unexpected datarows count for query: " + query, expect.getInt("datarowsCount"), datarows.length()); } @@ -262,7 +322,7 @@ private void verifyResultShape(String caseId, String query, JSONObject expect) int columnIndex = schemaColumnIndex(response, column); assertTrue( "case \"" - + caseId + + queryName + "\": column \"" + column + "\" not found in schema for query: " @@ -270,7 +330,7 @@ private void verifyResultShape(String caseId, String query, JSONObject expect) columnIndex >= 0); assertTrue( "case \"" - + caseId + + queryName + "\": expected non-empty datarows to check null column for query: " + query, datarows.length() > 0); @@ -278,7 +338,7 @@ private void verifyResultShape(String caseId, String query, JSONObject expect) JSONArray row = datarows.getJSONArray(r); assertTrue( "case \"" - + caseId + + queryName + "\": expected column \"" + column + "\" to be null in every row but row " @@ -293,14 +353,20 @@ private void verifyResultShape(String caseId, String query, JSONObject expect) } /** An advisory case only requires the query to be accepted (HTTP 200 with data). */ - private void verifyAdvisory200(String caseId, String query) throws IOException { - JSONObject response = runPplQuery(query); + private void assertAdvisory(String queryName, String query, BackendObservation obs) { + assertTrue( + "case \"" + + queryName + + "\": expected the query to be accepted (advisory) but it was " + + "rejected: " + + query, + !obs.rejected); assertTrue( "case \"" - + caseId + + queryName + "\": expected a datarows array in the 200 response for query: " + query, - response.has("datarows")); + obs.response.has("datarows")); } /** @@ -338,8 +404,91 @@ private int schemaColumnIndex(JSONObject response, String column) { return -1; } + /** Observed backend behavior for one query, captured before asserting the expectation. */ + private static final class BackendObservation { + final boolean rejected; + final int status; + final JSONObject body; // rejection body, or null when accepted + final JSONObject response; // accepted 200 response, or null when rejected + + private BackendObservation(boolean rejected, int status, JSONObject body, JSONObject response) { + this.rejected = rejected; + this.status = status; + this.body = body; + this.response = response; + } + + static BackendObservation accepted(JSONObject response) { + return new BackendObservation(false, 200, null, response); + } + + static BackendObservation rejected(int status, JSONObject body) { + return new BackendObservation(true, status, body, null); + } + + JSONObject toJson() { + JSONObject o = new JSONObject().put("httpStatus", status).put("rejected", rejected); + if (body != null) { + JSONObject err = body.optJSONObject("error"); + if (err != null) { + o.put("type", err.opt("type")).put("reason", err.opt("reason")); + } + } + return o; + } + } + + // --- grammar bundle export ------------------------------------------------- + + /** + * Fetch the candidate runtime grammar bundle and write it plus a target manifest, so the + * detector-validation job can lint against the SAME grammar this backend built. Best-effort by + * design: a run without {@code -Dppl.lint.grammar.bundle} (local dev) exports nothing; in CI a + * fetch/write failure is a real failure — a missing bundle means the detector half cannot run. + */ + private void exportGrammarArtifacts(List failures) { + String bundlePath = System.getProperty("ppl.lint.grammar.bundle"); + if (bundlePath == null || bundlePath.isEmpty()) { + return; + } + try { + Response response = client().performRequest(new Request("GET", GRAMMAR_API_ENDPOINT)); + String bundleBody = getResponseBody(response, true); + Files.write(Paths.get(bundlePath), bundleBody.getBytes(StandardCharsets.UTF_8)); + + JSONObject bundle = new JSONObject(bundleBody); + String grammarHash = bundle.optString("grammarHash", ""); + + String targetPath = System.getProperty("ppl.lint.target"); + if (targetPath != null && !targetPath.isEmpty()) { + JSONObject target = + new JSONObject() + .put("engineVersion", engineVersionRaw == null ? "" : engineVersionRaw) + .put("grammarHash", grammarHash) + .put("grammarBundle", Paths.get(bundlePath).getFileName().toString()); + Files.write(Paths.get(targetPath), target.toString(2).getBytes(StandardCharsets.UTF_8)); + } + log("_grammar", "export", "wrote candidate bundle (" + grammarHash + ") to " + bundlePath); + } catch (Exception e) { + failures.add( + "[grammar-export] failed to fetch/write " + GRAMMAR_API_ENDPOINT + ": " + e.getMessage()); + } + } + // --- cluster settings ------------------------------------------------------ + /** True when the contract's fixture leaves Calcite enabled (the default). */ + private boolean fixtureCalciteEnabled(JSONObject fixture) { + if (fixture == null) { + return true; + } + JSONObject settings = fixture.optJSONObject("clusterSettings"); + if (settings == null || !settings.has("calcite")) { + return true; + } + return settings.getBoolean("calcite"); + } + /** * Apply the contract's cluster settings and return the list of settings changed so the caller can * reset them afterwards. Grouped per-contract (not global) because contracts disagree: eventstats @@ -402,6 +551,7 @@ private int[] fetchClusterVersion() { Response response = client().performRequest(new Request("GET", "/")); JSONObject body = new JSONObject(getResponseBody(response, false)); String number = body.getJSONObject("version").getString("number"); + engineVersionRaw = number; return parseVersion(number); } catch (Exception e) { // Unknown version → do not skip anything. @@ -409,18 +559,72 @@ private int[] fetchClusterVersion() { } } - private boolean versionAtLeast(String required) { + /** + * Test a space-separated semver range (e.g. {@code ">=3.6.0 <3.8.0"}) against the candidate + * backend version. An empty/absent range or an unknown cluster version matches (do not + * over-filter). Supports the {@code >= > <= < =} comparators the design uses. + */ + private boolean versionMatchesRange(String range) { + if (range == null || range.trim().isEmpty()) { + return true; + } if (clusterVersion == null) { return true; } - int[] want = parseVersion(required); - for (int i = 0; i < 3; i++) { - if (clusterVersion[i] > want[i]) return true; - if (clusterVersion[i] < want[i]) return false; + for (String token : range.trim().split("\\s+")) { + if (!satisfiesComparator(token)) { + return false; + } } return true; } + private boolean satisfiesComparator(String token) { + String op; + String ver; + if (token.startsWith(">=")) { + op = ">="; + ver = token.substring(2); + } else if (token.startsWith("<=")) { + op = "<="; + ver = token.substring(2); + } else if (token.startsWith(">")) { + op = ">"; + ver = token.substring(1); + } else if (token.startsWith("<")) { + op = "<"; + ver = token.substring(1); + } else if (token.startsWith("=")) { + op = "="; + ver = token.substring(1); + } else { + op = "="; + ver = token; + } + int cmp = compareVersion(clusterVersion, parseVersion(ver)); + switch (op) { + case ">=": + return cmp >= 0; + case "<=": + return cmp <= 0; + case ">": + return cmp > 0; + case "<": + return cmp < 0; + default: + return cmp == 0; + } + } + + private int compareVersion(int[] a, int[] b) { + for (int i = 0; i < 3; i++) { + if (a[i] != b[i]) { + return Integer.compare(a[i], b[i]); + } + } + return 0; + } + private int[] parseVersion(String raw) { String cleaned = raw.split("-")[0]; String[] parts = cleaned.split("\\."); @@ -489,10 +693,12 @@ private JSONObject loadContractFile(String resourcePath) throws IOException { // --- reporting ------------------------------------------------------------- - private JSONObject reportEntry(String ruleId, String caseId, String query, String kind) { + private JSONObject reportEntry( + String ruleId, String queryName, String role, String query, String kind) { return new JSONObject() .put("ruleId", ruleId) - .put("caseId", caseId) + .put("queryName", queryName) + .put("role", role) .put("query", query) .put("kind", kind); } @@ -503,7 +709,7 @@ private void writeReport(JSONArray report) { return; } try { - Files.write(Paths.get(target), report.toString(2).getBytes()); + Files.write(Paths.get(target), report.toString(2).getBytes(StandardCharsets.UTF_8)); } catch (IOException e) { System.err.println("[ppl-lint] could not write backend report to " + target + ": " + e); } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json index 4c292860f6c..ca414f6f103 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/dedup-consecutive-unsupported.spec.json @@ -1,7 +1,6 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "dedup-consecutive-unsupported", - "oracleClass": "advisory", "grammarSurface": "compiled-simplified", "schedule": "nightly", "wiring": { @@ -21,22 +20,31 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "dedup-consecutive-true", - "query": "source={{index}} | dedup firstname consecutive=true", - "minVersionRequired": "3.3.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 1, "severity": "warning" }, - "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + "queries": { + "dedup-consecutive-true": { + "role": "trigger", + "query": "source={{index}} | dedup firstname consecutive=true" }, + "dedup-plain-control": { + "role": "control", + "query": "source={{index}} | dedup firstname" + } + }, + "expectations": [ { - "id": "dedup-plain-control", - "query": "source={{index}} | dedup firstname", - "minVersionRequired": "3.3.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + "version": ">=3.3.0", + "engine": "calcite", + "queries": { + "dedup-consecutive-true": { + "detectorCount": 1, + "severity": "warning", + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + }, + "dedup-plain-control": { + "detectorCount": 0, + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + } + } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json index 3e7c7302f77..bd0769934ea 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/disabled-join-type.spec.json @@ -1,9 +1,8 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "disabled-join-type", - "oracleClass": "rejection", "grammarSurface": "compiled-simplified", - "schedule": "pr", + "schedule": "nightly", "wiring": { "detector": "disabled-join-type", "enabled": true, @@ -21,32 +20,47 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "right-join-disabled", - "query": "source={{index}} | right join left=l right=r on l.account_number=r.account_number {{index}}", - "frontend": { "diagnosticCount": 1, "severity": "warning" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { "status": 400, "error": { "type": "SemanticCheckException", "reason": "Invalid Query" } } - } + "queries": { + "right-join-disabled": { + "role": "trigger", + "query": "source={{index}} | right join left=l right=r on l.account_number=r.account_number {{index}}" }, - { - "id": "cross-join-disabled", - "query": "source={{index}} | cross join left=l right=r on l.account_number=r.account_number {{index}}", - "frontend": { "diagnosticCount": 1, "severity": "warning" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { "status": 400, "error": { "type": "SemanticCheckException", "reason": "Invalid Query" } } - } + "cross-join-disabled": { + "role": "trigger", + "query": "source={{index}} | cross join left=l right=r on l.account_number=r.account_number {{index}}" }, + "inner-join-control": { + "role": "control", + "query": "source={{index}} | join left=l right=r on l.account_number=r.account_number {{index}} | head 1" + } + }, + "expectations": [ { - "id": "inner-join-control", - "query": "source={{index}} | join left=l right=r on l.account_number=r.account_number {{index}} | head 1", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + "version": ">=0.0.0", + "queries": { + "right-join-disabled": { + "detectorCount": 1, + "severity": "warning", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "SemanticCheckException", "reason": "Invalid Query" } } + } + }, + "cross-join-disabled": { + "detectorCount": 1, + "severity": "warning", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "SemanticCheckException", "reason": "Invalid Query" } } + } + }, + "inner-join-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json index 514476d746a..d9071cc4978 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/division-by-zero.spec.json @@ -1,9 +1,8 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "division-by-zero", - "oracleClass": "result-shape", "grammarSurface": "compiled-simplified", - "schedule": "pr", + "schedule": "nightly", "wiring": { "detector": "division-by-zero", "enabled": true, @@ -21,24 +20,39 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "divide-by-zero-literal", - "query": "source={{index}} | eval ratio = balance / 0 | fields ratio | head 1", - "frontend": { "diagnosticCount": 1, "severity": "warning" }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "columnAllNull": "ratio" } } + "queries": { + "divide-by-zero-literal": { + "role": "trigger", + "query": "source={{index}} | eval ratio = balance / 0 | fields ratio | head 1" }, - { - "id": "divide-by-nonzero-control", - "query": "source={{index}} | eval ratio = balance / 2 | fields ratio | head 1", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + "divide-by-nonzero-control": { + "role": "control", + "query": "source={{index}} | eval ratio = balance / 2 | fields ratio | head 1" }, + "modulo-by-zero-flagged": { + "role": "trigger", + "query": "source={{index}} | eval m = balance % 0 | fields m | head 1" + } + }, + "expectations": [ { - "id": "modulo-by-zero-flagged", - "query": "source={{index}} | eval m = balance % 0 | fields m | head 1", - "frontend": { "diagnosticCount": 1, "severity": "warning" }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "columnAllNull": "m" } } + "version": ">=0.0.0", + "queries": { + "divide-by-zero-literal": { + "detectorCount": 1, + "severity": "warning", + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "columnAllNull": "ratio" } } + }, + "divide-by-nonzero-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + }, + "modulo-by-zero-flagged": { + "detectorCount": 1, + "severity": "warning", + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "columnAllNull": "m" } } + } + } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json index e383f773bb9..c67343ef23a 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/field-validation.spec.json @@ -1,9 +1,8 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "field-validation", - "oracleClass": "rejection", "grammarSurface": "compiled-simplified", - "schedule": "pr", + "schedule": "nightly", "wiring": { "detector": "field-validation", "enabled": true, @@ -35,32 +34,48 @@ } }, "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "unknown-field-existence", - "query": "source={{index}} | where nonexistent_field > 3", - "frontend": { "diagnosticCount": 1, "severity": "error", "matchMessage": "nonexistent_field" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Field [nonexistent_field] not found." } } - } + "queries": { + "unknown-field-existence": { + "role": "trigger", + "query": "source={{index}} | where nonexistent_field > 3" }, - { - "id": "grok-field-slot-shape-typo", - "query": "source={{index}} | grok field=firstname \"%{WORD:w}\"", - "frontend": { "diagnosticCount": 1, "severity": "error" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Field [field] not found." } } - } + "grok-field-slot-shape-typo": { + "role": "trigger", + "query": "source={{index}} | grok field=firstname \"%{WORD:w}\"" }, + "known-field-control": { + "role": "control", + "query": "source={{index}} | where age > 30 | head 1" + } + }, + "expectations": [ { - "id": "known-field-control", - "query": "source={{index}} | where age > 30 | head 1", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + "version": ">=3.4.0", + "queries": { + "unknown-field-existence": { + "detectorCount": 1, + "severity": "error", + "matchMessage": "nonexistent_field", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Field [nonexistent_field] not found." } } + } + }, + "grok-field-slot-shape-typo": { + "detectorCount": 1, + "severity": "error", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Field [field] not found." } } + } + }, + "known-field-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json index 1d800dfcd6d..e86a19c1002 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/head-without-sort.spec.json @@ -1,9 +1,8 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "head-without-sort", - "oracleClass": "advisory", "grammarSurface": "compiled-simplified", - "schedule": "pr", + "schedule": "nightly", "wiring": { "detector": "head-without-sort", "enabled": true, @@ -21,18 +20,30 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "head-without-sort", - "query": "source={{index}} | head 5", - "frontend": { "diagnosticCount": 1, "severity": "info" }, - "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + "queries": { + "head-without-sort": { + "role": "trigger", + "query": "source={{index}} | head 5" }, + "head-with-sort-control": { + "role": "control", + "query": "source={{index}} | sort age | head 5" + } + }, + "expectations": [ { - "id": "head-with-sort-control", - "query": "source={{index}} | sort age | head 5", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + "version": ">=0.0.0", + "queries": { + "head-without-sort": { + "detectorCount": 1, + "severity": "info", + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + }, + "head-with-sort-control": { + "detectorCount": 0, + "backend": { "kind": "advisory", "httpStatus": 200, "expect": { "accepted": true } } + } + } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/manifest.json b/integ-test/src/test/resources/ppl-lint/contracts/manifest.json index 31b54ecf18b..805e1be9fd8 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/manifest.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/manifest.json @@ -1,6 +1,6 @@ { - "schemaVersion": 2, - "description": "Index of PPL lint rule validation contracts. Each entry pins one OSD analyzer rule to live SQL /_plugins/_ppl behavior. The frontend adapter (scripts/ppl-lint/run-frontend-contract.mjs) and backend IT (PplLintRuleValidationIT) both read these files.", + "schemaVersion": 3, + "description": "Index of PPL lint rule validation contracts. Each entry pins one OSD analyzer rule to live SQL /_plugins/_ppl behavior. The detector runner (scripts/ppl-lint/run-frontend-contract.mjs) and backend IT (PplLintRuleValidationIT) both read these files. `contracts` is the full corpus; `enforced` is the phase-one, reviewed, error-severity subset with a stable backend rejection oracle that blocks a PR (design §5.1, §5.2). Everything not in `enforced` runs non-blocking (nightly / advisory) until it has an equally stable oracle and owner review.", "contracts": [ "unsupported-window-function-in-eventstats.spec.json", "division-by-zero.spec.json", @@ -11,5 +11,25 @@ "multisearch-min-subsearch.spec.json", "union-min-datasets.spec.json", "replace-wildcard-asymmetry.spec.json" - ] + ], + "enforced": [ + "unsupported-window-function-in-eventstats.spec.json", + "multisearch-min-subsearch.spec.json", + "union-min-datasets.spec.json", + "replace-wildcard-asymmetry.spec.json" + ], + "pendingReview": [ + "field-validation.spec.json" + ], + "nonEnforcing": [ + "division-by-zero.spec.json", + "head-without-sort.spec.json", + "disabled-join-type.spec.json", + "dedup-consecutive-unsupported.spec.json" + ], + "notes": { + "enforced": "Reviewed error rules with a deterministic backend rejection and a valid negative control. These block the required validation-result check.", + "pendingReview": "Error rules awaiting Peng/Chen usefulness + false-positive review (design §5.2) before joining `enforced`. field-validation self-suppresses without field context and is a semantic rule rather than a clean HTTP-400 grammar rejection.", + "nonEnforcing": "Warning / info / advisory / result-shape rules. They lack a stable backend rejection oracle and never block a PR; they run for coverage on the nightly schedule." + } } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json index 5d64a7ba674..018d086fec3 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/multisearch-min-subsearch.spec.json @@ -1,10 +1,10 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "multisearch-min-subsearch", - "oracleClass": "rejection", "grammarSurface": "runtime-bundle", - "schedule": "nightly", + "schedule": "pr", "requiredParserRules": ["multisearchCommand", "subSearch"], + "notes": "Query-initial (no leading pipe) on purpose — see the note on union-min-datasets. OSD's runtime lint prepends a synthetic 'source=t ' prefix to pipe-first queries, which would change the effective parse relative to what the backend receives. A query-initial 'multisearch [...]' is sent byte-identically to both halves.", "wiring": { "detector": "multisearch-min-subsearch", "enabled": true, @@ -22,26 +22,34 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "multisearch-single-subsearch", - "query": "| multisearch [ search source={{index}} ]", - "minVersionRequired": "3.4.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 1, "severity": "error" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { "status": 400, "error": { "type": "SyntaxCheckException", "reason": "Invalid Query" } } - } + "queries": { + "multisearch-single-subsearch": { + "role": "trigger", + "query": "multisearch [ search source={{index}} ]" }, + "multisearch-two-subsearches-control": { + "role": "control", + "query": "multisearch [ search source={{index}} ] [ search source={{index}} ]" + } + }, + "expectations": [ { - "id": "multisearch-two-subsearches-control", - "query": "| multisearch [ search source={{index}} ] [ search source={{index}} ]", - "minVersionRequired": "3.4.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + "version": ">=3.4.0", + "queries": { + "multisearch-single-subsearch": { + "detectorCount": 1, + "severity": "error", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "SyntaxCheckException", "reason": "Invalid Query" } } + } + }, + "multisearch-two-subsearches-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json index 36a0231c423..8946dad60a9 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/replace-wildcard-asymmetry.spec.json @@ -1,9 +1,8 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "replace-wildcard-asymmetry", - "oracleClass": "rejection", "grammarSurface": "runtime-bundle", - "schedule": "nightly", + "schedule": "pr", "requiredParserRules": ["replacePair", "stringLiteral"], "wiring": { "detector": "replace-wildcard-asymmetry", @@ -22,32 +21,41 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ + "queries": { + "replace-wildcard-count-mismatch": { + "role": "trigger", + "query": "source={{index}} | replace \"*_a\" with \"b_*_*\" in firstname" + }, + "replace-symmetric-control": { + "role": "control", + "query": "source={{index}} | replace \"*_a\" with \"b_*\" in firstname | head 1" + } + }, + "expectations": [ { - "id": "replace-wildcard-count-mismatch", - "query": "source={{index}} | replace \"*_a\" with \"b_*_*\" in firstname", - "minVersionRequired": "3.4.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 1, "severity": "error" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { - "status": 400, - "error": { - "type": "IllegalArgumentException", - "reason": "Error in 'replace' command: Wildcard count mismatch - pattern has 1 wildcard(s), replacement has 2. Replacement must have same number of wildcards or none." + "version": ">=3.4.0", + "engine": "calcite", + "queries": { + "replace-wildcard-count-mismatch": { + "detectorCount": 1, + "severity": "error", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { + "status": 400, + "error": { + "type": "IllegalArgumentException", + "reason": "Error in 'replace' command: Wildcard count mismatch - pattern has 1 wildcard(s), replacement has 2. Replacement must have same number of wildcards or none." + } + } } + }, + "replace-symmetric-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } } } - }, - { - "id": "replace-symmetric-control", - "query": "source={{index}} | replace \"*_a\" with \"b_*\" in firstname | head 1", - "minVersionRequired": "3.4.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json index eb13103a37d..7f110cc9423 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/union-min-datasets.spec.json @@ -1,10 +1,10 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "union-min-datasets", - "oracleClass": "rejection", "grammarSurface": "runtime-bundle", - "schedule": "nightly", + "schedule": "pr", "requiredParserRules": ["unionCommand", "unionDataset", "pplCommands"], + "notes": "Query-initial (no leading pipe) on purpose. OSD's runtime lint prepends a synthetic 'source=t ' prefix to pipe-first queries, so linting '| union [...]' actually parses 'source=t | union [...]' — a valid MID-pipeline union (implicit upstream dataset) that the detector deliberately does not flag. The backend, receiving the raw pipe-first query, would still reject it, so a pipe-first trigger makes the two halves test different effective queries (violating the design's 'same queries' rule). A query-initial 'union [...]' is sent byte-identically to both sides and keeps the differential sound.", "wiring": { "detector": "union-min-datasets", "enabled": true, @@ -22,26 +22,35 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ - { - "id": "union-single-dataset", - "query": "| union [ source={{index}} ]", - "minVersionRequired": "3.7.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 1, "severity": "error" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Union command requires at least two datasets. Provided: 1" } } - } + "queries": { + "union-single-dataset": { + "role": "trigger", + "query": "union [ source={{index}} ]" }, + "union-two-datasets-control": { + "role": "control", + "query": "union [ source={{index}} ] [ source={{index}} ]" + } + }, + "expectations": [ { - "id": "union-two-datasets-control", - "query": "| union [ source={{index}} ] [ source={{index}} ]", - "minVersionRequired": "3.7.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + "version": ">=3.7.0", + "engine": "calcite", + "queries": { + "union-single-dataset": { + "detectorCount": 1, + "severity": "error", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { "status": 400, "error": { "type": "IllegalArgumentException", "reason": "Union command requires at least two datasets. Provided: 1" } } + } + }, + "union-two-datasets-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + } } ] } diff --git a/integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json b/integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json index 8ad00479144..e1254b14583 100644 --- a/integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json +++ b/integ-test/src/test/resources/ppl-lint/contracts/unsupported-window-function-in-eventstats.spec.json @@ -1,7 +1,6 @@ { - "schemaVersion": 2, + "schemaVersion": 3, "ruleId": "unsupported-window-function-in-eventstats", - "oracleClass": "rejection", "grammarSurface": "compiled-simplified", "schedule": "pr", "wiring": { @@ -21,29 +20,37 @@ "isCalcite": true }, "index": "opensearch-sql_test_index_account", - "cases": [ + "queries": { + "eventstats-rank": { + "role": "trigger", + "query": "source={{index}} | eventstats rank() as rank_value" + }, + "eventstats-avg-control": { + "role": "control", + "query": "source={{index}} | eventstats avg(age) as avg_age" + } + }, + "expectations": [ { - "id": "eventstats-rank", - "query": "source={{index}} | eventstats rank() as rank_value", - "minVersionRequired": "3.4.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 1, "severity": "error" }, - "backend": { - "kind": "rejection", - "httpStatus": 400, - "body": { - "status": 400, - "error": { "type": "CalciteUnsupportedException", "reason": "Unexpected window function: rank" } + "version": ">=3.4.0", + "queries": { + "eventstats-rank": { + "detectorCount": 1, + "severity": "error", + "backend": { + "kind": "rejection", + "httpStatus": 400, + "body": { + "status": 400, + "error": { "type": "CalciteUnsupportedException", "reason": "Unexpected window function: rank" } + } + } + }, + "eventstats-avg-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } } } - }, - { - "id": "eventstats-avg-control", - "query": "source={{index}} | eventstats avg(age) as avg_age", - "minVersionRequired": "3.4.0", - "engineRequired": "calcite", - "frontend": { "diagnosticCount": 0 }, - "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } } ] } diff --git a/scripts/ppl-lint-rule-validation.sh b/scripts/ppl-lint-rule-validation.sh index 515dda9c3bf..295168f8aad 100755 --- a/scripts/ppl-lint-rule-validation.sh +++ b/scripts/ppl-lint-rule-validation.sh @@ -5,27 +5,36 @@ # # Local developer entry point for the PPL lint rule validation contract. # -# Runs both halves of the cross-repository check from a SQL checkout: -# 1. Frontend: loads the compiled OpenSearch-Dashboards (OSD) PPL analyzer and -# asserts the rule's diagnostic counts against the shared contract. -# 2. Backend: runs the Gradle integration test against a live /_plugins/_ppl -# endpoint on the SQL plugin built from this checkout. +# Runs both halves of the cross-repository check from a SQL checkout, in the same +# order as CI (design §3.1): +# 1. Backend: runs the Gradle integration test against a live /_plugins/_ppl +# endpoint on the SQL plugin built from this checkout, and — while the +# cluster is alive — exports the candidate runtime grammar bundle +# (ppl-grammar-bundle.json), a target manifest (target.json), and the +# observed backend report (backend-report.json). +# 2. Detector: bootstraps an OpenSearch-Dashboards (OSD) checkout, deserializes +# the candidate bundle through OSD's headless lint API, runs the real +# detectors against the same queries, and asserts the detector-vs-backend +# differential. +# +# The backend half must run first: the detector half lints against the bundle it +# exports. Use SKIP_BACKEND=1 only if you already have the three artifacts. # # Usage: -# # OSD main frontend check plus SQL backend IT (fetches OSD into .ci/) +# # OSD main detector check plus SQL backend IT (fetches OSD into .ci/) # ./scripts/ppl-lint-rule-validation.sh # # # Reuse an existing OSD checkout (skips clone + bootstrap if node_modules present) # OSD_SOURCE_PATH=../OpenSearch-Dashboards ./scripts/ppl-lint-rule-validation.sh # # # Reproduce a CI run against a specific OSD revision -# OSD_REF= ./scripts/ppl-lint-rule-validation.sh +# OSD_REF= ./scripts/ppl-lint-rule-validation.sh # -# # Skip one half +# # Skip one half (detector needs the backend artifacts to exist already) # SKIP_BACKEND=1 ./scripts/ppl-lint-rule-validation.sh -# SKIP_FRONTEND=1 ./scripts/ppl-lint-rule-validation.sh +# SKIP_DETECTOR=1 ./scripts/ppl-lint-rule-validation.sh # -# # Run the full nightly corpus (runtime-only + advisory rules + coverage) +# # Run the full nightly corpus (all rules + coverage assertion) # PPL_LINT_SCHEDULE=nightly ./scripts/ppl-lint-rule-validation.sh set -euo pipefail @@ -37,23 +46,37 @@ OSD_REPO_URL="${OSD_REPO_URL:-https://github.com/opensearch-project/OpenSearch-D OSD_REF="${OSD_REF:-main}" DEFAULT_OSD_CHECKOUT="$SQL_ROOT/.ci/OpenSearch-Dashboards" CONTRACT_DIR="$SQL_ROOT/integ-test/src/test/resources/ppl-lint/contracts" -FRONTEND_SCRIPT="$SQL_ROOT/scripts/ppl-lint/run-frontend-contract.mjs" +DETECTOR_SCRIPT="$SQL_ROOT/scripts/ppl-lint/run-frontend-contract.mjs" IT_CLASS="org.opensearch.sql.calcite.remote.PplLintRuleValidationIT" # pr (fast, blocking subset) or nightly (full corpus + coverage assertion). PPL_LINT_SCHEDULE="${PPL_LINT_SCHEDULE:-pr}" +# Candidate artifacts the backend half exports and the detector half consumes. +GRAMMAR_BUNDLE="$SQL_ROOT/ppl-grammar-bundle.json" +TARGET_MANIFEST="$SQL_ROOT/target.json" +BACKEND_REPORT="$SQL_ROOT/backend-report.json" +DETECTOR_REPORT="$SQL_ROOT/detector-report.json" + log() { echo "[ppl-lint-rule-validation] $*"; } -resolve_opensearch_version() { - local raw - raw=$(grep -oE '"opensearch.version", "[^"]+"' build.gradle | head -1 | - sed -E 's/.*"opensearch.version", "([^"]+)"/\1/') - echo "${raw%%-*}" +run_backend() { + log "Running backend integration test: $IT_CLASS (schedule=$PPL_LINT_SCHEDULE)" + ./gradlew :integ-test:integTest --tests "$IT_CLASS" \ + -Dppl.lint.schedule="$PPL_LINT_SCHEDULE" \ + -Dppl.lint.report="$BACKEND_REPORT" \ + -Dppl.lint.grammar.bundle="$GRAMMAR_BUNDLE" \ + -Dppl.lint.target="$TARGET_MANIFEST" + log "Backend integration test passed. Exported: $(basename "$GRAMMAR_BUNDLE"), $(basename "$TARGET_MANIFEST")." } -run_frontend() { +run_detector() { local osd_checkout="$1" + if [[ ! -f "$GRAMMAR_BUNDLE" ]]; then + log "ERROR: $GRAMMAR_BUNDLE not found. Run the backend half first (do not set SKIP_BACKEND=1)." + exit 2 + fi + if [[ ! -d "$osd_checkout/node_modules" ]]; then log "Bootstrapping OSD at $osd_checkout (this can take a while)..." (cd "$osd_checkout" && yarn osd bootstrap) @@ -61,20 +84,27 @@ run_frontend() { log "Reusing bootstrapped OSD at $osd_checkout (node_modules present)." fi - local os_version - os_version="$(resolve_opensearch_version)" - log "Running frontend contract against OSD analyzer (PPL_SQL_VERSION=$os_version, schedule=$PPL_LINT_SCHEDULE)..." + log "Running detector validation against the candidate bundle (schedule=$PPL_LINT_SCHEDULE)..." ( cd "$osd_checkout" PPL_LINT_CONTRACT_DIR="$CONTRACT_DIR" \ PPL_LINT_SCHEDULE="$PPL_LINT_SCHEDULE" \ - PPL_SQL_VERSION="$os_version" \ - PPL_LINT_REPORT="$SQL_ROOT/frontend-report.json" \ - node -r ./src/setup_node_env "$FRONTEND_SCRIPT" + PPL_LINT_GRAMMAR_BUNDLE="$GRAMMAR_BUNDLE" \ + PPL_LINT_TARGET_MANIFEST="$TARGET_MANIFEST" \ + PPL_LINT_BACKEND_REPORT="$BACKEND_REPORT" \ + PPL_LINT_REPORT="$DETECTOR_REPORT" \ + node -r ./src/setup_node_env "$DETECTOR_SCRIPT" ) + log "Detector validation passed." } -if [[ "${SKIP_FRONTEND:-0}" != "1" ]]; then +if [[ "${SKIP_BACKEND:-0}" != "1" ]]; then + run_backend +else + log "SKIP_BACKEND=1 — skipping the SQL backend integration test (using existing artifacts)." +fi + +if [[ "${SKIP_DETECTOR:-0}" != "1" ]]; then if [[ -n "${OSD_SOURCE_PATH:-}" ]]; then OSD_CHECKOUT="$(cd "$OSD_SOURCE_PATH" && pwd)" log "Using existing OSD checkout: $OSD_CHECKOUT" @@ -95,20 +125,9 @@ if [[ "${SKIP_FRONTEND:-0}" != "1" ]]; then OSD_SHA="$(git -C "$OSD_CHECKOUT" rev-parse HEAD)" log "OSD revision under test: $OSD_SHA" - run_frontend "$OSD_CHECKOUT" - log "Frontend contract passed." -else - log "SKIP_FRONTEND=1 — skipping the OSD frontend contract." -fi - -if [[ "${SKIP_BACKEND:-0}" != "1" ]]; then - log "Running backend integration test: $IT_CLASS (schedule=$PPL_LINT_SCHEDULE)" - ./gradlew :integ-test:integTest --tests "$IT_CLASS" \ - -Dppl.lint.schedule="$PPL_LINT_SCHEDULE" \ - -Dppl.lint.report="$SQL_ROOT/backend-report.json" - log "Backend integration test passed." + run_detector "$OSD_CHECKOUT" else - log "SKIP_BACKEND=1 — skipping the SQL backend integration test." + log "SKIP_DETECTOR=1 — skipping the OSD detector contract." fi log "Done." diff --git a/scripts/ppl-lint/README.md b/scripts/ppl-lint/README.md new file mode 100644 index 00000000000..f17a428b959 --- /dev/null +++ b/scripts/ppl-lint/README.md @@ -0,0 +1,195 @@ +# PPL lint rule validation + +A required, cross-repository GitHub Actions check that proves the OpenSearch +Dashboards (OSD) PPL lint detectors and the SQL backend still agree — on the +**same candidate runtime grammar** built by a SQL pull request. + +PPL language behavior lives in SQL; PPL lint detectors live in OSD. A SQL change +can silently invalidate an OSD rule (a parser refactor stops a detector matching, +or a semantic change makes a flagged query valid) without touching OSD. Neither +repository's own unit tests catch that. This check does. + +- **Design:** `ppl-lint-ci-validation-design.md` +- **Workflow:** [`.github/workflows/ppl-lint-rule-validation.yml`](../../.github/workflows/ppl-lint-rule-validation.yml) +- **Contracts:** [`integ-test/src/test/resources/ppl-lint/contracts/`](../../integ-test/src/test/resources/ppl-lint/contracts) + +## The pipeline + +Three jobs run in a line; artifacts are the only bridge between them. + +``` +backend-validation ──(target.json, ppl-grammar-bundle.json, backend-report.json)──▶ + detector-validation ──▶ validation-result (the single required check) +``` + +1. **backend-validation** (OpenSearch CI container). Builds the SQL PR, starts + the Gradle test cluster, runs each contract's trigger/control queries against + `POST /_plugins/_ppl`, and — while the cluster is alive — exports: + - `ppl-grammar-bundle.json` — the candidate runtime grammar (`GET /_plugins/_ppl/_grammar`); + - `target.json` — `{ engineVersion, grammarHash, grammarBundle }`; + - `backend-report.json` — the observed HTTP behavior per query. +2. **detector-validation** (`ubuntu-latest`). Checks out and bootstraps OSD as a + Node code dependency (no OSD server, no Monaco, no browser), then runs + [`run-frontend-contract.mjs`](run-frontend-contract.mjs). That runner + deserializes the candidate bundle through OSD's production headless lint API + (`src/plugins/data/public/antlr/opensearch_ppl/headless_ppl_lint`) and lints + each query with the **real** detectors on the **candidate** grammar. It then + asserts the detector-vs-backend differential. +3. **validation-result**. `if: always()`, `needs: [backend-validation, + detector-validation]`. Fails unless both succeeded — so a skipped detector + (because the backend failed first) still reds the check instead of looking + green. It writes the per-rule PR summary and uploads `run-manifest.json`. This + is the **only** job repo admins pin to branch protection. + +## Workflow inputs and modes + +| Trigger | Mode | OSD ref | Enforcing? | +| --- | --- | --- | --- | +| `pull_request` | SQL PR validation | `main` | **Yes** — the required check | +| `workflow_dispatch` (`osd_ref`) | OSD-branch evidence | the given commit/branch | No — pre-merge evidence only | +| `schedule` (nightly) | full corpus + coverage | `main` | No | + +`workflow_dispatch` inputs: + +- `osd_ref` — an OSD commit or branch to validate instead of `main`. Resolved to + an immutable commit SHA and recorded in the run manifest. A manual run **cannot** + satisfy branch protection; merge the OSD change first, then rerun the required + `pull_request` check against OSD `main`. +- `schedule` — `pr` (fast blocking subset) or `nightly` (full corpus). + +## Local reproduction + +From the SQL checkout: + +```bash +# Backend IT (exports the bundle) then detector check against OSD main. +./scripts/ppl-lint-rule-validation.sh + +# Reuse an already-bootstrapped OSD checkout. +OSD_SOURCE_PATH=../OpenSearch-Dashboards ./scripts/ppl-lint-rule-validation.sh + +# Reproduce a specific CI run's OSD revision (from run-manifest.json). +OSD_REF= ./scripts/ppl-lint-rule-validation.sh + +# Full nightly corpus + coverage assertion. +PPL_LINT_SCHEDULE=nightly ./scripts/ppl-lint-rule-validation.sh + +# Re-run only one half (detector needs the backend artifacts to exist). +SKIP_DETECTOR=1 ./scripts/ppl-lint-rule-validation.sh +SKIP_BACKEND=1 ./scripts/ppl-lint-rule-validation.sh +``` + +The backend half writes `ppl-grammar-bundle.json`, `target.json`, and +`backend-report.json` to the SQL repo root; the detector half consumes them and +writes `detector-report.json`. + +### Runner environment contract + +`run-frontend-contract.mjs` is run from inside the OSD checkout with +`node -r ./src/setup_node_env` and reads: + +| Env var | Meaning | +| --- | --- | +| `PPL_LINT_CONTRACT_DIR` | directory of `*.spec.json` + `manifest.json` | +| `PPL_LINT_SCHEDULE` | `pr` or `nightly` | +| `PPL_LINT_GRAMMAR_BUNDLE` | candidate `ppl-grammar-bundle.json` (required; no compiled fallback) | +| `PPL_LINT_TARGET_MANIFEST` | `target.json` (engine version + grammar hash) | +| `PPL_LINT_BACKEND_REPORT` | `backend-report.json` (enables the differential) | +| `PPL_LINT_REPORT` | where to write `detector-report.json` | +| `PPL_LINT_CONTRACT_FILE` | (optional) run a single spec instead of the dir | + +## Contract format (schema v3) + +One JSON file per rule under `contracts/`, listed in `manifest.json`. Each file +has a top-level `queries` map (each `{ role: "trigger"|"control", query }`) and a +version-scoped `expectations[]`. Exactly one expectation must match the candidate +backend version (zero or more than one fails before any query runs). + +```jsonc +{ + "schemaVersion": 3, + "ruleId": "union-min-datasets", + "grammarSurface": "runtime-bundle", + "schedule": "pr", + "wiring": { "detector": "union-min-datasets", "enabled": true, "severity": "error", ... }, + "backendFixture": { "indices": ["ACCOUNT"], "clusterSettings": { "calcite": true, "calciteFallback": false } }, + "frontendContext": { "isCalcite": true }, + "index": "opensearch-sql_test_index_account", + "queries": { + "union-single-dataset": { "role": "trigger", "query": "| union [ source={{index}} ]" }, + "union-two-datasets-control": { "role": "control", "query": "| union [ source={{index}} ] [ source={{index}} ]" } + }, + "expectations": [ + { + "version": ">=3.7.0", + "engine": "calcite", + "queries": { + "union-single-dataset": { + "detectorCount": 1, "severity": "error", + "backend": { "kind": "rejection", "httpStatus": 400, "body": { "status": 400, "error": { "type": "IllegalArgumentException" } } } + }, + "union-two-datasets-control": { + "detectorCount": 0, + "backend": { "kind": "result-shape", "httpStatus": 200, "expect": { "datarowsNonEmpty": true } } + } + } + } + ] +} +``` + +`backend.kind` is one of `rejection` (contracted 4xx + error type/reason), +`result-shape` (200 with datarow expectations), or `advisory` (soft 200-only +oracle). When a behavior changes in a new version, keep **both** version-scoped +expectations so the nightly matrix proves the rule still fires on the old version +while the candidate check proves the fix on the new one. + +### Pitfall: do not write pipe-first (`| command …`) trigger queries + +The detector half and the backend half must run the **byte-identical** query +(design's "Same queries" requirement). OSD's runtime lint path prepends a +synthetic `source=t ` prefix to any query that starts with a pipe, so linting +`| union [ source=idx ]` actually parses `source=t | union [ source=idx ]` — a +valid *mid-pipeline* union whose implicit upstream dataset makes the detector +stay silent. The backend, receiving the raw pipe-first query, still rejects it. +The two halves then disagree even though nothing is wrong. Write triggers in a +**query-initial** form (`union [ source=idx ]`, `multisearch [ search source=idx ]`) +that both sides accept verbatim. Until SQL emits `pipeStartRuleIndex` in the +grammar bundle (design §6, D-pipe), a pipe-first trigger with a distinct start +rule cannot be validated end to end. + +### The enforced set + +`manifest.json` partitions the corpus: + +- `enforced` — reviewed error rules with a deterministic backend rejection and a + valid negative control. These block `validation-result`. Phase one: + `unsupported-window-function-in-eventstats`, `multisearch-min-subsearch`, + `union-min-datasets`, `replace-wildcard-asymmetry`. +- `pendingReview` — error rules awaiting Peng/Chen usefulness review before + joining `enforced` (currently `field-validation`). +- `nonEnforcing` — warning/info/advisory/result-shape rules. They run on the + nightly schedule for coverage and never block a PR. + +## Interpreting a failure + +| Failure | Meaning | +| --- | --- | +| Grammar bundle export fails | The candidate SQL build does not provide a usable runtime grammar. | +| Trigger no longer parses | The grammar changed ownership of the error or regressed. | +| Detector emits no diagnostic | The detector is incompatible with the candidate parse tree. | +| Detector flags the control | The detector became too broad. | +| Backend accepts the trigger | The lint rule's premise may be fixed or stale. | +| Backend rejects the control | Query, fixture, settings, or SQL behavior regressed. | +| No version expectation matches | The rule test does not cover the candidate version. | + +CI never rewrites expected results. A behavior change is an intentional, reviewed +edit to a versioned expectation **and** the corresponding OSD rule. If a SQL +change depends on an OSD rule update, merge the OSD change first, then rerun the +required SQL check against OSD `main`. + +## Artifacts + +Every run uploads: `run-manifest.json` (exact SQL SHA, OSD SHA, mode, backend +version, grammar hash, selected validation set), the candidate grammar bundle, +the backend and detector reports, the committed contracts used, and the job logs. diff --git a/scripts/ppl-lint/assemble-run-manifest.mjs b/scripts/ppl-lint/assemble-run-manifest.mjs new file mode 100644 index 00000000000..4cc80fe9513 --- /dev/null +++ b/scripts/ppl-lint/assemble-run-manifest.mjs @@ -0,0 +1,157 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Assemble the PPL lint validation run manifest and the compact per-rule PR + * summary in the result job (design §3.3, §4.4, T10). + * + * Inputs (env, all optional so a partial run still produces a manifest): + * SQL_SHA, OSD_REF, OSD_SHA, EVENT_NAME, SCHEDULE, + * BACKEND_RESULT, DETECTOR_RESULT, GITHUB_STEP_SUMMARY. + * Artifact files under ./artifacts (downloaded from both jobs): + * target.json (engineVersion + grammarHash), backend-report.json, + * detector-report.json. + * + * Outputs: + * run-manifest.json in the workspace root; a markdown table appended to + * $GITHUB_STEP_SUMMARY. + */ + +import fs from 'fs'; +import path from 'path'; + +const ARTIFACTS = 'artifacts'; + +function readJson(file) { + try { + if (fs.existsSync(file)) { + return JSON.parse(fs.readFileSync(file, 'utf8')); + } + } catch (error) { + // eslint-disable-next-line no-console + console.error(`[ppl-lint-manifest] could not parse ${file}: ${error.message}`); + } + return undefined; +} + +function main() { + const target = readJson(path.join(ARTIFACTS, 'target.json')) || {}; + const detector = readJson(path.join(ARTIFACTS, 'detector-report.json')) || {}; + const backend = readJson(path.join(ARTIFACTS, 'backend-report.json')) || []; + + const eventName = process.env.EVENT_NAME || ''; + const osdRef = process.env.OSD_REF || 'main'; + const mode = + eventName === 'pull_request' + ? 'sql-pr-validation' + : eventName === 'schedule' + ? 'nightly' + : osdRef && osdRef !== 'main' + ? 'osd-branch-evidence' + : 'manual'; + + const backendResult = process.env.BACKEND_RESULT || 'unknown'; + const detectorResult = process.env.DETECTOR_RESULT || 'unknown'; + const passed = backendResult === 'success' && detectorResult === 'success'; + + // The selected validation set is the set of rules the detector run actually + // evaluated (post schedule filtering). + const validationSet = Array.from( + new Set((detector.results || []).map((r) => r.ruleId)) + ).sort(); + + const manifest = { + mode, + // A workflow_dispatch osd_ref run is pre-merge evidence, never a + // branch-protection result (design §4.1.1, T11). + requiredCheck: eventName === 'pull_request', + event: eventName, + schedule: process.env.SCHEDULE || detector.schedule || 'pr', + sqlSha: process.env.SQL_SHA || '', + osdRef, + osdSha: process.env.OSD_SHA || '', + engineVersion: target.engineVersion || detector.engineVersion || '', + grammarHash: target.grammarHash || detector.grammarHash || '', + differential: !!detector.differential, + validationSet, + result: { + backend: backendResult, + detector: detectorResult, + passed, + }, + }; + + fs.writeFileSync('run-manifest.json', JSON.stringify(manifest, null, 2)); + + writeSummary(manifest, detector, backend); +} + +/** Compact per-rule PR summary: Rule | Version | Grammar | Detector | Backend | Result. */ +function writeSummary(manifest, detector, backend) { + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (!summaryPath) { + return; + } + + const backendByKey = new Map(); + for (const e of Array.isArray(backend) ? backend : []) { + backendByKey.set(`${e.ruleId}::${e.queryName}`, e); + } + + const shortHash = (h) => (h ? String(h).replace(/^sha256:/, '').slice(0, 12) : '—'); + + const lines = []; + lines.push('## PPL lint rule validation'); + lines.push(''); + lines.push(`- Mode: \`${manifest.mode}\`${manifest.requiredCheck ? ' (required)' : ' (non-enforcing)'}`); + lines.push(`- SQL: \`${manifest.sqlSha || '—'}\``); + lines.push(`- OSD: \`${manifest.osdSha || '—'}\` (ref \`${manifest.osdRef}\`)`); + lines.push(`- Backend version: \`${manifest.engineVersion || '—'}\``); + lines.push(`- Grammar: \`${shortHash(manifest.grammarHash)}\``); + lines.push( + `- Result: backend **${manifest.result.backend}**, detector **${manifest.result.detector}** → ` + + `**${manifest.result.passed ? 'PASS' : 'FAIL'}**` + ); + lines.push(''); + lines.push('| Rule | Query | Version | Grammar | Detector | Backend | Result |'); + lines.push('| ---- | ----- | ------- | ------- | -------- | ------- | ------ |'); + + for (const r of detector.results || []) { + const be = backendByKey.get(`${r.ruleId}::${r.queryName}`); + const detectorCell = `${r.actual}/${r.expected}${r.severities && r.severities.length ? ` (${r.severities.join(',')})` : ''}`; + const backendCell = be + ? be.rejected + ? `HTTP ${be.observed ? be.observed.httpStatus : '4xx'}` + : 'accepted' + : '—'; + const ok = + r.actual === r.expected && (!be || (r.role === 'trigger' ? be.rejected : !be.rejected)); + lines.push( + `| \`${r.ruleId}\` | \`${r.queryName}\` | \`${manifest.engineVersion || '—'}\` | ` + + `\`${shortHash(manifest.grammarHash)}\` | ${detectorCell} | ${backendCell} | ${ok ? 'Pass' : 'Fail'} |` + ); + } + + if ((detector.failures || []).length > 0) { + lines.push(''); + lines.push('

Failures'); + lines.push(''); + for (const f of detector.failures) { + lines.push(`- ${f}`); + } + lines.push(''); + lines.push('
'); + } + + lines.push(''); + try { + fs.appendFileSync(summaryPath, lines.join('\n') + '\n'); + } catch (error) { + // eslint-disable-next-line no-console + console.error(`[ppl-lint-manifest] could not write step summary: ${error.message}`); + } +} + +main(); diff --git a/scripts/ppl-lint/run-frontend-contract.mjs b/scripts/ppl-lint/run-frontend-contract.mjs index e3163e2ad44..8bd1ff5bad1 100644 --- a/scripts/ppl-lint/run-frontend-contract.mjs +++ b/scripts/ppl-lint/run-frontend-contract.mjs @@ -4,7 +4,7 @@ */ /** - * SQL-owned frontend contract adapter for the PPL lint rule validation CI. + * SQL-owned detector-validation runner for the PPL lint rule validation CI. * * This script is executed from inside an OpenSearch-Dashboards (OSD) checkout, * for example: @@ -12,56 +12,61 @@ * cd .ci/OpenSearch-Dashboards * PPL_LINT_CONTRACT_DIR= \ * PPL_LINT_SCHEDULE=pr \ - * PPL_SQL_VERSION= \ - * PPL_LINT_REPORT= \ + * PPL_LINT_GRAMMAR_BUNDLE= \ + * PPL_LINT_TARGET_MANIFEST= \ + * PPL_LINT_BACKEND_REPORT= \ + * PPL_LINT_REPORT= \ * node -r ./src/setup_node_env \ * "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" * * `node -r ./src/setup_node_env` installs OSD's process-wide auto-transpilation * hook (`@osd/optimizer`'s `registerNodeAutoTranspilation`), which transpiles - * `packages/osd-monaco/src/**` TypeScript on `require()` regardless of where the - * entry script lives. That is what lets this SQL-owned `.mjs` load the compiled - * OSD analyzer without OSD's own Jest. + * `src/plugins/**` and `packages/osd-monaco/src/**` TypeScript on `require()` + * regardless of where the entry script lives. That is what lets this SQL-owned + * `.mjs` load OSD's Node-safe headless lint API without OSD's own Jest. * - * The analyzer, catalog and detector registry are NOT re-exported from the - * `@osd/monaco` package barrel, so they are loaded via their deep module paths. - * Because this is an ES module, `require` is obtained with `createRequire`, and - * the modules are resolved against the OSD checkout root (process.cwd()) rather - * than the location of this script (which lives in the SQL repo, not OSD). - * - * This adapter is the frontend half of a schema-v2 cross-repository differential + * This is the detector half of a schema-v3 cross-repository differential * contract (see integ-test/src/test/resources/ppl-lint/contracts/*.spec.json). - * It asserts three things per rule: + * Unlike the earlier PoC — which linted with the compiled analyzer or a + * hand-rolled reparse against OSD `main`'s checked-in grammar — it lints against + * the *candidate* runtime grammar bundle the SQL backend job exported, via OSD's + * production headless API (`headless_ppl_lint`). Both halves therefore validate + * the exact same candidate grammar (design §4.3). + * + * It asserts, per contract: * 1. Wiring: the OSD catalog entry deep-equals the contract's `wiring` block, - * so a silently removed/retyped/regated detector reds the build. - * 2. Diagnostics: for each case the analyzer emits exactly the contracted - * number of `ruleId` diagnostics (the differential the backend half pins to - * live-engine behavior). - * 3. Coverage (nightly only): every enabled catalog rule has a contract file. + * so a silently removed/retyped/re-gated/re-severitied detector reds the + * build. + * 2. Detector: for the single version expectation that matches the candidate + * backend version, each query emits exactly the contracted number of + * `ruleId` diagnostics at the contracted severity. + * 3. Differential (when PPL_LINT_BACKEND_REPORT is supplied): the observed + * backend behavior for each query agrees with the observed detector output + * — a trigger the detector flags is one the backend rejected; a control the + * detector passes is one the backend accepted (design §3.2, §4.3). + * 4. Coverage (nightly only): every enabled catalog rule has a contract file. */ import fs from 'fs'; import path from 'path'; import { createRequire } from 'module'; -const RULE_MODULE = 'packages/osd-monaco/src/ppl/ppl_language_analyzer'; -const CATALOG_MODULE = 'packages/osd-monaco/src/ppl/lint/catalog'; -const DETECTOR_REGISTRY_MODULE = 'packages/osd-monaco/src/ppl/lint/detector_registry'; -const LINT_RUNNER_MODULE = 'packages/osd-monaco/src/ppl/lint/lint_runner'; -const RULE_INDEX_MODULE = 'packages/osd-monaco/src/ppl/lint/rule_index'; -const GRAMMAR_MODULE = 'packages/osd-antlr-grammar/target/index.js'; -// Explain lint lives only on OSD branches that ship the explain rule class; the -// adapter feature-detects it and skips explain cases when it is absent. -const RUN_EXPLAIN_MODULE = 'packages/osd-monaco/src/ppl/lint/explain/run_explain_lint'; +// OSD's Node-safe headless lint API (design §4.3). Deep-path module; resolved +// against the OSD checkout root, not this script's SQL-repo location. +const HEADLESS_MODULE = 'src/plugins/data/public/antlr/opensearch_ppl/headless_ppl_lint'; +// The Monaco-free engine barrel (@osd/monaco/ppl-lint) exposes the catalog; the +// detector registry is a deep import used only for the wiring registration check. +const CATALOG_MODULE = 'packages/osd-monaco/ppl-lint'; +const DETECTOR_REGISTRY_MODULE = 'packages/osd-monaco/target/ppl/lint/detector_registry.js'; function log(message) { // eslint-disable-next-line no-console - console.log(`[ppl-lint-frontend-contract] ${message}`); + console.log(`[ppl-lint-detector-contract] ${message}`); } function fatal(message) { // eslint-disable-next-line no-console - console.error(`[ppl-lint-frontend-contract] FATAL: ${message}`); + console.error(`[ppl-lint-detector-contract] FATAL: ${message}`); process.exit(2); } @@ -138,19 +143,84 @@ function loadOsd() { } }; - const { PPLLanguageAnalyzer } = resolveOsd(RULE_MODULE); + const headless = resolveOsd(HEADLESS_MODULE); const { getBundledCatalog } = resolveOsd(CATALOG_MODULE); - const { getDetector } = resolveOsd(DETECTOR_REGISTRY_MODULE); - const { runLint } = resolveOsd(LINT_RUNNER_MODULE); - const ruleIndex = resolveOsd(RULE_INDEX_MODULE); - const grammar = resolveOsd(GRAMMAR_MODULE, { optional: true }); - const explain = resolveOsd(RUN_EXPLAIN_MODULE, { optional: true }); + const registry = resolveOsd(DETECTOR_REGISTRY_MODULE, { optional: true }); + + const { deserializeBundleOrThrow, lintQueryWithBundle } = headless; + if (typeof deserializeBundleOrThrow !== 'function' || typeof lintQueryWithBundle !== 'function') { + fatal( + `Headless lint API not found in ${HEADLESS_MODULE}. ` + + `Expected exports deserializeBundleOrThrow + lintQueryWithBundle. ` + + `Is the OSD checkout on a branch that ships the headless API (design §4.3)?` + ); + } + if (typeof getBundledCatalog !== 'function') { + fatal(`getBundledCatalog not found in ${CATALOG_MODULE}.`); + } + + const getDetector = registry && registry.getDetector; + return { deserializeBundleOrThrow, lintQueryWithBundle, getBundledCatalog, getDetector, osdRoot }; +} + +/** Load the candidate grammar bundle + deserialize it once (fail loud; CI has no fallback). */ +function loadCandidateGrammar(osd) { + const bundlePath = process.env.PPL_LINT_GRAMMAR_BUNDLE; + if (!bundlePath) { + fatal( + 'PPL_LINT_GRAMMAR_BUNDLE is not set. Detector validation lints against the candidate ' + + 'runtime grammar bundle exported by the backend job; there is no compiled fallback.' + ); + } + if (!fs.existsSync(bundlePath)) { + fatal(`Candidate grammar bundle not found: ${bundlePath}`); + } + let bundle; + try { + bundle = JSON.parse(fs.readFileSync(bundlePath, 'utf8')); + } catch (error) { + fatal(`Could not parse grammar bundle ${bundlePath}: ${error.message}`); + } + try { + return osd.deserializeBundleOrThrow(bundle); + } catch (error) { + fatal(`Could not deserialize candidate grammar bundle: ${error.message}`); + } + return undefined; // unreachable +} - if (typeof PPLLanguageAnalyzer !== 'function') { - fatal(`PPLLanguageAnalyzer was not a constructor when loaded from ${RULE_MODULE}.`); +/** Read the target manifest (engineVersion + grammarHash) written beside the bundle. */ +function loadTarget() { + const targetPath = process.env.PPL_LINT_TARGET_MANIFEST; + if (targetPath && fs.existsSync(targetPath)) { + try { + return JSON.parse(fs.readFileSync(targetPath, 'utf8')); + } catch (error) { + log(`WARN: could not parse target manifest ${targetPath}: ${error.message}`); + } } + // Back-compat / local runs without a target manifest. + return { engineVersion: process.env.PPL_SQL_VERSION || '', grammarHash: '' }; +} - return { PPLLanguageAnalyzer, getBundledCatalog, getDetector, runLint, ruleIndex, grammar, explain, osdRoot }; +/** Index the backend report by `${ruleId}::${queryName}` for the differential. */ +function loadBackendReport() { + const reportPath = process.env.PPL_LINT_BACKEND_REPORT; + if (!reportPath || !fs.existsSync(reportPath)) { + return undefined; + } + let entries; + try { + entries = JSON.parse(fs.readFileSync(reportPath, 'utf8')); + } catch (error) { + log(`WARN: could not parse backend report ${reportPath}: ${error.message}`); + return undefined; + } + const byKey = new Map(); + for (const entry of Array.isArray(entries) ? entries : []) { + byKey.set(`${entry.ruleId}::${entry.queryName}`, entry); + } + return byKey; } /** Coerce "3.8.0-SNAPSHOT" / "3.8" to a comparable [major, minor, patch]. */ @@ -161,17 +231,79 @@ function parseVersion(v) { return [Number(m[1]), Number(m[2] || 0), Number(m[3] || 0)]; } -function versionGte(a, b) { - const pa = parseVersion(a); - const pb = parseVersion(b); - if (!pa || !pb) return true; // unknown → do not skip +function compareVersion(a, b) { for (let i = 0; i < 3; i++) { - if (pa[i] > pb[i]) return true; - if (pa[i] < pb[i]) return false; + if (a[i] !== b[i]) return a[i] < b[i] ? -1 : 1; + } + return 0; +} + +/** + * Test a space-separated semver range (e.g. ">=3.6.0 <3.8.0") against the + * candidate backend version. An empty range or an unknown version matches (do + * not over-filter). Mirrors PplLintRuleValidationIT.versionMatchesRange. + */ +function versionMatchesRange(range, version) { + if (!range || !range.trim()) return true; + const have = parseVersion(version); + if (!have) return true; + for (const token of range.trim().split(/\s+/)) { + let op = '='; + let ver = token; + if (token.startsWith('>=')) { + op = '>='; + ver = token.slice(2); + } else if (token.startsWith('<=')) { + op = '<='; + ver = token.slice(2); + } else if (token.startsWith('>')) { + op = '>'; + ver = token.slice(1); + } else if (token.startsWith('<')) { + op = '<'; + ver = token.slice(1); + } else if (token.startsWith('=')) { + op = '='; + ver = token.slice(1); + } + const cmp = compareVersion(have, parseVersion(ver) || [0, 0, 0]); + const ok = + (op === '>=' && cmp >= 0) || + (op === '<=' && cmp <= 0) || + (op === '>' && cmp > 0) || + (op === '<' && cmp < 0) || + (op === '=' && cmp === 0); + if (!ok) return false; } return true; } +/** + * Select the single expectation that applies to the candidate version + engine. + * Exactly one must match (design §5.3): zero means the rule test does not cover + * this version; more than one means overlapping ranges. Both fail. + */ +function selectExpectation(spec, version, isCalcite, failures) { + const expectations = spec.expectations || []; + const matches = expectations.filter((exp) => { + if (!versionMatchesRange(exp.version, version)) return false; + if (exp.engine === 'calcite' && isCalcite !== true) return false; + return true; + }); + if (matches.length === 1) { + return matches[0]; + } + const label = version || 'unknown'; + if (matches.length === 0) { + failures.push(`[${spec.ruleId}] no version expectation matches backend version ${label}.`); + } else { + failures.push( + `[${spec.ruleId}] ${matches.length} expectations match backend version ${label} (exactly one required).` + ); + } + return undefined; +} + /** * Assert the OSD catalog entry deep-equals the contract's `wiring` block. This is * the primary OSD-drift tripwire: if a detector is removed, retyped, re-gated or @@ -198,7 +330,9 @@ function checkWiring(spec, catalog, getDetector, failures) { ]; for (const [name, expected, actual] of checks) { if (expected !== undefined && expected !== actual) { - failures.push(`[${ruleId}] wiring.${name} expected ${JSON.stringify(expected)} but catalog has ${JSON.stringify(actual)}.`); + failures.push( + `[${ruleId}] wiring.${name} expected ${JSON.stringify(expected)} but catalog has ${JSON.stringify(actual)}.` + ); } } @@ -206,12 +340,14 @@ function checkWiring(spec, catalog, getDetector, failures) { const a = entry.appliesTo || {}; for (const key of ['minVersion', 'maxVersion', 'engine']) { if (wiring.appliesTo[key] !== undefined && wiring.appliesTo[key] !== a[key]) { - failures.push(`[${ruleId}] wiring.appliesTo.${key} expected ${JSON.stringify(wiring.appliesTo[key])} but catalog has ${JSON.stringify(a[key])}.`); + failures.push( + `[${ruleId}] wiring.appliesTo.${key} expected ${JSON.stringify(wiring.appliesTo[key])} but catalog has ${JSON.stringify(a[key])}.` + ); } } } - if (wiring.detector && typeof getDetector(wiring.detector) !== 'function') { + if (wiring.detector && typeof getDetector === 'function' && typeof getDetector(wiring.detector) !== 'function') { failures.push(`[${ruleId}] has no registered detector "${wiring.detector}".`); } @@ -219,16 +355,21 @@ function checkWiring(spec, catalog, getDetector, failures) { } /** - * Build the per-case lint context. Derives `fields`/`typeMap` from the - * `deriveFromMapping` block (a single source shared with the backend seeding), - * and sets an enable override for default-off rules that declare `forceEnable`. + * Build the per-contract lint context passed to `lintQueryWithBundle`. Derives + * `fields`/`typeMap` from the `deriveFromMapping` block (a single source shared + * with the backend seeding), pins `dataSourceVersion`/`knownVersion` to the + * candidate backend version so version filtering matches the backend, and sets + * an enable override for default-off rules that declare `forceEnable`. */ -function buildContext(spec, sqlVersion) { +function buildContext(spec, engineVersion) { const fc = spec.frontendContext || {}; const context = { isCalcite: fc.isCalcite !== false, - dataSourceVersion: sqlVersion, - grammarSurface: spec.grammarSurface === 'runtime-bundle' ? 'runtime-bundle' : 'compiled-simplified', + dataSourceVersion: engineVersion || undefined, + // Pin the "latest verified engine" to the candidate version rather than the + // hardcoded OSD_KNOWN_VERSION ('3.7.0'), which can mis-filter rules near a + // version boundary (design §4.3, D-version). + knownVersion: engineVersion || undefined, }; const mapping = fc.deriveFromMapping; @@ -257,99 +398,36 @@ function buildContext(spec, sqlVersion) { return context; } -/** Count diagnostics for this rule via the compiled-simplified analyzer. */ -function lintCompiled(analyzer, query, context, ruleId) { - const result = analyzer.lint(query, context); - return result.diagnostics.filter((d) => d.ruleId === ruleId); -} - -/** - * Count diagnostics for a runtime-only rule by parsing with the exported runtime - * grammar and running the detector registry directly. This exercises OSD-main's - * runtime grammar, NOT the cluster-versioned bundle production fetches, so it is - * a wiring/coverage check rather than a true cluster-grammar fidelity check. - * Returns undefined when the runtime grammar can't reach the rule on this OSD - * checkout (the rule's parser rules are absent) so the caller can skip cleanly. - */ -function lintRuntime(osd, spec, query, context, ruleId) { - const { grammar, runLint, ruleIndex } = osd; - if (!grammar || !grammar.OpenSearchPPLParser || !grammar.OpenSearchPPLLexer) { - return undefined; - } - const antlr = requireAntlr(osd.osdRoot); - if (!antlr) { - return undefined; - } - const { OpenSearchPPLLexer, OpenSearchPPLParser } = grammar; - - const runtimeMap = new Map(); - const names = OpenSearchPPLParser.ruleNames || []; - for (let i = 0; i < names.length; i++) { - runtimeMap.set(names[i], i); - } - - // The exported runtime grammar on this OSD checkout may predate the command a - // runtime-only rule keys off (union/multisearch/replace are absent on the - // legacy `opensearch_ppl` grammar). Detecting the absence here lets the caller - // record a clean skip — the wiring assertion already ran — instead of a false - // "0 diagnostics" failure. - const required = spec.requiredParserRules || []; - for (const name of required) { - if (!runtimeMap.has(name)) { - return undefined; - } - } - - const input = antlr.CharStream.fromString(query); - const lexer = new OpenSearchPPLLexer(input); - const tokenStream = new antlr.CommonTokenStream(lexer); - const parser = new OpenSearchPPLParser(tokenStream); - parser.removeErrorListeners(); - const tree = parser.root ? parser.root() : parser.pplStatement && parser.pplStatement(); - if (!tree) { - return undefined; - } - - const ruleNameToIndex = ruleIndex.createRuntimeRuleNameToIndex(runtimeMap); - - const diagnostics = runLint(tree, { - ruleNameToIndex, - dataSourceVersion: context.dataSourceVersion, - context: { ...context, grammarSurface: 'runtime-bundle' }, - }); - return diagnostics.filter((d) => d.ruleId === ruleId); -} - -let cachedAntlr; -function requireAntlr(osdRoot) { - if (cachedAntlr !== undefined) { - return cachedAntlr || undefined; - } - try { - const require = createRequire(path.join(osdRoot, 'noop.js')); - cachedAntlr = require('antlr4ng'); - } catch { - cachedAntlr = null; - } - return cachedAntlr || undefined; -} - function main() { const schedule = process.env.PPL_LINT_SCHEDULE || 'pr'; - const sqlVersion = process.env.PPL_SQL_VERSION; const reportPath = process.env.PPL_LINT_REPORT; const osd = loadOsd(); - const { PPLLanguageAnalyzer, getBundledCatalog, getDetector, osdRoot } = osd; + const { getBundledCatalog, getDetector, lintQueryWithBundle, osdRoot } = osd; const catalog = getBundledCatalog(); - const analyzer = new PPLLanguageAnalyzer(); + + const grammar = loadCandidateGrammar(osd); + const target = loadTarget(); + const engineVersion = target.engineVersion || process.env.PPL_SQL_VERSION || ''; + const backendReport = loadBackendReport(); const contracts = loadContracts(); const failures = []; - const report = { osdRoot, schedule, sqlVersion, results: [] }; + const report = { + osdRoot, + schedule, + engineVersion, + grammarHash: target.grammarHash || '', + differential: !!backendReport, + results: [], + }; log(`OSD root: ${osdRoot}`); - log(`schedule=${schedule} PPL_SQL_VERSION=${sqlVersion || '(unset)'} contracts=${contracts.length}`); + log( + `schedule=${schedule} engineVersion=${engineVersion || '(unset)'} ` + + `grammarHash=${target.grammarHash || '(unset)'} differential=${!!backendReport} ` + + `contracts=${contracts.length}` + ); for (const { file, spec } of contracts) { const ruleId = spec.ruleId; @@ -367,66 +445,100 @@ function main() { continue; } - const context = buildContext(spec, sqlVersion); - const isRuntime = context.grammarSurface === 'runtime-bundle'; - - for (const testCase of spec.cases || []) { - const query = testCase.query.split('{{index}}').join(index); - const fe = testCase.frontend || {}; - const expected = fe.diagnosticCount; + const context = buildContext(spec, engineVersion); + const expectation = selectExpectation(spec, engineVersion, context.isCalcite, failures); + if (!expectation) { + continue; + } - // Per-case version/engine gate mirrors the backend so both halves skip - // identically instead of disagreeing on a self-suppressed rule. - if (testCase.minVersionRequired && !versionGte(sqlVersion, testCase.minVersionRequired)) { - log(`SKIP ${ruleId}/${testCase.id} (needs >= ${testCase.minVersionRequired}, have ${sqlVersion || 'unknown'})`); - continue; - } - if (testCase.engineRequired === 'calcite' && context.isCalcite !== true) { - log(`SKIP ${ruleId}/${testCase.id} (needs calcite engine)`); + const queries = spec.queries || {}; + const expectedQueries = expectation.queries || {}; + for (const queryName of Object.keys(expectedQueries)) { + const queryDef = queries[queryName]; + if (!queryDef) { + failures.push(`[${ruleId}] expectation references unknown query "${queryName}".`); continue; } + const role = queryDef.role || 'trigger'; + const query = queryDef.query.split('{{index}}').join(index); + const expected = expectedQueries[queryName]; + const expectedCount = expected.detectorCount; - let matches; - if (testCase.explainFixture) { - matches = lintExplain(osd, spec, testCase, context, ruleId); - if (matches === undefined) { - log(`SKIP ${ruleId}/${testCase.id} (explain lint unavailable on this OSD checkout)`); - continue; - } - } else if (isRuntime) { - matches = lintRuntime(osd, spec, query, context, ruleId); - if (matches === undefined) { - // Runtime grammar can't reach this rule on this OSD checkout: the - // wiring assertion above still ran, so record a skip (not a failure). - log(`SKIP ${ruleId}/${testCase.id} (runtime grammar rule absent on this OSD checkout; wiring asserted)`); - report.results.push({ ruleId, caseId: testCase.id, query, expected, actual: null, skipped: 'runtime-grammar-absent' }); - continue; - } - } else { - matches = lintCompiled(analyzer, query, context, ruleId); - } - + const result = lintQueryWithBundle(query, grammar, context); + const matches = (result.diagnostics || []).filter((d) => d.ruleId === ruleId); const actual = matches.length; - const ok = actual === expected; + const ok = actual === expectedCount; - log(` ${ok ? 'PASS' : 'FAIL'} ${ruleId}/${testCase.id}: expected ${expected}, got ${actual} — ${query}`); + log( + ` ${ok ? 'PASS' : 'FAIL'} ${ruleId}/${queryName} (${role}): ` + + `expected ${expectedCount}, got ${actual} — ${query}` + ); const severityOk = - !fe.severity || actual === 0 || matches.every((m) => m.severity === fe.severity); + !expected.severity || actual === 0 || matches.every((m) => m.severity === expected.severity); const messageOk = - !fe.matchMessage || matches.some((m) => (m.message || '').includes(fe.matchMessage)); - - report.results.push({ ruleId, caseId: testCase.id, query, expected, actual, severities: matches.map((m) => m.severity) }); + !expected.matchMessage || matches.some((m) => (m.message || '').includes(expected.matchMessage)); + + const resultEntry = { + ruleId, + queryName, + role, + query, + expected: expectedCount, + actual, + severities: matches.map((m) => m.severity), + }; if (!ok) { - failures.push(`[${ruleId}/${testCase.id}] expected ${expected} "${ruleId}" diagnostic(s), got ${actual} for: ${query}`); + failures.push( + `[${ruleId}/${queryName}] expected ${expectedCount} "${ruleId}" diagnostic(s), got ${actual} for: ${query}` + ); } if (!severityOk) { - failures.push(`[${ruleId}/${testCase.id}] expected severity "${fe.severity}" for: ${query}`); + failures.push(`[${ruleId}/${queryName}] expected severity "${expected.severity}" for: ${query}`); } if (!messageOk) { - failures.push(`[${ruleId}/${testCase.id}] expected message to contain "${fe.matchMessage}" for: ${query}`); + failures.push(`[${ruleId}/${queryName}] expected message to contain "${expected.matchMessage}" for: ${query}`); } + + // Differential: the observed backend behavior must agree with the observed + // detector output through the shared contract (design §3.2, §4.3). A + // rejection-kind query the backend rejected must be one the detector flags; + // a success/advisory query the backend accepted must be one the detector + // passes. This catches drift the two halves would otherwise hide by both + // pinning to the same JSON. + if (backendReport) { + const backendKind = expected.backend && expected.backend.kind; + const expectRejected = backendKind === 'rejection'; + const be = backendReport.get(`${ruleId}::${queryName}`); + if (!be) { + failures.push(`[${ruleId}/${queryName}] no backend report entry (backend did not run this query).`); + } else { + resultEntry.backendRejected = !!be.rejected; + if (!!be.rejected !== expectRejected) { + failures.push( + `[${ruleId}/${queryName}] differential: backend ${be.rejected ? 'rejected' : 'accepted'} ` + + `but the contract's backend.kind="${backendKind}" expects ${expectRejected ? 'rejection' : 'acceptance'} for: ${query}` + ); + } + // Trigger/control cross-check against the detector's own verdict. + const detectorFlagged = actual > 0; + if (role === 'trigger' && detectorFlagged !== !!be.rejected) { + failures.push( + `[${ruleId}/${queryName}] differential: trigger detector ${detectorFlagged ? 'flagged' : 'passed'} ` + + `but backend ${be.rejected ? 'rejected' : 'accepted'} for: ${query}` + ); + } + if (role === 'control' && (detectorFlagged || be.rejected)) { + failures.push( + `[${ruleId}/${queryName}] differential: control must pass on both sides but detector ${detectorFlagged ? 'flagged' : 'passed'} ` + + `and backend ${be.rejected ? 'rejected' : 'accepted'} for: ${query}` + ); + } + } + } + + report.results.push(resultEntry); } } @@ -452,39 +564,13 @@ function main() { if (failures.length > 0) { // eslint-disable-next-line no-console - console.error(`[ppl-lint-frontend-contract] FAIL: ${failures.length} problem(s):\n- ${failures.join('\n- ')}`); + console.error( + `[ppl-lint-detector-contract] FAIL: ${failures.length} problem(s):\n- ${failures.join('\n- ')}` + ); process.exit(1); } - log(`PASS: all contracts agreed with the OSD analyzer (schedule=${schedule}).`); -} - -/** - * Explain-case handling. Loads the captured plan fixture and runs the OSD explain - * lint over it. Returns undefined when the explain rule class is not present on - * this OSD checkout (feature-detected via the optional module). - */ -function lintExplain(osd, spec, testCase, context, ruleId) { - if (!osd.explain || typeof osd.explain.runExplainLint !== 'function') { - return undefined; - } - const dir = process.env.PPL_LINT_CONTRACT_DIR; - if (!dir) { - return undefined; - } - const fixturePath = path.join(dir, testCase.explainFixture); - if (!fs.existsSync(fixturePath)) { - return undefined; - } - const plan = JSON.parse(fs.readFileSync(fixturePath, 'utf8')); - const query = testCase.query.split('{{index}}').join(spec.index); - const diagnostics = osd.explain.runExplainLint(plan, { - query, - overrides: context.overrides, - dataSourceVersion: context.dataSourceVersion, - isCalcite: context.isCalcite, - }); - return (diagnostics || []).filter((d) => d.ruleId === ruleId); + log(`PASS: all contracts agreed with the OSD detectors on the candidate bundle (schedule=${schedule}).`); } main(); From 9b799e7931973360b496320626f89be206fac0b2 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Mon, 20 Jul 2026 16:22:21 -0700 Subject: [PATCH 07/10] fix(ci): pipefail so detector runner failure is not masked by tee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detector-validation step piped the runner through `tee`, so the step took tee's (success) exit status and a real runner failure — e.g. the OSD headless module being absent on OSD main — went green as a vacuous pass. The first live PR run hit exactly this: the runner exited 2 with "Expected OSD module not found ... headless_ppl_lint", yet detector-validation and validation-result both reported success. Add `set -o pipefail` so node's non-zero exit propagates and the required check correctly reds until the OSD headless API merges. Signed-off-by: Hanyu Wei --- .github/workflows/ppl-lint-rule-validation.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index 83fc01d0734..df35a0b1569 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -224,6 +224,10 @@ jobs: PPL_LINT_BACKEND_REPORT: ${{ github.workspace }}/artifacts/backend-report.json PPL_LINT_REPORT: ${{ github.workspace }}/detector-report.json run: | + # pipefail so the runner's non-zero exit propagates through `tee` — + # otherwise the pipeline takes tee's (success) status and a real + # detector failure would go green (a vacuous pass). + set -o pipefail node -r ./src/setup_node_env \ "$GITHUB_WORKSPACE/scripts/ppl-lint/run-frontend-contract.mjs" \ | tee "$GITHUB_WORKSPACE/detector-contract.log" From eef3110d2c65903e9bd0c57760313b4eb22813f7 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Mon, 20 Jul 2026 17:39:50 -0700 Subject: [PATCH 08/10] feat(ci): allow workflow_dispatch to validate against an OSD fork detector-validation hardcoded repository: opensearch-project/OpenSearch-Dashboards, so osd_ref could only resolve commits/branches that exist upstream. An unmerged OSD change on a fork (e.g. the headless lint API before it lands on OSD main) could not be validated end to end. Add an osd_repo workflow_dispatch input (default opensearch-project/OpenSearch-Dashboards) that the OSD checkout honors, thread osd_repo through the detector job output into the run manifest + PR summary, and treat any non-upstream-main target as osd-branch-evidence (never a required check). The required pull_request run is unchanged: it still checks out upstream OSD main. Signed-off-by: Hanyu Wei --- .../workflows/ppl-lint-rule-validation.yml | 29 +++++++++++++------ scripts/ppl-lint/README.md | 7 +++++ scripts/ppl-lint/assemble-run-manifest.mjs | 7 +++-- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index df35a0b1569..367ee0976e0 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -31,9 +31,10 @@ name: PPL lint rule validation # Modes (design §3.4, §4.1.1): # - pull_request: SQL PR validation against OSD `main`. The ONLY enforcing mode; # this is what branch protection pins to. Runs the fast schedule:pr subset. -# - workflow_dispatch (osd_ref): pre-merge evidence for an unmerged OSD branch. -# Records the resolved immutable OSD commit SHA but CANNOT satisfy branch -# protection — only the pull_request run does. +# - workflow_dispatch (osd_repo + osd_ref): pre-merge evidence for an unmerged +# OSD branch, optionally on a fork (osd_repo). Records the resolved immutable +# OSD commit SHA but CANNOT satisfy branch protection — only the pull_request +# run does. # - schedule (nightly): the full corpus + a coverage assertion. on: @@ -42,6 +43,10 @@ on: - cron: '0 10 * * *' workflow_dispatch: inputs: + osd_repo: + description: OSD repository to check out (a fork, for pre-merge evidence). Defaults to opensearch-project/OpenSearch-Dashboards. + required: false + type: string osd_ref: description: OSD commit or branch to validate instead of main (pre-merge evidence only) required: false @@ -133,6 +138,7 @@ jobs: needs: backend-validation runs-on: ubuntu-latest outputs: + osd_repo: ${{ steps.osd-ref.outputs.repo }} osd_ref: ${{ steps.osd-ref.outputs.ref }} osd_sha: ${{ steps.osd-rev.outputs.sha }} schedule: ${{ steps.schedule.outputs.value }} @@ -140,14 +146,18 @@ jobs: - name: Checkout SQL pull request uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - # The required pull_request run always validates against OSD `main`; only a - # manual workflow_dispatch may target an unmerged OSD ref, and that run is - # pre-merge evidence, not a branch-protection result (design §4.1.1). + # The required pull_request run always validates against OSD `main` on the + # canonical repo; only a manual workflow_dispatch may target an unmerged OSD + # ref (and optionally a fork), and that run is pre-merge evidence, not a + # branch-protection result (design §4.1.1). - name: Resolve OSD ref id: osd-ref env: REQUESTED_REF: ${{ inputs.osd_ref }} - run: echo "ref=${REQUESTED_REF:-main}" >> "$GITHUB_OUTPUT" + REQUESTED_REPO: ${{ inputs.osd_repo }} + run: | + echo "ref=${REQUESTED_REF:-main}" >> "$GITHUB_OUTPUT" + echo "repo=${REQUESTED_REPO:-opensearch-project/OpenSearch-Dashboards}" >> "$GITHUB_OUTPUT" - name: Resolve contract schedule id: schedule @@ -173,7 +183,7 @@ jobs: - name: Checkout OpenSearch-Dashboards uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: - repository: opensearch-project/OpenSearch-Dashboards + repository: ${{ steps.osd-ref.outputs.repo }} ref: ${{ steps.osd-ref.outputs.ref }} path: .ci/OpenSearch-Dashboards @@ -184,7 +194,7 @@ jobs: run: | sha=$(git -C .ci/OpenSearch-Dashboards rev-parse HEAD) echo "sha=$sha" >> "$GITHUB_OUTPUT" - echo "OSD revision: \`$sha\` (ref: ${{ steps.osd-ref.outputs.ref }})" >> "$GITHUB_STEP_SUMMARY" + echo "OSD revision: \`$sha\` (repo: ${{ steps.osd-ref.outputs.repo }}, ref: ${{ steps.osd-ref.outputs.ref }})" >> "$GITHUB_STEP_SUMMARY" # Read the Node/Yarn toolchain from the OSD checkout rather than hardcoding # it, so an OSD toolchain bump does not silently drift this job. @@ -274,6 +284,7 @@ jobs: - name: Assemble run manifest and summary env: SQL_SHA: ${{ github.sha }} + OSD_REPO: ${{ needs.detector-validation.outputs.osd_repo }} OSD_REF: ${{ needs.detector-validation.outputs.osd_ref }} OSD_SHA: ${{ needs.detector-validation.outputs.osd_sha }} EVENT_NAME: ${{ github.event_name }} diff --git a/scripts/ppl-lint/README.md b/scripts/ppl-lint/README.md index f17a428b959..e9578517608 100644 --- a/scripts/ppl-lint/README.md +++ b/scripts/ppl-lint/README.md @@ -51,12 +51,19 @@ backend-validation ──(target.json, ppl-grammar-bundle.json, backend-report.j `workflow_dispatch` inputs: +- `osd_repo` — the OSD repository to check out, for validating an unmerged change + that lives on a fork. Defaults to `opensearch-project/OpenSearch-Dashboards`. + The `osd_ref` must exist in this repo (a purely local commit cannot be fetched). - `osd_ref` — an OSD commit or branch to validate instead of `main`. Resolved to an immutable commit SHA and recorded in the run manifest. A manual run **cannot** satisfy branch protection; merge the OSD change first, then rerun the required `pull_request` check against OSD `main`. - `schedule` — `pr` (fast blocking subset) or `nightly` (full corpus). +To validate an OSD change that is not yet merged, push it to a branch on your OSD +fork and dispatch with `osd_repo=/OpenSearch-Dashboards` and +`osd_ref=`. + ## Local reproduction From the SQL checkout: diff --git a/scripts/ppl-lint/assemble-run-manifest.mjs b/scripts/ppl-lint/assemble-run-manifest.mjs index 4cc80fe9513..840e631ef88 100644 --- a/scripts/ppl-lint/assemble-run-manifest.mjs +++ b/scripts/ppl-lint/assemble-run-manifest.mjs @@ -43,12 +43,14 @@ function main() { const eventName = process.env.EVENT_NAME || ''; const osdRef = process.env.OSD_REF || 'main'; + const osdRepo = process.env.OSD_REPO || 'opensearch-project/OpenSearch-Dashboards'; + const isUpstreamMain = osdRepo === 'opensearch-project/OpenSearch-Dashboards' && osdRef === 'main'; const mode = eventName === 'pull_request' ? 'sql-pr-validation' : eventName === 'schedule' ? 'nightly' - : osdRef && osdRef !== 'main' + : !isUpstreamMain ? 'osd-branch-evidence' : 'manual'; @@ -70,6 +72,7 @@ function main() { event: eventName, schedule: process.env.SCHEDULE || detector.schedule || 'pr', sqlSha: process.env.SQL_SHA || '', + osdRepo, osdRef, osdSha: process.env.OSD_SHA || '', engineVersion: target.engineVersion || detector.engineVersion || '', @@ -107,7 +110,7 @@ function writeSummary(manifest, detector, backend) { lines.push(''); lines.push(`- Mode: \`${manifest.mode}\`${manifest.requiredCheck ? ' (required)' : ' (non-enforcing)'}`); lines.push(`- SQL: \`${manifest.sqlSha || '—'}\``); - lines.push(`- OSD: \`${manifest.osdSha || '—'}\` (ref \`${manifest.osdRef}\`)`); + lines.push(`- OSD: \`${manifest.osdSha || '—'}\` (${manifest.osdRepo} @ \`${manifest.osdRef}\`)`); lines.push(`- Backend version: \`${manifest.engineVersion || '—'}\``); lines.push(`- Grammar: \`${shortHash(manifest.grammarHash)}\``); lines.push( From e13ff803372db1dcdc84e2171ab7cfc79a2aaf02 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Tue, 21 Jul 2026 21:50:09 -0700 Subject: [PATCH 09/10] fix(ci): resolve OSD target via repo variables so PR check can validate against the paired OSD ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detector-validation job imports OSD's headless lint API (src/plugins/data/public/antlr/opensearch_ppl/headless_ppl_lint), which is not yet on OSD main — it lives on the paired branch Hanyu-W/OpenSearch-Dashboards@ppl-lint-headless-api. The required pull_request run hardcoded opensearch-project/OpenSearch-Dashboards@main, so it failed on 'Expected OSD module not found' while the fork-targeting workflow_dispatch run passed. Resolve the OSD repo/ref in precedence order: workflow_dispatch input > OSD_REPO/OSD_REF repo variables > canonical opensearch-project/...@main. The committed default stays main; the temporary fork override lives in mutable repo variables and reverts by deleting them once the OSD PR merges. Signed-off-by: Hanyu Wei --- .../workflows/ppl-lint-rule-validation.yml | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index 367ee0976e0..de355dd2158 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -29,8 +29,14 @@ name: PPL lint rule validation # version, and grammar hash. # # Modes (design §3.4, §4.1.1): -# - pull_request: SQL PR validation against OSD `main`. The ONLY enforcing mode; -# this is what branch protection pins to. Runs the fast schedule:pr subset. +# - pull_request: SQL PR validation against the resolved OSD target. The ONLY +# enforcing mode; this is what branch protection pins to. Runs the fast +# schedule:pr subset. The committed default is `main` on the canonical repo; +# it can be overridden by the OSD_REPO/OSD_REF repo variables — see the +# "Resolve OSD ref" step. TEMPORARY: those repo variables are currently set to +# the unmerged paired OSD branch that ships the headless lint API this job +# needs; deleting them reverts to opensearch-project/...@main once that OSD PR +# merges. # - workflow_dispatch (osd_repo + osd_ref): pre-merge evidence for an unmerged # OSD branch, optionally on a fork (osd_repo). Records the resolved immutable # OSD commit SHA but CANNOT satisfy branch protection — only the pull_request @@ -146,18 +152,33 @@ jobs: - name: Checkout SQL pull request uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - # The required pull_request run always validates against OSD `main` on the - # canonical repo; only a manual workflow_dispatch may target an unmerged OSD - # ref (and optionally a fork), and that run is pre-merge evidence, not a - # branch-protection result (design §4.1.1). + # Resolve which OSD checkout the detectors run against, in precedence order: + # 1. workflow_dispatch input (osd_repo / osd_ref) — explicit manual run + # 2. repo variable (vars.OSD_REPO / vars.OSD_REF) — the override + # point; set/cleared in repo settings with no workflow edit + # 3. canonical default opensearch-project/OpenSearch-Dashboards@main + # + # The committed default is intentionally the canonical repo + `main`, so the + # file always declares that the required check validates against upstream. + # TEMPORARY OVERRIDE: the headless lint API this job imports + # (src/plugins/data/public/antlr/opensearch_ppl/headless_ppl_lint) is not yet + # on OSD `main`; it lives on the paired branch + # Hanyu-W/OpenSearch-Dashboards@ppl-lint-headless-api. Until that OSD PR + # merges, the OSD_REPO/OSD_REF repo variables are set to that branch so the + # required check validates against the OSD ref that actually ships the API. + # Deleting those two repo variables (no code change) reverts to `main`. - name: Resolve OSD ref id: osd-ref env: REQUESTED_REF: ${{ inputs.osd_ref }} REQUESTED_REPO: ${{ inputs.osd_repo }} + VAR_REF: ${{ vars.OSD_REF }} + VAR_REPO: ${{ vars.OSD_REPO }} run: | - echo "ref=${REQUESTED_REF:-main}" >> "$GITHUB_OUTPUT" - echo "repo=${REQUESTED_REPO:-opensearch-project/OpenSearch-Dashboards}" >> "$GITHUB_OUTPUT" + ref="${REQUESTED_REF:-${VAR_REF:-main}}" + repo="${REQUESTED_REPO:-${VAR_REPO:-opensearch-project/OpenSearch-Dashboards}}" + echo "ref=$ref" >> "$GITHUB_OUTPUT" + echo "repo=$repo" >> "$GITHUB_OUTPUT" - name: Resolve contract schedule id: schedule From b1fc00c69076002628641f66f324527854bd0f62 Mon Sep 17 00:00:00 2001 From: Hanyu Wei Date: Wed, 22 Jul 2026 15:16:11 -0700 Subject: [PATCH 10/10] ci(ppl-lint): document sibling-workflow reuse; harden detector job Mirror the sibling SQL Java workflows explicitly (header comment) and make the detector job more robust without changing what it validates: - Note that the workflow reuses Get-CI-Image-Tag, the OpenSearch CI container + ci-image-start-command, and the chown/su non-root Gradle pattern from sql-test-and-build-workflow.yml, and that action SHAs match the siblings. - Record measured CI cost: bootstrap (~2m13s, CPU-bound even with a warm yarn cache) dominates; the lint is ~2s. Hence no per-contract matrix, and why overlapping bootstrap with the backend job is a tracked follow-up. - Download the backend artifact AFTER bootstrap so a flaky artifact download cannot waste a completed bootstrap. - Add a bootstrap retry-with-backoff loop (mirrors the OSD build workflow). - Add timeout-minutes: 30 to both validation jobs. Signed-off-by: Hanyu Wei --- .../workflows/ppl-lint-rule-validation.yml | 46 ++++++++++++++++--- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ppl-lint-rule-validation.yml b/.github/workflows/ppl-lint-rule-validation.yml index de355dd2158..543139e3ebb 100644 --- a/.github/workflows/ppl-lint-rule-validation.yml +++ b/.github/workflows/ppl-lint-rule-validation.yml @@ -42,6 +42,25 @@ name: PPL lint rule validation # OSD commit SHA but CANNOT satisfy branch protection — only the pull_request # run does. # - schedule (nightly): the full corpus + a coverage assertion. +# +# Workflow shape deliberately mirrors the sibling SQL Java workflows so a +# maintainer sees one pattern, not a bespoke one: +# - sql-test-and-build-workflow.yml : the Get-CI-Image-Tag reusable workflow, +# the OpenSearch CI container + ci-image-start-command, and the +# `chown 1000:1000` + `su` non-root Gradle invocation (backend-validation). +# - integ-tests-with-security.yml : the report-upload-on-always() shape. +# Action SHAs are pinned to the same versions those siblings use, so dependabot +# bumps one set, not two drifting ones. +# +# CI cost (measured 2026-07-22, ~13 min wall clock): backend-validation ~5 min +# (container init ~2 min + backend IT/export ~2m50s); detector-validation ~3 min, +# of which OSD `yarn osd bootstrap` is ~2m13s and the actual lint is ~2s. The +# bootstrap dominates and is CPU-bound (it was ~2m13s even with a warm yarn +# cache), so it is NOT sharded into a per-contract matrix (that would multiply +# the 2m13s, not the 2s). Overlapping bootstrap with the backend job is a tracked +# follow-up, not done here: it would require transferring the bootstrapped OSD +# tree (multi-GB, 30+ workspace symlinks, plus built target/) between runners, +# which OSD's own CI deliberately avoids. See ~/ppl-lint-ci-fixes-impl-plan.md. on: pull_request: @@ -73,6 +92,7 @@ jobs: name: Backend validation (live /_plugins/_ppl + grammar export) needs: Get-CI-Image-Tag runs-on: ubuntu-latest + timeout-minutes: 30 container: image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} @@ -143,6 +163,7 @@ jobs: name: Detector validation (OSD headless lint on candidate bundle) needs: backend-validation runs-on: ubuntu-latest + timeout-minutes: 30 outputs: osd_repo: ${{ steps.osd-ref.outputs.repo }} osd_ref: ${{ steps.osd-ref.outputs.ref }} @@ -195,12 +216,6 @@ jobs: fi echo "value=$value" >> "$GITHUB_OUTPUT" - - name: Download backend artifacts - uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 - with: - name: ppl-lint-backend - path: artifacts - - name: Checkout OpenSearch-Dashboards uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: @@ -243,7 +258,24 @@ jobs: - name: Bootstrap OpenSearch-Dashboards working-directory: .ci/OpenSearch-Dashboards - run: yarn osd bootstrap + # Retry-with-backoff mirrors the OSD build workflow's bootstrap step; + # `yarn osd bootstrap` occasionally fails on a transient registry hiccup. + run: | + for i in 1 2 3; do + yarn osd bootstrap && exit 0 + echo "Bootstrap attempt $i failed, retrying in 10s..." + sleep 10 + done + exit 1 + + # Downloaded after bootstrap (not before): the ~2m13s bootstrap does not + # need the backend artifact — only the lint step below does — so a flaky + # artifact download cannot waste a completed bootstrap. + - name: Download backend artifacts + uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 + with: + name: ppl-lint-backend + path: artifacts - name: Run detector validation against the candidate bundle working-directory: .ci/OpenSearch-Dashboards