Copilot commented on code in PR #17800:
URL: https://github.com/apache/pinot/pull/17800#discussion_r2880259607
##########
pinot-plugins/pinot-file-system/pinot-adls/src/test/java/org/apache/pinot/plugin/filesystem/test/ADLSGen2PinotFSTest.java:
##########
@@ -141,6 +142,34 @@ public DataLakeFileSystemClient
getOrCreateClientWithFileSystem(DataLakeServiceC
assertTrue(sasTokenFS != null);
}
+ @Test
+ public void testChecksumEnabledWithMd5DisabledFails() {
+ PinotConfiguration pinotConfiguration = new PinotConfiguration();
+ pinotConfiguration.setProperty("authenticationType", "SAS_TOKEN");
+ pinotConfiguration.setProperty("sasToken",
"sp=rwdl&se=2025-12-31T23:59:59Z&sv=2022-11-02&sr=c&sig=test");
+ pinotConfiguration.setProperty("accountName", "testaccount");
+ pinotConfiguration.setProperty("fileSystemName", "testcontainer");
+ pinotConfiguration.setProperty("enableChecksum", "true");
+
+ ADLSGen2PinotFS adlsGen2PinotFs = new ADLSGen2PinotFS() {
+ @Override
+ public DataLakeFileSystemClient
getOrCreateClientWithFileSystem(DataLakeServiceClient serviceClient,
+ String fileSystemName) {
+ return _mockFileSystemClient;
+ }
+ };
+
+ try {
+ PinotMd5Mode.setPinotMd5Disabled(true);
+ IllegalStateException exception =
+ expectThrows(IllegalStateException.class, () ->
adlsGen2PinotFs.init(pinotConfiguration));
+ assertTrue(exception.getMessage().contains("pinot.md5.disabled"));
+ assertTrue(exception.getMessage().contains("enableChecksum"));
Review Comment:
This assertion hard-codes the property key string. Using
CommonConstants.CONFIG_OF_PINOT_MD5_DISABLED (and similarly a constant for
enableChecksum, if available) would keep the test resilient to future renames.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -2025,6 +2027,81 @@ public void testValidateInvalidDedupConfigs() {
}
}
+ @Test
+ public void testValidateUpsertConfigWithMd5Disabled() {
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME)
+ .addSingleValueDimension("myCol", FieldSpec.DataType.STRING)
+ .setPrimaryKeyColumns(Lists.newArrayList("myCol"))
+ .build();
+ UpsertConfig upsertConfig = new UpsertConfig(UpsertConfig.Mode.FULL);
+ upsertConfig.setHashFunction(HashFunction.MD5);
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.REALTIME).setTableName(TABLE_NAME)
+ .setUpsertConfig(upsertConfig)
+ .setRoutingConfig(
+ new RoutingConfig(null, null,
RoutingConfig.STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, false))
+ .setStreamConfigs(getStreamConfigs())
+ .build();
+ try {
+ PinotMd5Mode.setPinotMd5Disabled(true);
+ IllegalStateException exception =
+ expectThrows(IllegalStateException.class, () ->
TableConfigUtils.validateUpsertAndDedupConfig(tableConfig,
+ schema));
+
assertTrue(exception.getMessage().contains(CommonConstants.CONFIG_OF_PINOT_MD5_DISABLED));
+ } finally {
+ PinotMd5Mode.setPinotMd5Disabled(false);
+ }
+ }
+
+ @Test
+ public void testValidateDedupConfigWithMd5Disabled() {
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME)
+ .addSingleValueDimension("myCol", FieldSpec.DataType.STRING)
+ .setPrimaryKeyColumns(Lists.newArrayList("myCol"))
+ .build();
+ DedupConfig dedupConfig = new DedupConfig();
+ dedupConfig.setHashFunction(HashFunction.MD5);
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.REALTIME).setTableName(TABLE_NAME)
+ .setDedupConfig(dedupConfig)
+ .setRoutingConfig(
+ new RoutingConfig(null, null,
RoutingConfig.STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, false))
+ .setStreamConfigs(getStreamConfigs())
+ .build();
+ try {
+ PinotMd5Mode.setPinotMd5Disabled(true);
+ IllegalStateException exception =
+ expectThrows(IllegalStateException.class, () ->
TableConfigUtils.validateUpsertAndDedupConfig(tableConfig,
+ schema));
+
assertTrue(exception.getMessage().contains(CommonConstants.CONFIG_OF_PINOT_MD5_DISABLED));
+ } finally {
+ PinotMd5Mode.setPinotMd5Disabled(false);
+ }
+ }
+
+ @Test
+ public void testValidateDedupConfigWithMd5DisabledAllowsUpsertModeNoneMd5() {
Review Comment:
The test name implies it’s validating a DedupConfig + MD5-disabled scenario,
but the body primarily asserts that an UpsertConfig with Mode.NONE can still
carry HashFunction.MD5 without failing. Consider renaming the test to match the
behavior being verified so it’s easier to understand and search for later.
```suggestion
public void testValidateUpsertModeNoneAllowsMd5WhenMd5Disabled() {
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
