Add Snowflake structured data types to unmodifiable Data Types

[LonwoLonwo]

Add Snowflake structured data types in the unmodifiable Data Types list to avoid the incorrect results of the diffChangeLog command

Impact

• Bug fix (non-breaking change which fixes expected existing functionality)
• Enhancement/New feature (adds functionality without impacting existing logic)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Structured data types are compared incorrectly.

Structured data types in the Snowflake can not be created with parameters: https://docs.snowflake.com/en/sql-reference/data-types-semistructured.html

But the result of comparing tables with these types of columns with the diffChangeLog command will be

CREATE OR REPLACE TABLE TEST_SEMI_STRUCTURED(
	var VARIANT(0),
	arr ARRAY(0),
	obj OBJECT(0));

because by default Liquibase returns data type name + parameters

And the simplest way to avoid it is adding data types in the unmodifiable types list:

https://github.com/liquibase/liquibase/blob/master/liquibase-core/src/main/java/liquibase/snapshot/jvm/ColumnSnapshotGenerator.java#L435

Steps:

1. Create a table in schema1:

CREATE TABLE TEST_SEMI_STRUCTURED(
	var VARIANT,
	arr ARRAY,
	obj OBJECT);

2. Compare schema1 with an empty schema.

If I need to add tests - please, give me advice on how to do it better. Maybe like MSSQLDatabaseTest#isUnmodifiable?
If I need to create a ticket for the issue first - tell me also.

[nvoxland]

Initial/Pre-Review Thoughts

The change makes sense to me. I appreciate the detail in your PR.

There is no need for an issue for specific bugfixes like this. It can be nice to track and discuss the feature request (issue) independently from implementation options for that feature (PR) for more complex things, but for "X doesn't work unless we do Y" type bugs it's generally not worth the hasstle.

I'd tend to think this fix doesn't need a test. Tests should be reserved for "non-trivial code" especially "logic that can have bugs in it", but this basically just a declarative statement with no logic itself. The test would end up really just being "Is java able to correctly handle an array literal" which the answer is yes. We have tests elsewhere which check that "unmodifiable" values are being used correctly.

What could be useful as a test is more of an overall test that those newly-added datatypes are handled correctly, either through test-harness based tests in the liquibase-snowflake module or in the integration-tests module. But, neither are well set up for that right now on snowflake and I don't think it's worth blocking this PR for getting that going.

Questions I have:

• None

Potential risks:

• None

What could make the full review difficult:

• Nothing

[LonwoLonwo]

Thanks for the detailed response @nvoxland

[github-actions]

Unit Test Results

  5 265 files  ±0    5 265 suites  ±0   35m 19s ⏱️ +10s
  4 853 tests ±0    4 634 ✔️ ±0     219 💤 ±0  0 ❌ ±0 
62 035 runs  ±0  55 998 ✔️ ±0  6 037 💤 ±0  0 ❌ ±0 

Results for commit 97a5d7d. ± Comparison against base commit 958ae65.

♻️ This comment has been updated with latest results.

[filipelautert]

Lot's of Snowflake fixes for this release, thanks! How was it working before?

[LonwoLonwo]

@filipelautert If we talk about us, we had the Snowflake extension fork with all these fixes :)

[XDelphiGrl]

This PR prevents Liquibase from adding invalid datatype parameters to generated changesets for objects containing array, object and variant datatypes.

• Fix impacts Snowflake only.
• Functional and test harness executions passing.
• No additional testing necessary.

APPROVED