fix(snowflake): recognise query sources with leading comments - #377
Open
barveP wants to merge 2 commits into
Open
fix(snowflake): recognise query sources with leading comments#377barveP wants to merge 2 commits into
barveP wants to merge 2 commits into
Conversation
A dataset source that is a SQL query but does not start with the bare SELECT/WITH keyword (leading -- or /* */ comment, CRLF after the keyword, surrounding parentheses, or no whitespace as in SELECT*FROM) fell through to the relation path, which split the text on dots and emitted an uppercased bogus database/schema/table with no warning. Detect queries after skipping leading whitespace, SQL comments and opening parentheses, matching SELECT/WITH as a whole word so names such as SELECT_RESULTS stay relations. Query text is emitted verbatim as base_table.definition. The relation path now requires three valid quoted or unquoted identifiers and raises otherwise. Fixes apache#376
The keyword check used a \b word boundary, but Snowflake allows `$` in unquoted identifiers, so a valid table reference such as select$archive.public.orders was classified as a query. Match the keyword only when the next character cannot continue an unquoted identifier. Also skip Snowflake's `// ...` single-line comments before a query, and cover both cases with tests.
Comment on lines
+432
to
+437
| _LEADING_SQL_TRIVIA = re.compile( | ||
| r"^(?:\s+|--[^\n]*(?:\n|$)|//[^\n]*(?:\n|$)|/\*.*?\*/)+", re.DOTALL | ||
| ) | ||
| _QUERY_KEYWORD = re.compile(r"^(?:SELECT|WITH)(?![A-Za-z0-9_$])", re.IGNORECASE) | ||
| _UNQUOTED_IDENTIFIER = re.compile(r"^[A-Za-z_][A-Za-z0-9_$]*$") | ||
| _QUOTED_IDENTIFIER = re.compile(r'^"(?:[^"]|"")+"$') |
Contributor
There was a problem hiding this comment.
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
Author
There was a problem hiding this comment.
Thanks, that could make this simpler. Happy to try SQLGlot here.
@khush-bhatia, what do you think?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_parse_sourcein 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\nafter the keyword, is wrapped in parentheses, or has no whitespace afterSELECTfell through to the relation path, which split the text on dots and emitted an uppercaseddatabase/schema/tablewith no warning:became
base_table: {database: "-- REVENUE SOURCE\nSELECT AMOUNT FROM DB", schema: SCHEMA, table: ORDERS}.This PR:
_is_query_source, which skips leading whitespace, SQL comments and opening parentheses, then matchesSELECT/WITHonly when the next character cannot continue an unquoted identifier.SELECT_RESULTSandSELECT$ARCHIVEstay relations;SELECT*FROM,SELECT/*c*/and CRLF after the keyword are queries. Query text is emitted verbatim asbase_table.definition, which Snowflake documents for query-backed tables.OssieConversionErrorotherwise, 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
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changesConverters
converters/is updated to reflect spec or ontology changesValidation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changedExamples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green)Compliance