Skip to content

Add Apache Ossie interchange conformance - #330

Open
nicosuave wants to merge 1 commit into
semantic-catalog-foundationfrom
ossie-interchange
Open

Add Apache Ossie interchange conformance#330
nicosuave wants to merge 1 commit into
semantic-catalog-foundationfrom
ossie-interchange

Conversation

@nicosuave

Copy link
Copy Markdown
Member

Stack created with GitHub Stacks CLIGive Feedback 💬

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf5341286e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +52 to +54
"snowflake": "snowflake",
"spark": "spark",
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Support ClickHouse in the lowering dialect map

When callers lower an ANSI-only document with target_dialect="clickhouse", _sql_expression_error cannot find a configured parser and marks every field or metric invalid; strict lowering then removes all scopes. This makes Ossie lowering unusable with the ClickHouse backend that SemanticLayer explicitly supports, so map clickhouse to SQLGlot's ClickHouse dialect.

Useful? React with 👍 / 👎.

def _classify_source(source: str, source_dialect: str | None) -> tuple[str, str] | None:
dialect = _SQLGLOT_DIALECTS.get(_normalize_dialect(source_dialect)) if source_dialect else None
try:
parsed = sqlglot.parse_one(source, read=dialect)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject multiple statements in dataset sources

For a schema-valid source such as SELECT * FROM orders; DROP TABLE orders, sqlglot.parse_one classifies only the first parsed statement while the complete original string is retained in source_text and placed into the executable Model. The expression path explicitly enforces exactly one statement, but dataset sources do not, allowing trailing SQL to enter the runtime model; parse the complete source and require exactly one statement before classifying it.

Useful? React with 👍 / 👎.

dialect = _SQLGLOT_DIALECTS.get(_normalize_dialect(source_dialect)) if source_dialect else None
try:
parsed = sqlglot.parse_one(source, read=dialect)
except sqlglot.errors.ParseError:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Catch tokenizer failures during source classification

When a dataset source has malformed tokenization, such as an unterminated quoted identifier, SQLGlot raises TokenError rather than ParseError. That exception escapes _classify_source and crashes lower_ossie_document instead of producing the promised structured ossie.lowering.source_ambiguous diagnostic; catch the common SqlglotError base as the expression validator already does.

Useful? React with 👍 / 👎.

object_pairs_hook=_unique_json_object,
parse_constant=_reject_non_finite_json_number,
)
return yaml.load(text, Loader=_UniqueKeySafeLoader)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Bound YAML alias expansion

For untrusted YAML containing a compact alias-expansion graph, SafeLoader constructs shared nested objects and the later depth traversal revisits each alias occurrence without a visited-node or work budget. An acyclic graph below the 256-level limit can therefore cause exponential CPU consumption despite the 16 MiB source limit; reject aliases or enforce a bounded node/alias traversal during loading.

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant