vinishjail97 commented on code in PR #940:
URL: https://github.com/apache/incubator-xtable/pull/940#discussion_r4049511281
##########
xtable-aws/src/test/java/org/apache/xtable/glue/table/TestDeltaGlueCatalogTableBuilder.java:
##########
@@ -92,12 +95,57 @@ void testGetUpdateTableInput() {
assertEquals(expected, output);
}
+ @Test
+ void testGetUpdateTableInputPreservesExistingStorageSettings() {
+ // Regression test: a from-scratch StorageDescriptor rebuild (like
getCreateTableRequest
+ // uses) would silently drop additionalLocations and any custom serde
parameter set directly
+ // in Glue outside of XTable -- refresh must instead build off the
existing descriptor.
+ setupCommonMocks();
+ List<String> additionalLocations =
Collections.singletonList("s3://base-path/extra-location");
+ Map<String, String> existingSerdeParams =
+ new
HashMap<>(deltaGlueCatalogTableBuilder.getSerDeParameters(TEST_DELTA_INTERNAL_TABLE));
+ existingSerdeParams.put("custom.serde.param", "custom-value");
+
+ StorageDescriptor existingStorageDescriptor =
+ StorageDescriptor.builder()
+ .columns(DELTA_GLUE_SCHEMA)
+ .location(TEST_BASE_PATH)
+ .additionalLocations(additionalLocations)
+
.inputFormat("org.apache.hadoop.hive.ql.io.parquet.MapredParquetInputFormat")
+
.outputFormat("org.apache.hadoop.hive.ql.io.parquet.MapredParquetOutputFormat")
+ .serdeInfo(
+ SerDeInfo.builder()
+ .serializationLibrary(
+
"org.apache.hadoop.hive.ql.io.parquet.serde.ParquetHiveSerDe")
+ .parameters(existingSerdeParams)
+ .build())
+ .build();
+ Table glueTable =
+ Table.builder()
+ .parameters(deltaGlueCatalogTableBuilder.getTableParameters())
+ .storageDescriptor(existingStorageDescriptor)
+ .partitionKeys(PARTITION_KEYS)
+ .build();
+
+ TableInput output =
+ deltaGlueCatalogTableBuilder.getUpdateTableRequest(
+ TEST_UPDATED_DELTA_INTERNAL_TABLE, glueTable,
TEST_CATALOG_TABLE_IDENTIFIER);
+
+ StorageDescriptor outputStorageDescriptor = output.storageDescriptor();
+ assertEquals(additionalLocations,
outputStorageDescriptor.additionalLocations());
+ assertEquals(existingSerdeParams,
outputStorageDescriptor.serdeInfo().parameters());
+ assertEquals(UPDATED_DELTA_GLUE_SCHEMA, outputStorageDescriptor.columns());
+ }
+
private StorageDescriptor getTestStorageDescriptor(List<Column> columns) {
return StorageDescriptor.builder()
.columns(columns)
.location(TEST_BASE_PATH)
+
.inputFormat("org.apache.hadoop.hive.ql.io.parquet.MapredParquetInputFormat")
Review Comment:
Thanks for the `toBuilder()` change, that reads well now. I pushed the two
cases in 9061e561 so this does not have to wait on another round trip.
- `testGetUpdateTableInputRepairsLegacyStorageDescriptor` starts from a
descriptor with no `inputFormat`, no `outputFormat` and a null
`serializationLibrary`, which is what a table registered by an earlier XTable
version looks like. It asserts refresh fills all three, and that the columns,
the location and a custom serde parameter survive.
- `testGetUpdateTableInputPopulatesMissingSerDeInfo` covers the
`existing.serdeInfo() == null` branch.
I also lifted the three Hive class names into constants, since the file
repeated the literals five times.
As a check that the new cases actually pin the behaviour, I dropped the
three format setters from `getUpdatedStorageDescriptor` locally and re-ran.
Both new tests failed on `expected: <...MapredParquetInputFormat> but was:
<null>`, while the two existing refresh tests still passed. `mvn -pl xtable-aws
test` is green with the fix in place, at 76 tests, and `spotless:check` passes.
Please take a look and shout if you would rather word any of it differently.
--
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]
