You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "barveP (via GitHub)" <gi...@apache.org> on 2026/09/10 03:43:31 UTC
[PR] fix(snowflake): recognise query sources with leading comments [ossie]
barveP opened a new pull request, #377:
URL: https://github.com/apache/ossie/pull/377
## 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:
```yaml
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` as a whole word. `SELECT_RESULTS` stays a relation; `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 18 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
- [x] Converter logic in `converters/` is updated to reflect spec or ontology changes
- [x] 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
- [x] All existing tests pass (`pytest` / CI green)
- [x] New functionality is covered by tests
### Compliance
- [x] ASF license headers are present on all new source files
- [x] No third-party dependencies are added without PMC/IPMC approval
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(snowflake): recognise query sources with leading comments [ossie]
Posted by "barveP (via GitHub)" <gi...@apache.org>.
barveP commented on code in PR #377:
URL: https://github.com/apache/ossie/pull/377#discussion_r3983139622
##########
converters/snowflake/src/ossie_snowflake/converter.py:
##########
@@ -421,6 +422,34 @@ def _normalize_identifier(identifier):
return stripped
return stripped.upper()
+# A dataset source is a SQL query when, after leading whitespace, SQL comments
+# (`-- ...`, `// ...` and `/* ... */`) and any opening parentheses, it starts
+# with SELECT or WITH followed by something that cannot continue an unquoted
+# identifier. That keeps names such as SELECT_RESULTS or SELECT$ARCHIVE on the
+# relation path (Snowflake allows `$` in unquoted identifiers, so `\b` would be
+# wrong), while `SELECT*FROM`, `SELECT/*c*/` and CRLF after the keyword are
+# still recognised as queries.
+_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'^"(?:[^"]|"")+"$')
Review Comment:
Thanks, that could make this simpler. Happy to try SQLGlot here.
@khush-bhatia, what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(snowflake): recognise query sources with leading comments [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on code in PR #377:
URL: https://github.com/apache/ossie/pull/377#discussion_r4033859038
##########
converters/snowflake/src/ossie_snowflake/converter.py:
##########
@@ -421,6 +425,30 @@ def _normalize_identifier(identifier):
return stripped
return stripped.upper()
+_UNQUOTED_IDENTIFIER = re.compile(r"^[A-Za-z_][A-Za-z0-9_$]*$")
+_QUOTED_IDENTIFIER = re.compile(r'^"(?:[^"]|"")+"$')
Review Comment:
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](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) on the parse/don't validate pattern)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(snowflake): recognise query sources with leading comments [ossie]
Posted by "barveP (via GitHub)" <gi...@apache.org>.
barveP commented on code in PR #377:
URL: https://github.com/apache/ossie/pull/377#discussion_r4012889769
##########
converters/snowflake/src/ossie_snowflake/converter.py:
##########
@@ -421,6 +422,34 @@ def _normalize_identifier(identifier):
return stripped
return stripped.upper()
+# A dataset source is a SQL query when, after leading whitespace, SQL comments
+# (`-- ...`, `// ...` and `/* ... */`) and any opening parentheses, it starts
+# with SELECT or WITH followed by something that cannot continue an unquoted
+# identifier. That keeps names such as SELECT_RESULTS or SELECT$ARCHIVE on the
+# relation path (Snowflake allows `$` in unquoted identifiers, so `\b` would be
+# wrong), while `SELECT*FROM`, `SELECT/*c*/` and CRLF after the keyword are
+# still recognised as queries.
+_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'^"(?:[^"]|"")+"$')
Review Comment:
tried sqlglot and pushed an update. lmk what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(snowflake): recognise query sources with leading comments [ossie]
Posted by "barveP (via GitHub)" <gi...@apache.org>.
barveP commented on code in PR #377:
URL: https://github.com/apache/ossie/pull/377#discussion_r4044608449
##########
converters/snowflake/src/ossie_snowflake/converter.py:
##########
@@ -421,6 +425,30 @@ def _normalize_identifier(identifier):
return stripped
return stripped.upper()
+_UNQUOTED_IDENTIFIER = re.compile(r"^[A-Za-z_][A-Za-z0-9_$]*$")
+_QUOTED_IDENTIFIER = re.compile(r'^"(?:[^"]|"")+"$')
Review Comment:
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](https://github.com/tobymao/sqlglot/blob/main/sqlglot/parser.py)), so it rejects from and `qualify` but accepts `order`, `table`, `group`, which snowflake reserves too ([reserved-keywords](https://docs.snowflake.com/en/sql-reference/reserved-keywords)). the regexes are snowflake's identifier grammar verbatim ([identifiers](https://docs.snowflake.com/en/sql-reference/identifiers-syntax)): 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(snowflake): recognise query sources with leading comments [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on code in PR #377:
URL: https://github.com/apache/ossie/pull/377#discussion_r3982111906
##########
converters/snowflake/src/ossie_snowflake/converter.py:
##########
@@ -421,6 +422,34 @@ def _normalize_identifier(identifier):
return stripped
return stripped.upper()
+# A dataset source is a SQL query when, after leading whitespace, SQL comments
+# (`-- ...`, `// ...` and `/* ... */`) and any opening parentheses, it starts
+# with SELECT or WITH followed by something that cannot continue an unquoted
+# identifier. That keeps names such as SELECT_RESULTS or SELECT$ARCHIVE on the
+# relation path (Snowflake allows `$` in unquoted identifiers, so `\b` would be
+# wrong), while `SELECT*FROM`, `SELECT/*c*/` and CRLF after the keyword are
+# still recognised as queries.
+_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'^"(?:[^"]|"")+"$')
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(snowflake): recognise query sources with leading comments [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #377:
URL: https://github.com/apache/ossie/pull/377
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org