Use INT4 type alias for CockroachDB JDBC compatibility

[sathesuraj]

Explicitly defined integer columns as INT4 in SQL script so that a CockroachDB JDBC driver correctly maps them to Java's Integer type, despite them being stored internally as INT8 in CockroachDB. This enables CockroachDB support for the metastore and will continue to work for PostgreSQL.

Fixes: #2464

[flyrain]

LGTM. Schema changes will keep happening over time. Can we add a comment that INT4 should be use as an alias for integer to keep the CockroachDB compatible?

[singhpk234]

Thank you for the change @sathesuraj !

Just to double sure so that we can call Polaris CockroachDB compatible, and protect Cocroach DB integration works in future relases (any regressions is caught), my suggestion would be to add eq class for PostgresRelationalJdbcLifeCycleManagement , mostly we will have to change the container to use cocroach container instead of pg
can be done via

    return new CockroachContainer(
        dockerImage("cockroach").asCompatibleSubstituteFor("cockroachdb/cockroach"));

may be we can refactor the base class a lit bit too, and the wiring of it to a profile would work mostly same.

Really appreciate you taking time to testing it cocroach and adding this capability to Polaris !

[singhpk234]

seems like cocroach-db uses pg driver as we have checks for the display type here
https://github.com/apache/polaris/blob/main/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java#L46

would be nice to have a comment.

[sathesuraj]

Yes, cockroach DB uses pg driver. I'll add a comment for it.

[dimas-b]

Thanks for your contribution @sathesuraj !

@singhpk234 made a good point about testing. As far as I can see current JDBC persistence tests use only H2. It is great for light-weight unit tests, but does not provide assurance that the code + schema actually work with PostgreSQL or CockroachDB.

Now that we're starting to make non-trivial schema changes, it would be nice to make tests similar to JdbcRestCatalogIT and RelationalJdbcBootstrapCommandTest but using actual PG and CockroachDB docker images. See RelationalJdbcAdminProfile for inspiration.

Ideally, it would be nice to add those tests in this PR, but if you do not have capacity ATM, I think it should be ok to merge these schema changes "as is" and add tests later.

[dimas-b]

If we do not add tests in this PR, let's keep #2464 open until we have end-to-end tests.

[github-actions]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[flyrain]

This is a nice improvement. I'd love to see this to be merged. We may also need to check the specific SQL code of Cockroach DB. The current SQL codes are all from Postgres, https://github.com/polaris-catalog/polaris/blob/main/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java#L55-L55

[sathesuraj]

This is a nice improvement. I'd love to see this to be merged. We may also need to check the specific SQL code of Cockroach DB. The current SQL codes are all from Postgres, https://github.com/polaris-catalog/polaris/blob/main/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java#L55-L55

@flyrain - Unfortunately, I could not spend time to add end-to-end tests for this change. I'll try to work on the comments and will update this PR.

[flyrain]

@sathesuraj. I'm fine with adding test in another PR by other contributors if you don't have time. Thanks a lot for the contribution!

[snazy]

I think Cockroach really deserves E2E tests.
While CR has is pg-compatible on the wire, it doesn't behave exactly like "the original PostgreSQL", which is especially true for reported SQL errors, which are IIRC inspected in the code base.