fix(snowflake): recognise query sources with leading comments

[barveP]

Summary

_parse_source in the Snowflake converter decided "query or table" with a fixed prefix check (SELECT , SELECT\n, SELECT\t, WITH …). A query that starts with a --, // or /* */ comment, uses \r\n after the keyword, is wrapped in parentheses, or has no whitespace after SELECT fell through to the relation path, which split the text on dots and emitted an uppercased database/schema/table with no warning:

source: |
  -- revenue source
  SELECT amount FROM db.schema.orders

became base_table: {database: "-- REVENUE SOURCE\nSELECT AMOUNT FROM DB", schema: SCHEMA, table: ORDERS}.

This PR:

• adds _is_query_source, which skips leading whitespace, SQL comments and opening parentheses, then matches SELECT/WITH only when the next character cannot continue an unquoted identifier. SELECT_RESULTS and SELECT$ARCHIVE stay relations; SELECT*FROM, SELECT/*c*/ and CRLF after the keyword are queries. Query text is emitted verbatim as base_table.definition, which Snowflake documents for query-backed tables.
• makes the relation path require three valid quoted or unquoted identifiers and raise OssieConversionError otherwise, so unrecognised text can no longer become a database name.

No behaviour change for existing physical-table or plain-query inputs: all 107 existing tests pass unchanged, and 23 tests are added (query shapes, relation-shaped garbage rejection, a keyword-prefixed table name and a $ identifier as controls, and one end-to-end conversion). The change applies cleanly alongside #338. The Honeydew and NVIDIA converters have the same prefix check and will get follow-up PRs referencing #376.

Related Issues

Fixes #376

Checklist

Specification

• Spec changes are included in core-spec/ and follow the existing structure
• Spec changes have been discussed on the mailing list or in a linked issue
• Breaking changes to the spec are clearly called out in the summary

Ontology

• Ontology changes in ontology/ are consistent with spec changes
• New or modified terms are defined and documented

Converters

• Converter logic in converters/ is updated to reflect spec or ontology changes
• New converters include tests under the converter's test directory

Validation

• Validation rules in validation/ are updated if the spec changed
• New validation cases are covered by tests

Documentation

• docs/ is updated to reflect any user-facing changes
• New features or behaviors are documented with examples where appropriate
• CONTRIBUTING.md is updated if the contribution process changed

Examples

• examples/ are added or updated for any new spec constructs or converter support

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

[christianeu-db]

Probably a question for @khush-bhatia but I see sqlglot used in other parts of Ossie & it supports the Snowflake dialect. I wonder if directly parsing would be a bit easier for handling edge cases when compared to regex

[barveP]

Thanks, that could make this simpler. Happy to try SQLGlot here.
@khush-bhatia, what do you think?

[barveP]

tried sqlglot and pushed an update. lmk what you think.

[christianeu-db]

Looking a lot simpler - thanks for taking the feedback! I left one other comment (with some pseudocode) about the remaining regex could be removed / the parsing logic could be consolidated.

once that's in, it's worth re-pinging one of the committers (esp. one with snowflake expertise). I like the encapsulation of _is_query_source that you added - this makes the query detection logic easier to extend in the futrue

[christianeu-db]

You might be able to avoid these regex as well with an approach like the following:

def try_parse_source_relation(source_stripped):
try:
table = parse_one(source_stripped, read="snowflake", into=exp.Table)
except ParseError:
return None
if table is None or len(table.parts) != 3 or not all(isinstance(p, exp.Identifier) for p in table.parts):
return None

return {
"database": _render_identifier(table.parts[0]),
"schema": _render_identifier(table.parts[1]),
"table": _render_identifier(table.parts[2]),
}

There is one nuance that the parse_one might be permissive to trailing semi-colons (so that might be a case to check in try_parse_source_relation => return None).

This helps keep the control flow a little cleaner:

if is_source(source_stripped):
return ...

parsed_relation = try_parse_relation(source_stripped)
if parsed_relation is not None:
return parsed_relation

throw_error

(ideas from this blog on the parse/don't validate pattern)

[barveP]

thanks. took the control-flow shape in the latest commit.

i also tried parse_one(..., into=exp.Table) for the relation path. it works with four guards: trailing ; (as you noted), comments, extra args like an alias or AT (OFFSET => ...) that would otherwise be dropped silently, and isinstance(exp.Table), since a.b.c; d.e.f parses to a Block. with those it passes every test except from.public.orders and db.public.qualify

those two are the real question. sqlglot decides which keywords can be identifiers from its own ID_VAR_TOKENS list (parser doc), so it rejects from and qualify but accepts order, table, group, which snowflake reserves too (reserved-keywords). the regexes are snowflake's identifier grammar verbatim (identifiers): they check shape only and leave reserved words to snowflake

so it's partial enforcement via sqlglot, or none. i lean none because it's consistent but i'm fine either way and have the parse_one version ready (with a bump to sqlglot 30.13.0, since 30.12.0 misparses "@".public.orders).

which do we prefer?

cc: @khush-bhatia since you know the snowflake side best