[kayemkim]

Summary

A dialect added to the core-spec Dialect enum also has to be registered in validate.py, as the sqlglot reader that parses it in DIALECT_MAP or as None plus an entry in SKIP_SQL_VALIDATION. Nothing checks that today: python-ci compares OssieDialect with the schema and validation-ci regenerates spec.yaml, but a dialect missing from DIALECT_MAP, or present there as None without a SKIP_SQL_VALIDATION entry, is parsed with sqlglot's default reader without a word, and a misspelled reader name only surfaces as a ValueError when a document in that dialect is validated. #598 was sent back for a missing table entry, and #558 and #607 at their current heads each still miss one.

Two tests pin the tables to the schema enum, the None entries that are not skipped to ANSI_SQL and OSSIE_SQL_2026, and every reader name to a dialect sqlglot knows. All of it holds on current main, and it runs on a core-spec/** change since validation-ci triggers on it, so a registration PR that skips validate.py fails there instead of in review.

The Holistics half of the earlier revision left with the converters (#523). The same two tables now live in apache/ossie-converters and get their own check there.

Verification

• validation/tests/: 249 passed, two of them new (main: 247).
• Mutations tried in place: a name added to the schema enum only, the same name added to DIALECT_MAP as None without a skip entry, a skipped dialect removed from the skip list, a skipped dialect given a reader, and a reader misspelled (bigqeury). Each fails one assertion, and the set difference names the dialect.

Related Issues

Discussion on dev@: https://lists.apache.org/thread/0tql9g8myg255hhonhbzo906rdpgstcf. No issue, the thread is the discussion.

Checklist

Validation

• New validation cases are covered by tests

Tests

• All existing tests pass (pytest / CI green)
• New functionality is covered by tests

Compliance

• ASF license headers are present on all new source files
• No third-party dependencies are added without PMC/IPMC approval

[jbonofre]

The docstring says SQLGLOT_DIALECT mirrors DIALECT_MAP and NOT_SQL mirrors the skip list, but nothing checks that. Each side is compared only to the schema enum, so the two can disagree and every test still passes. For example, changing "DATABRICKS": "databricks" to "spark" in sqlrefs.py passes all three tests here.
Could we compare them directly?

from importlib.util import module_from_spec, spec_from_file_location

import pytest


def _validate_module():
    # validate.py exits at import time without jsonschema.
    pytest.importorskip("jsonschema")
    path = snapshots.REPO_ROOT / "validation" / "validate.py"
    spec = spec_from_file_location("ossie_validate", path)
    assert spec is not None and spec.loader is not None
    module = module_from_spec(spec)
    spec.loader.exec_module(module)
    return module


def test_the_tables_match_validate_py() -> None:
    validate = _validate_module()

    assert sqlrefs.NOT_SQL == validate.SKIP_SQL_VALIDATION
    assert sqlrefs.SQLGLOT_DIALECT == {
        d: reader
        for d, reader in validate.DIALECT_MAP.items()
        if d not in validate.SKIP_SQL_VALIDATION
    }

Also, converter-holistics-ci.yml doesn't run on changes under validation/**, so a change to DIALECT_MAP alone would not run this test.

That said, as the converters will move, it's not a big deal 😄

[kayemkim]

Compared directly now in ecc467b, with validate.py loaded the way its own tests do.

[jbonofre]

This checks whether each reader is None, but not that a non-None reader is a real sqlglot dialect.
A typo like "bigqeury" passes. Then sqlglot.parse_one raises ValueError: Unknown dialect 'bigqeury', which validate_sql_expression doesn't catch, because it only catches ParseError, TokenError and RecursionError.

Could we add a separate test? Keeping it separate means the enum check still runs when sqlglot isn't installed:

def test_every_sqlglot_reader_exists() -> None:
    sqlglot = pytest.importorskip("sqlglot")

    for reader in _VALIDATE.DIALECT_MAP.values():
        if reader is not None:
            sqlglot.Dialect.get_or_raise(reader)

[kayemkim]

Added as a separate test in ecc467b, behind importorskip so the enum check keeps running without sqlglot.

[jbonofre]

The Holistics side has the same gap: a misspelled reader on SQLGLOT_DIALECT passes all three tests. sqlrefs.columns catches only ParseError, TokenError and RecursionError, so the ValueError isn't turned into UnparseableSQL.

The conversion then crashes instead of reporting an issue. sqlglot is a runtime dependency here, so a plain import works:

import sqlglot


def test_every_reader_is_a_sqlglot_dialect() -> None:
    for reader in sqlrefs.SQLGLOT_DIALECT.values():
        if reader is not None:
            sqlglot.Dialect.get_or_raise(reader)

[kayemkim]

Added in ecc467b with a plain import, since sqlglot is a runtime dependency here.

[kayemkim]

Pushed ecc467b with the three. The Holistics test now compares NOT_SQL and SQLGLOT_DIALECT with SKIP_SQL_VALIDATION and DIALECT_MAP directly, loading validate.py the way its own tests do, and both sides check every reader with Dialect.get_or_raise. The validate.py one is a separate test behind importorskip("sqlglot") so the enum check still runs without it. I tried the cases you named on this branch: "spark" for DATABRICKS, bigqeury on either side and a dropped DAX skip entry each fail now, and both suites pass.

On the validation/** trigger, agreed it can wait for the move. I'll leave the notes on #558 and #607 and open the CONTRIBUTING.md paragraph as its own PR.

[christianeu-db]

FYI: @kayemkim I put together a PR here / linked thread, which removes the validation of enums / puts the dialect table more as a community extension table rather than a core part of the spec.

Could be nice to coordinate these approaches (e.g. I think it makes sense to have a table-derived set of constants but perhaps outside the core spec

[kayemkim]

Thanks for the pointer. I read #617 and ran this branch on top of it: the two tests that read the schema enum fail with KeyError, the table cross-check and the reader tests still pass. If the enum goes, I would point those two at the table in extensions/dialects.md instead, so the registry carries the check. I put the rest on the list thread and on #617. Whichever lands first, I will do the rebase.

[jbonofre]

The converters moved to apache/ossie-converters repository, so converters/ no longer exists on main.

Could you rebase from main and drop this file?

[kayemkim]

Rebased on main after #523 and dropped the Holistics file. The same two tables now live in ossie-converters, and I will pin them there once that repository carries the schema. What stays is the validate.py side: DIALECT_MAP and the skip list against the enum, plus the check that every reader names a sqlglot dialect. 249 passed locally.

Once #617 is in, the enum comparison moves to the table in extensions/dialects.md and the two renamed sets, as said on the list.