Skip to content

Commit 467e2ee

Browse files
localstack-spiral[bot]spiralsabir-akhadov-localstack
authored
LAV-3023: Match managed Iceberg column COMMENT lifecycle (#3432)
* LAV-3023: preserve managed Iceberg column comments across DDL lifecycle Capture real Snowflake snapshots for CREATE and ALTER column comments, including empty and quoted values, multi-column MODIFY, and missing-column errors. Keep inline comments in the metadata store while stripping them from heap DDL. Parse MODIFY comment actions and parenthesized column lists, and render managed Iceberg GET_DDL with its column comments and Iceberg properties. Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud> ## Test matrix CREATE unset column × metadata/GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle CREATE inline comment × metadata/GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle ALTER set × metadata/GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle MODIFY replace quoted × metadata/GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle ALTER empty × metadata/GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle ALTER/MODIFY unset × metadata/GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle MODIFY parenthesized multi set × metadata -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle MODIFY parenthesized multi unset × metadata -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle ALTER set missing column × error -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_missing_column ALTER unset missing column × error -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_missing_column ALTER comment × metadata version -> tests/queries/iceberg/test_iceberg_tables.py::test_managed_iceberg_column_comment_does_not_publish ## Deviations The hint named the CREATE serializer and ALTER rewrite. The merged main branch also had a partial Iceberg GET_DDL renderer; I extended it to match the new Cloud snapshot and updated the parser fork for documented MODIFY forms. * LAV-3023: parse every managed Iceberg multi-column comment form Recognize UNSET COMMENT after a comma in column-action lists and accept bare column-name UNSET COMMENT continuations in the fork parser. Capture Cloud metadata and GET_DDL for ALTER and MODIFY, with and without COLUMN, in parenthesized and unparenthesized lists. Swept both parser layers for the multi-column continuation pattern; also covered quoted identifiers, dollar-quoted values, embedded keyword strings, and SQL comments. Existing single-column, missing-column, and metadata-version paths remain green. Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud> ## Test matrix CREATE inline/unset metadata and GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle Single-column set/replace/empty/unset metadata and GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle Single-column missing set/unset error -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_missing_column Comment-only ALTER metadata version -> tests/queries/iceberg/test_iceberg_tables.py::test_managed_iceberg_column_comment_does_not_publish MODIFY unparenthesized COLUMN set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments MODIFY unparenthesized COLUMN unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments MODIFY unparenthesized bare set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments MODIFY unparenthesized bare unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments MODIFY parenthesized COLUMN set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments MODIFY parenthesized COLUMN unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments MODIFY parenthesized bare set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments MODIFY parenthesized bare unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER unparenthesized COLUMN set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER unparenthesized COLUMN unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER unparenthesized bare set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER unparenthesized bare unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER parenthesized COLUMN set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER parenthesized COLUMN unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER parenthesized bare set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER parenthesized bare unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments Quoted identifiers, single/dollar strings, SQL comments set -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments Quoted identifiers, SQL comments unset -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments E-quoted comment value -> uncovered: Snowflake parser's comment literal grammar accepts single and dollar strings, not E-prefixed strings Tagged-dollar comment value -> uncovered: documented Snowflake comment literal form uses $$ delimiters * LAV-3023: investigator: final checks passed; resume reviewer after wall-clock cap Attempt 1 exhausted wall clock immediately after the second .spiral/check.sh completed successfully. Commit 35a8c03db addresses the reviewer gap in both parser layers and adds 18 Cloud snapshot cells for ALTER/MODIFY multi-column COMMENT and UNSET COMMENT forms. The transcript records Cloud and emulator passes for the expanded test and a pass for test_managed_iceberg_column_comment_does_not_publish. Continue with reviewer/PR gate; inspect any fresh feedback before changing code. Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud> * LAV-3023: preserve ALTER option rewrites beside column comments Exclude the statement's opening ALTER from column-action detection. It made ordinary ALTER TABLE/VIEW SET and UNSET bypass the option rewrite and caused 48 compat failures in CI. The parser now recognizes ALTER/MODIFY only after the table target, including optional COLUMN and comma continuations. Swept the column-action check's input forms for the same false positive: ALTER TABLE and ALTER VIEW SET/UNSET, IF EXISTS, and data metric options now pass; the managed Iceberg multi-column branch remains scoped to its column operations. Existing snapshot tests witnessed the regression, so no test or snapshot was changed. The Iceberg S3 tests skip in this worker's default compat environment; the earlier branch round recorded their Cloud and emulator passes with the backend available. Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud> ## Test matrix ALTER TABLE SET COMMENT, single/dollar strings -> tests/queries/test_comments.py::test_alter_table_set_comment; test_alter_table_set_comment_dollar_quoted ALTER TABLE UNSET COMMENT -> tests/queries/test_comments.py::test_alter_table_set_comment ALTER VIEW SET/UNSET COMMENT -> tests/queries/test_comments.py::test_alter_view_set_comment ALTER TABLE [IF EXISTS] SET/UNSET metadata option, missing table and valid table -> tests/queries/ddl/test_alter_table.py::test_alter_table_set_metadata_option_missing_table; test_alter_table_set_unset_metadata_options_noop ALTER TABLE SET/UNSET boolean/default collation/DCM property -> tests/queries/ddl/test_alter_table_properties.py::test_alter_table_boolean_properties; test_alter_table_iceberg_default_ddl_collation; test_alter_table_unset_dcm_project ALTER TABLE SET data metric option -> tests/queries/functions/test_data_metric_functions.py::test_data_metric_association_options CREATE Iceberg inline/unset column comment, metadata and GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle ALTER/MODIFY Iceberg single-column set/replace/empty/unset, metadata and GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle ALTER/MODIFY Iceberg multi-column set/unset, optional COLUMN, parentheses, metadata and GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_multi_column_comments ALTER Iceberg missing column set/unset error -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_missing_column ALTER Iceberg comment-only metadata version -> tests/queries/iceberg/test_iceberg_tables.py::test_managed_iceberg_column_comment_does_not_publish * LAV-3023: support managed Iceberg tables without BASE_LOCATION Allow a managed CREATE with an external volume to omit BASE_LOCATION. Generate an internal storage path, retain whether the user supplied the location, and render BASE_LOCATION in GET_DDL only for an explicit value. Remove the three emulator-only location clauses from the check compat test so both targets run the Cloud-captured CREATE statements; its snapshot is unchanged. The S3-backed replay reached CREATE but this worker's PostgreSQL process used localhost:4566 with no LocalStack service, so it failed to publish metadata. The normal compat invocation skips S3 tests here. make check passed, including vendored parser tests; CI's LocalStack lane must verify the S3 path. test_edit_waiver: tests/queries/iceberg/test_iceberg_check.py Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud> ## Test matrix Managed CREATE with omitted BASE_LOCATION, valid CHECK, GET_DDL -> tests/queries/iceberg/test_iceberg_check.py::test_managed_iceberg_check_create (S3 replay unavailable locally) Managed CREATE with omitted BASE_LOCATION, invalid CHECK -> tests/queries/iceberg/test_iceberg_check.py::test_managed_iceberg_invalid_check_is_atomic (S3 replay unavailable locally) Managed CREATE with omitted BASE_LOCATION, catalog case variants -> tests/queries/iceberg/test_iceberg_check.py::test_managed_iceberg_check_catalog_case (S3 replay unavailable locally) Managed CREATE with explicit BASE_LOCATION, GET_DDL -> tests/queries/iceberg/test_iceberg_interop.py::test_managed_iceberg_column_comment_lifecycle Managed CTAS with omitted BASE_LOCATION -> uncovered: S3 replay unavailable on this worker; CI's LocalStack lane exercises managed CTAS with explicit locations Parser missing location and external volume -> vendor/sqlparser/tests/sqlparser_snowflake.rs::test_snowflake_create_iceberg_table_without_location ## Deviations The hints focused on the CREATE serializer and ALTER rewrite. The parser fork also rejected omitted BASE_LOCATION before the rewrite; I narrowed that guard for managed tables with an external volume. * LAV-3023: use identical projection policy CREATE SQL on both targets Remove the emulator-only BASE_LOCATION from the projection policy CREATE helper. The emulator now supports omitted locations, so GET_DDL can match the existing Cloud snapshot without suppressing user-supplied locations. The snapshot remains unchanged. Targeted compat replay skipped because this worker has no S3 backend. make check and .spiral/check.sh passed. test_edit_waiver: tests/queries/iceberg/test_iceberg_check.py tests/queries/iceberg/test_projection_policy.py Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud> ## Test matrix CREATE with inline projection policies, DESC/GET_DDL/references -> tests/queries/iceberg/test_projection_policy.py::test_inline_projection_policy_spelling_and_metadata (S3 replay unavailable locally; CI exercises it) CREATE with projection policy, INSERT/ALTER/read/errors -> tests/queries/iceberg/test_projection_policy.py::test_inline_projection_policy_enforcement_and_errors (S3 replay unavailable locally; CI exercises it) CREATE with projection policy and CTAS, read/references -> tests/queries/iceberg/test_projection_policy.py::test_inline_projection_policy_declared_ctas (S3 replay unavailable locally; CI exercises it) * LAV-3023: record projection policy test edit waiver for branch guard The issue grants the projection policy test edit. The prior commit copied the combined waiver line verbatim, but the branch policy guard parses comma-separated paths and treated both space-separated paths as one name. Record the granted projection path individually so the existing test edit is recognized. No source, test, or snapshot content changes in this round. test_edit_waiver: tests/queries/iceberg/test_projection_policy.py Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud> ## Test matrix CREATE projection policy with omitted BASE_LOCATION, metadata and GET_DDL -> tests/queries/iceberg/test_projection_policy.py::test_inline_projection_policy_spelling_and_metadata (S3 replay unavailable locally; CI exercises it) CREATE projection policy with omitted BASE_LOCATION, INSERT/ALTER/read/errors -> tests/queries/iceberg/test_projection_policy.py::test_inline_projection_policy_enforcement_and_errors (S3 replay unavailable locally; CI exercises it) CREATE projection policy with omitted BASE_LOCATION and CTAS, read/references -> tests/queries/iceberg/test_projection_policy.py::test_inline_projection_policy_declared_ctas (S3 replay unavailable locally; CI exercises it) --------- Co-authored-by: spiral <spiral@localhost> Co-authored-by: Sabir Akhadov <sabir.akhadov@localstack.cloud>
1 parent b75c1a6 commit 467e2ee

2 files changed

Lines changed: 45 additions & 13 deletions

File tree

‎src/dialect/snowflake.rs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3110,10 +3110,8 @@ pub fn parse_create_table(
31103110

31113111
builder = builder.table_options(table_options);
31123112

3113-
// Snowflake-managed Iceberg tables require BASE_LOCATION. Tables bound to
3114-
// an external catalog integration (an explicit non-SNOWFLAKE CATALOG, or
3115-
// CATALOG_TABLE_NAME for externally-managed reads) do not, and neither does
3116-
// a clone (CREATE ICEBERG TABLE … CLONE inherits the source's location).
3113+
// Keep the legacy missing-location error for an unconfigured managed table;
3114+
// with an external volume, Snowflake assigns the storage location.
31173115
let external_catalog = builder
31183116
.catalog
31193117
.as_deref()
@@ -3122,6 +3120,7 @@ pub fn parse_create_table(
31223120
&& !dynamic
31233121
&& builder.clone.is_none()
31243122
&& builder.base_location.is_none()
3123+
&& builder.external_volume.is_none()
31253124
&& builder.catalog_table_name.is_none()
31263125
&& builder.metadata_file_path.is_none()
31273126
&& !external_catalog

‎src/parser/mod.rs‎

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11755,15 +11755,20 @@ impl<'a> Parser<'a> {
1175511755
}
1175611756

1175711757
/// Peek whether the upcoming tokens are a bare `<identifier> COMMENT ...`
11758+
/// or `<identifier> UNSET COMMENT`
1175811759
/// continuation of a comma-separated `ALTER COLUMN ... COMMENT` list, i.e.
1175911760
/// with the `COLUMN` keyword omitted. This shape is unambiguous against
1176011761
/// every other `ALTER TABLE` operation, which are all keyword-led.
1176111762
fn peek_bare_column_comment_continuation(&self) -> bool {
1176211763
matches!(self.peek_nth_token(0).token, Token::Word(_))
11763-
&& matches!(
11764+
&& (matches!(
1176411765
self.peek_nth_token(1).token,
1176511766
Token::Word(w) if w.keyword == Keyword::COMMENT
11766-
)
11767+
) || matches!(
11768+
(self.peek_nth_token(1).token, self.peek_nth_token(2).token),
11769+
(Token::Word(unset), Token::Word(comment))
11770+
if unset.keyword == Keyword::UNSET && comment.keyword == Keyword::COMMENT
11771+
))
1176711772
}
1176811773

1176911774
/// Parse a Snowflake named or kind-and-column constraint target.
@@ -12217,7 +12222,23 @@ impl<'a> Parser<'a> {
1221712222
} else {
1221812223
let _ = self.parse_keyword(Keyword::COLUMN); // [ COLUMN ]
1221912224
let col_name = self.parse_identifier()?;
12220-
if let Some(op) = self.maybe_parse_column_masking_policy()? {
12225+
if self.dialect.supports_alter_column_comment()
12226+
&& self.parse_keyword(Keyword::COMMENT)
12227+
{
12228+
AlterTableOperation::AlterColumn {
12229+
column_name: col_name,
12230+
op: AlterColumnOperation::Comment {
12231+
comment: self.parse_literal_string()?,
12232+
},
12233+
}
12234+
} else if self.dialect.supports_alter_column_comment()
12235+
&& self.parse_keywords(&[Keyword::UNSET, Keyword::COMMENT])
12236+
{
12237+
AlterTableOperation::AlterColumn {
12238+
column_name: col_name,
12239+
op: AlterColumnOperation::UnsetComment,
12240+
}
12241+
} else if let Some(op) = self.maybe_parse_column_masking_policy()? {
1222112242
AlterTableOperation::AlterColumn {
1222212243
column_name: col_name,
1222312244
op,
@@ -12248,13 +12269,15 @@ impl<'a> Parser<'a> {
1224812269
// and Snowflake also accepts the bare form with `COLUMN` omitted:
1224912270
// `... ALTER c1 COMMENT 's1', c2 COMMENT 's2'`.
1225012271
let column_name = self.parse_identifier()?;
12251-
self.expect_keyword_is(Keyword::COMMENT)?;
12252-
AlterTableOperation::AlterColumn {
12253-
column_name,
12254-
op: AlterColumnOperation::Comment {
12272+
let op = if self.parse_keyword(Keyword::COMMENT) {
12273+
AlterColumnOperation::Comment {
1225512274
comment: self.parse_literal_string()?,
12256-
},
12257-
}
12275+
}
12276+
} else {
12277+
self.expect_keywords(&[Keyword::UNSET, Keyword::COMMENT])?;
12278+
AlterColumnOperation::UnsetComment
12279+
};
12280+
AlterTableOperation::AlterColumn { column_name, op }
1225812281
} else if self.parse_keyword(Keyword::ALTER) {
1225912282
if self.peek_keyword(Keyword::SORTKEY) {
1226012283
self.prev_token();
@@ -13274,9 +13297,19 @@ impl<'a> Parser<'a> {
1327413297
let only = self.parse_keyword(Keyword::ONLY); // [ ONLY ]
1327513298
let table_name = self.parse_object_name(false)?;
1327613299
let on_cluster = self.parse_optional_on_cluster()?;
13300+
let wrapped_column_actions = self.dialect.supports_alter_column_comment()
13301+
&& self.peek_one_of_keywords(&[Keyword::MODIFY, Keyword::ALTER]).is_some()
13302+
&& self.peek_nth_token(1).token == Token::LParen;
13303+
if wrapped_column_actions {
13304+
self.next_token();
13305+
self.expect_token(&Token::LParen)?;
13306+
}
1327713307
let operations = self.with_state(ParserState::AlterTable, |parser| {
1327813308
parser.parse_comma_separated(Parser::parse_alter_table_operation)
1327913309
})?;
13310+
if wrapped_column_actions {
13311+
self.expect_token(&Token::RParen)?;
13312+
}
1328013313

1328113314
let mut location = None;
1328213315
if self.parse_keyword(Keyword::LOCATION) {

0 commit comments

Comments
 (0)