Copilot commented on code in PR #11915:
URL: https://github.com/apache/gravitino/pull/11915#discussion_r3576722231
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java:
##########
@@ -2298,6 +2299,69 @@ void testAlterCatalogProperties() throws SQLException {
metalake.dropCatalog(testCatalogName, true);
}
+ /**
+ * Verifies round-trip for new ClickHouse types (Int128/256, UInt128/256,
Enum8/16, Date32) via
+ * SQL CREATE TABLE + Gravitino loadTable. BFloat16 is excluded because it
requires ClickHouse
+ * 24.12+ (the Gravitino CI image is 24.8.14). The BFloat16 converter
mapping is verified in
+ * {@link
org.apache.gravitino.catalog.clickhouse.converter.TestClickHouseTypeConverter}.
+ */
+ @Test
+ void testCreateTableWithNewTypeMappings() {
+ String tableName = GravitinoITUtils.genRandomName("test_new_types");
+ clickhouseService.executeQuery(
+ String.format(
+ "CREATE TABLE %s.%s ("
+ + "c_int128 Int128, c_int256 Int256, "
+ + "c_uint128 UInt128, c_uint256 UInt256, "
+ + "c_enum8 Enum8('a'=1,'b'=2), "
+ + "c_enum16 Enum16('x'=1,'y'=2), "
+ + "c_date32 Date32"
+ + ") ORDER BY c_int128",
+ schemaName, tableName));
+ Table loadedTable =
+ catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName,
tableName));
+ Assertions.assertEquals(Types.ExternalType.of("Int128"),
loadedTable.columns()[0].dataType());
+ Assertions.assertEquals(Types.ExternalType.of("Int256"),
loadedTable.columns()[1].dataType());
+ Assertions.assertEquals(Types.ExternalType.of("UInt128"),
loadedTable.columns()[2].dataType());
+ Assertions.assertEquals(Types.ExternalType.of("UInt256"),
loadedTable.columns()[3].dataType());
+ // Enum types use instanceof check (not exact value match) because the
ClickHouse JDBC driver
+ // may normalize the enum definition format (e.g., spacing around '=' and
',').
+ Assertions.assertTrue(loadedTable.columns()[4].dataType() instanceof
Types.ExternalType);
+ Assertions.assertTrue(loadedTable.columns()[5].dataType() instanceof
Types.ExternalType);
+ Assertions.assertEquals(Types.ExternalType.of("Date32"),
loadedTable.columns()[6].dataType());
+ }
+
+ /**
+ * Tests that GraphiteMergeTree engine creation fails when graphite.config
property is missing.
+ * Note: Positive path test (successful creation with valid config) is not
included because it
+ * requires a pre-configured graphite_rollup element on the ClickHouse
server side, which is not
+ * available in the standard test container.
+ */
+ @Test
+ void testGraphiteMergeTreeEngineCreation() {
+ String tableName = GravitinoITUtils.genRandomName("test_graphite");
+ Column[] columns =
+ new Column[] {
+ Column.of("id", Types.IntegerType.get(), "pk", false, false,
DEFAULT_VALUE_NOT_SET),
+ Column.of("val", Types.StringType.get(), "data", true, false,
DEFAULT_VALUE_NOT_SET)
+ };
+ Map<String, String> properties = new HashMap<>();
+ properties.put(GRAVITINO_ENGINE_KEY, ENGINE.GRAPHITEMERGETREE.getValue());
+ // GraphiteMergeTree requires graphite.config property
Review Comment:
The PR changes GraphiteMergeTree metadata to require ORDER BY
(ENGINE.GRAPHITEMERGETREE now uses the (value, true, true) constructor), but
this isn’t asserted by any test. Adding a simple assertion here would cover the
regression without needing server-side graphite_rollup config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
