Add config to disable S3 trailing checksum validation #3357

rishii-19-works · 2026-01-05T17:27:19Z

This PR adds a config-driven, S3-compatible way to disable AWS SDK trailing
checksum validation by wiring the setting through Iceberg’s S3FileIO properties.

Default behavior remains unchanged
No provider-specific logic
Applies to all S3-compatible object stores

Local note: Spotless passes locally. Full build fails on Windows during unrelated
JAR packaging tasks; CI on Linux should validate.

Fixes #3346

dimas-b · 2026-01-05T17:48:02Z

polaris-core/src/main/java/org/apache/polaris/core/entity/NamespaceEntity.java


  public static class Builder extends PolarisEntity.BaseBuilder<NamespaceEntity, Builder> {
+
+    private static final int MAX_NAMESPACE_DEPTH = 10;


Unrelated change?

dimas-b

Thanks for looking into #3346 , @rishii-19-works !

dimas-b · 2026-01-05T17:52:00Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java

+        properties.getOrDefault("s3.enable-trailing-checksums", "true");
+
+    if (!Boolean.parseBoolean(enableTrailingChecksums)) {
+      properties.put(S3FileIOProperties.CHECKSUM_VALIDATION_ENABLED, "false");


It is probably preferable to handle FileIO-related config via StorageAccessConfig. This way it will apply both to Polaris and clients (engines).

This change should not be necessary with the StorageAccessConfig-base solution.

rishii-19-works · 2026-01-05T17:56:14Z

Thanks for the review and feedback!

I appreciate the suggestion about handling this config through StorageAccessConfig.
I’ll take a look at that approach and see how it can be incorporated so it applies more broadly to clients/engines as well.

Please let me know if you have pointers on where you’d prefer that configuration to be passed or stored.

dimas-b · 2026-01-05T17:59:49Z

Please let me know if you have pointers on where you’d prefer that configuration to be passed or stored.

Given that the behaviour this config setting is trying to control is specific to the storage technology, I'd guess putting it into AwsStorageConfigInfo would be appropriate (cf. pathStyleAccess). This way the same sever can have catalogs using different storage without them affecting one another.

The next best choice is RealmConfig (via FeatureConfiguration).

dimas-b · 2026-01-05T18:16:24Z

@rishii-19-works : please go ahead :) I'd approve a solution that I proposed 😉

rishii-19-works · 2026-01-05T19:17:39Z

I’ve updated the PR to move the setting into AwsStorageConfigurationInfo
and apply the Iceberg-specific behavior during FileIO construction,
keeping it storage-scoped and per-catalog as discussed. Thanks!

rishii-19-works · 2026-01-05T20:25:21Z

Thanks for the guidance!
I’ve refactored the config into AwsStorageConfigurationInfo as discussed.
CI is currently failing due to some model/codegen alignment issues — I’ll dig into that and push a fix shortly.

rishii-19-works · 2026-01-06T04:28:00Z

Thanks for the review! I’ve removed the unrelated namespace-depth change and refactored the S3 checksum configuration into AwsStorageConfigurationInfo as discussed.
Local Windows builds still fail during JAR packaging due to reproducible-build permission handling, but this should pass on CI (Linux runners). Happy to make further changes if needed.

dimas-b

Please fix CI

dimas-b · 2026-01-06T19:21:48Z

polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java

-   *
-   * <p><strong>WARNING:</strong> This property is intended for testing purposes only and should not
-   * be used in production environments.
-   */


I doubt all change in this file are related to the stated purpose of this PR. Could you keep only the minimally necessary changes?

dimas-b · 2026-01-06T19:22:21Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java

+        properties.getOrDefault("s3.enable-trailing-checksums", "true");
+
+    if (!Boolean.parseBoolean(enableTrailingChecksums)) {
+      properties.put(S3FileIOProperties.CHECKSUM_VALIDATION_ENABLED, "false");


This change should not be necessary with the StorageAccessConfig-base solution.

rishii-19-works · 2026-01-07T15:42:28Z

Thanks for the review!

I’ve addressed the feedback by scoping the checksum behavior to AwsStorageConfigurationInfo, keeping DefaultFileIOFactory storage-agnostic, and reverting unrelated changes.

All CI checks are now green. Please let me know if anything else is needed.

adnanhemani

Sorry, I'm very confused here. Where are we actually using the config now? I can see in previous versions of the PR that the config was being utilized somewhere but now it's all gone...

dimas-b · 2026-01-07T22:26:16Z

polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java

+   * when unset.
+   */
+  @Nullable
+  public abstract Boolean getEnableTrailingChecksums();


This seems to be a "dangling" property 🤔 Is it possible for a user to set it? It also does not appear to be set in code 🤔 (basically what @adnanhemani commented)

dimas-b · 2026-01-07T22:27:32Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java

-    // Update with properties in case there are table-level overrides the credentials should
-    // always override table-level properties, since storage configuration will be found at
-    // whatever entity defines it
+    // Storage-level configuration should override table-level properties


These changes LGTM, but please move them into a separate PR to avoid conflating "cleanup" with feature work.

rishii-19-works · 2026-01-08T01:19:11Z

Thanks for pointing this out — you’re right.

The trailing checksum flag ended up being a dangling configuration in the current version, so I’ve removed it to keep the PR minimal and avoid introducing unused config.

The remaining change keeps FileIO wiring storage-agnostic and scoped to StorageAccessConfig, as discussed.

Happy to follow up with a separate PR if further cleanup is preferred.

dimas-b

@rishii-19-works : sorry to be direct here, but in its current state the diff of this PR does not appear to be related to the purpose stated in the description 🤷 If you're making incremental progress, please put the PR in the "draft" state for the sake of clarity.

dimas-b · 2026-01-08T16:58:12Z

polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java

  /** Flag indicating whether path-style bucket access should be forced in S3 clients. */
-  public abstract @Nullable Boolean getPathStyleAccess();
+  @Nullable
+  public abstract Boolean getPathStyleAccess();


Please use separate PRs for cleanup / typo fixes. Mixing cleanup change with feature work really complicates reviews, IMHO.

rishii-19-works · 2026-01-08T17:15:41Z

Thanks for the clarification — I’ve converted this PR to draft while I finish wiring the checksum behavior and align the diff with the stated intent. I’ll mark it ready once complete.

github-project-automation bot added this to Basic Kanban Board Jan 5, 2026

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jan 5, 2026

dimas-b reviewed Jan 5, 2026

View reviewed changes

dimas-b reviewed Jan 6, 2026

View reviewed changes

adnanhemani reviewed Jan 7, 2026

View reviewed changes

dimas-b reviewed Jan 7, 2026

View reviewed changes

dimas-b reviewed Jan 8, 2026

View reviewed changes

rishii-19-works marked this pull request as draft January 8, 2026 17:14

Add per-catalog option to disable S3 trailing checksum validation

c95f5f1

rishii-19-works force-pushed the disable-s3-trailing-checksums branch from 8fed697 to c95f5f1 Compare January 8, 2026 19:10

rishii-19-works added 3 commits January 9, 2026 01:18

Apply AWS storage config to Iceberg S3 properties

c04e9bb

Apply Spotless formatting

4ff5d66

Fix placement of AWS S3 trailing checksum configuration

efb56cb


		public static class Builder extends PolarisEntity.BaseBuilder<NamespaceEntity, Builder> {

		private static final int MAX_NAMESPACE_DEPTH = 10;

Add config to disable S3 trailing checksum validation #3357

Are you sure you want to change the base?

Add config to disable S3 trailing checksum validation #3357

Conversation

rishii-19-works commented Jan 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishii-19-works commented Jan 5, 2026

Uh oh!

dimas-b commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b commented Jan 5, 2026

Uh oh!

rishii-19-works commented Jan 5, 2026

Uh oh!

rishii-19-works commented Jan 5, 2026

Uh oh!

rishii-19-works commented Jan 6, 2026

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishii-19-works commented Jan 7, 2026

Uh oh!

adnanhemani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishii-19-works commented Jan 8, 2026

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishii-19-works commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dimas-b commented Jan 5, 2026 •

edited

Loading