Skip to content

Snowflake converter turns a query source with a leading comment into a fake table #376

Description

@barveP

If a dataset's source is a SQL query that starts with a comment, the Snowflake converter doesn't notice it's a query. It splits the text on dots, uppercases the pieces, and emits them as a physical table. No error, no warning.

Here's the smallest example. This is valid Ossie; the spec says source can be a table name or a query.

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

Running it through convert_ossie_to_snowflake gives:

base_table:
  database: "-- REVENUE SOURCE\nSELECT AMOUNT FROM DB"
  schema: SCHEMA
  table: ORDERS

The expected output is base_table: {definition: <the query>}, which is what Snowflake documents for query-backed tables.

The same thing happens with a /* */ comment, with a query wrapped in parentheses, with SELECT*FROM (no space), and with \r\n after SELECT or WITH. Reproduced on main at 50457d3.

Why

_parse_source in converters/snowflake/src/ossie_snowflake/converter.py decides "query or table" with one check:

upper.startswith(("SELECT ", "SELECT\n", "SELECT\t", "WITH ", "WITH\n", "WITH\t"))

Anything that doesn't match is assumed to be a table name. That path accepts any three dot-separated chunks without checking they're identifiers, so SQL text gets chopped up and emitted as database.schema.table.

Where else

The Honeydew converter has the same check and labels the query as a table. The NVIDIA converter has it too; its "must be a physical table" guard is bypassed by the comment, and it emits the same fake table. The Omni converter uses a word-boundary regex and raises a clear error instead, which is the right behavior when unsure.

Proposed fix

Two small changes, per converter:

  1. Skip leading whitespace, -- and /* */ comments, and opening parentheses before looking for SELECT or WITH, and match the keyword as a whole word. Keep the query text exactly as written.
  2. On the table path, require each of the three parts to be a real identifier (plain or double-quoted). If not, raise instead of uppercasing it.

This is independent of the structured source work in #109 / #173 / #338, which all keep string sources supported.

I have a patch for the Snowflake converter with tests (all existing tests pass, 18 added) and can follow up for Honeydew and NVIDIA. Happy to open the PR if this sounds right.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions