Core: Fix for respecting custom location providers in SerializableTable #12564 by przemekd · Pull Request #14280 · apache/iceberg

przemekd · 2025-10-08T21:19:18Z

Closes #11527 in a way that does not break fixes provided by changes needed to fix #9029.

It eagerly calculates the location provider field, but wraps that into a newly implemented wrapper that only throws an error if code really needs to use that location provider. In case table needs to be read only by other processes the error is not thrown, even if there is an error with getting the location provider object.

- Add missing .hasMessage() checks to assertThatThrownBy calls - Ensures compliance with AssertThatThrownByWithMessageCheck rule

- Add .hasMessage() check to multi-line assertThatThrownBy call - Ensures full compliance with AssertThatThrownByWithMessageCheck rule

singhpk234

Thanks for the change @przemekd.

reading the issue it seems like this has been impacting multiple users, we have a 1.10.1 window open can you please post it in iceberg mailing thread for 1.10.1
https://lists.apache.org/thread/9tjs060vzs6nnghk2brcw0hv89h4drp0 for visibility.

singhpk234 · 2025-10-09T16:25:30Z

+ *
+ * @param <T> the type of the result
+ */
+public class Try<T> implements Serializable {


Have you considered vavr - https://github.com/vavr-io/vavr/tree/main/vavr it has similar utility, i think you use its getUnchecked and its apache licenced.

we might need some other community members to weigh in here too, my observation is in past, on an orthogonal context, we preferred a lib like failsafe rather than wriring something on our own.

I feel like we can be even simpler to begin with here. I poked around locally, and I do agree we need some abstraction almost like a Try monad which encapsulates either a result of some kind or a failure. I originally was going to see if we could just use an abstraction over Tasks but ultimately, we need something to capture the state that it was a failure, not just handle the failure. We could use a separate AtomicReference and a boolean for indicating that a location provider could not be resolved, but that's pretty unclean.

That said, I don't think we need a full blown public class with all these APIs, at least as of now.

I think we could just have a package private class like the following with the main write API being of(SerializableSupplier<T> supplier) and the main read API being the getOrThrow.

class Try<T> implements Serializable { public static <T> of(SerializableSupplier<T> supplier) {//does what capture does} public static T getOrThrow().... }

I think this will remove the need for a separate test suite just for this class. Until we know we'll need this for a wider set of use cases, I think we should just keep this as minimal as possible.

@singhpk234 @amogh-jahagirdar I looked at Vavr, but for such limited use I chose not to add it. I pared the class down; it now catches Throwable and rethrows using sneakyThrow, following the same pattern as Vavr.

geruh · 2025-10-09T21:32:28Z

+
+    // The exception should be thrown when locationProvider() is actually called
+    assertThatThrownBy(serializableTable::locationProvider).isSameAs(failure);
+


Should we add a test for round trip serialization to ensure the behavior is the same?

TestHelpers.KryoHelpers.roundTripSerialize(serializableTable)

amogh-jahagirdar

Thanks @przemekd , I agree with the general idea behind the fix, I just think we could have a simpler, and hidden Try construct to begin with.

amogh-jahagirdar · 2025-10-13T02:00:58Z

+ *
+ * @param <T> the type of the result
+ */
+public class Try<T> implements Serializable {


I feel like we can be even simpler to begin with here. I poked around locally, and I do agree we need some abstraction almost like a Try monad which encapsulates either a result of some kind or a failure. I originally was going to see if we could just use an abstraction over Tasks but ultimately, we need something to capture the state that it was a failure, not just handle the failure. We could use a separate AtomicReference and a boolean for indicating that a location provider could not be resolved, but that's pretty unclean.

That said, I don't think we need a full blown public class with all these APIs, at least as of now.

I think we could just have a package private class like the following with the main write API being of(SerializableSupplier<T> supplier) and the main read API being the getOrThrow.

class Try<T> implements Serializable { public static <T> of(SerializableSupplier<T> supplier) {//does what capture does} public static T getOrThrow().... }

I think this will remove the need for a separate test suite just for this class. Until we know we'll need this for a wider set of use cases, I think we should just keep this as minimal as possible.

amogh-jahagirdar · 2025-10-13T02:02:46Z

+  public static <T> Try<T> capture(ThrowingSupplier<T> supplier) {
+    try {
+      return success(supplier.get());
+    } catch (Exception e) {


I'd handle Throwable for this particular case

amogh-jahagirdar

Thank you @przemekd . I think it mostly looks great, just had a comment that I think we should separate out the kryo assertions into a separate test method.

amogh-jahagirdar · 2025-10-15T21:03:45Z

+    Table kryoDeserialized = KryoHelpers.roundTripSerialize(serializableTable);
+    assertThatThrownBy(kryoDeserialized::locationProvider)
+        .isInstanceOf(RuntimeException.class)
+        .hasMessage("location provider failure");


Could we have kryo test in a separate test? In case something breaks in the future it's easier to isolate if it's a kryo issue or not instead of having to poke into the tests.

Sure thing, it makes total sense 👍🏻

amogh-jahagirdar

Great thank you @przemekd , looks good from my side. I'll leave this up for a bit for @singhpk234 @geruh to take another look

geruh

LGTM! the Try class now effectively defers the location provider outcomes during serialization. Internal testing of this workflow has been successful!

amogh-jahagirdar · 2025-10-16T17:36:56Z

Thank you @przemekd and @geruh @singhpk234 for reviewing. I'll go ahead and merge. This is something that should be cherry-picked onto the 1.10.x branch

…ble apache#12564 (apache#14280) (cherry picked from commit e667f64)

…ble #12564 (#14280) (#14532) (cherry picked from commit e667f64) Co-authored-by: przemekd <1896041+przemekd@users.noreply.github.com>

…ble apache#12564 (apache#14280)

Fix locationProvider deferred exception handling

753585b

github-actions Bot added spark core labels Oct 8, 2025

Fix Checkstyle violations in TestTry.java

9a869c5

- Add missing .hasMessage() checks to assertThatThrownBy calls - Ensures compliance with AssertThatThrownByWithMessageCheck rule

przemekd mentioned this pull request Oct 8, 2025

Core: Fix locationProvider implementation for SerializableTable #12564

Closed

Fix remaining Checkstyle violation in TestTry.java

d409637

- Add .hasMessage() check to multi-line assertThatThrownBy call - Ensures full compliance with AssertThatThrownByWithMessageCheck rule

singhpk234 reviewed Oct 9, 2025

View reviewed changes

geruh reviewed Oct 9, 2025

View reviewed changes

amogh-jahagirdar reviewed Oct 13, 2025

View reviewed changes

singhpk234 added this to the Iceberg 1.10.1 milestone Oct 15, 2025

przemekd added 2 commits October 15, 2025 22:55

Refactor

53d2c0d

Formatting

8a525bb

amogh-jahagirdar reviewed Oct 15, 2025

View reviewed changes

Separate kryo and java serialization tests

5925487

amogh-jahagirdar changed the title ~~Core: Fix locationProvider implementation for SerializableTable #12564~~ Core: Allow unresolved location providers in SerializableTable until they are needed for writes #12564 Oct 15, 2025

amogh-jahagirdar changed the title ~~Core: Allow unresolved location providers in SerializableTable until they are needed for writes #12564~~ Core: Delay surfacing location provider errors in SerializableTable until writes are performed #12564 Oct 15, 2025

amogh-jahagirdar changed the title ~~Core: Delay surfacing location provider errors in SerializableTable until writes are performed #12564~~ Core: Defer surfacing location provider errors in SerializableTable until writes are performed #12564 Oct 15, 2025

amogh-jahagirdar changed the title ~~Core: Defer surfacing location provider errors in SerializableTable until writes are performed #12564~~ Core: Delay surfacing location provider resolution failures in SerializableTable until writes are performed #12564 Oct 15, 2025

amogh-jahagirdar approved these changes Oct 15, 2025

View reviewed changes

amogh-jahagirdar requested review from geruh and singhpk234 October 15, 2025 21:20

amogh-jahagirdar changed the title ~~Core: Delay surfacing location provider resolution failures in SerializableTable until writes are performed #12564~~ Core: Fix for respecting custom location providers #12564 Oct 15, 2025

amogh-jahagirdar changed the title ~~Core: Fix for respecting custom location providers #12564~~ Core: Fix for respecting custom location providers in SerializableTable #12564 Oct 15, 2025

Style

58da147

geruh approved these changes Oct 16, 2025

View reviewed changes

amogh-jahagirdar merged commit e667f64 into apache:main Oct 16, 2025
42 checks passed

huaxingao pushed a commit to huaxingao/iceberg that referenced this pull request Nov 7, 2025

Core: Fix for respecting custom location providers in SerializableTa…

a8f517f

…ble apache#12564 (apache#14280) (cherry picked from commit e667f64)

huaxingao added a commit that referenced this pull request Nov 7, 2025

Core: Fix for respecting custom location providers in SerializableTa…

1bd3ce8

…ble #12564 (#14280) (#14532) (cherry picked from commit e667f64) Co-authored-by: przemekd <1896041+przemekd@users.noreply.github.com>

thomaschow pushed a commit to thomaschow/iceberg that referenced this pull request Jan 19, 2026

Core: Fix for respecting custom location providers in SerializableTa…

28bacad

…ble apache#12564 (apache#14280)

talatuyarer pushed a commit to talatuyarer/iceberg that referenced this pull request Apr 1, 2026

Core: Fix for respecting custom location providers in SerializableTa…

a16f508

…ble apache#12564 (apache#14280)

ns-khsu mentioned this pull request May 19, 2026

[Bug] lazily create locationProvider in SerializableTable for iceberg tables StarRocks/starrocks#73471

Closed

dirtysalt mentioned this pull request May 19, 2026

[BugFix] Lazily initialize LocationProvider in SerializableTable to fix read failure on tables with custom LocationProvider StarRocks/starrocks#73482

Merged

23 tasks


		// The exception should be thrown when locationProvider() is actually called
		assertThatThrownBy(serializableTable::locationProvider).isSameAs(failure);

Uh oh!

Conversation

przemekd commented Oct 8, 2025

Uh oh!

singhpk234 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

przemekd Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geruh Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

przemekd Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

geruh left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Oct 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

singhpk234 left a comment •

edited

Loading

singhpk234 Oct 9, 2025 •

edited

Loading

amogh-jahagirdar Oct 13, 2025 •

edited

Loading

przemekd Oct 15, 2025 •

edited

Loading

amogh-jahagirdar Oct 13, 2025 •

edited

Loading