Core, API: Add addNonDefaultSpec to UpdatePartitionSpec to not set the new partition spec as default #10736

shanielh · 2024-07-21T10:00:23Z

This may be used in cases where we want to add a partition spec but not set the new partition spec as default.

For example, if I want to make sure that there's an unpartitioned spec since I might want to commit specific data to it in some cases, I would first validate that there's an unpartition spec in the table, and if there isn't, I would try to add the unpartitioned spec but not set it as the new default partition spec

RussellSpitzer · 2024-07-22T16:38:12Z

My big question here is what is the value of adding the Spec before you are ready to write data?

shanielh · 2024-07-22T17:18:51Z

My big question here is what is the value of adding the Spec before you are ready to write data?

I don't think it matters whether I add the spec just before writing to it or prior. But I wouldn't like to set it as the new default.

RussellSpitzer · 2024-07-22T21:25:37Z

My big question here is what is the value of adding the Spec before you are ready to write data?

I don't think it matters whether I add the spec just before writing to it or prior. But I wouldn't like to set it as the new default.

Wouldn't that mean we would want to modify the write APIs? Isn't that essentially just an engine decision?

shanielh · 2024-07-23T04:32:09Z

My big question here is what is the value of adding the Spec before you are ready to write data?

I don't think it matters whether I add the spec just before writing to it or prior. But I wouldn't like to set it as the new default.

Wouldn't that mean we would want to modify the write APIs? Isn't that essentially just an engine decision?

The write APIs already support that.

RussellSpitzer · 2024-07-23T17:55:45Z

Got it, Ok let me take a closer look at this

RussellSpitzer

This seems reasonable but we do need to add some tests to TestUpdatePartitionSpec to validate the behavior

shanielh · 2024-07-24T06:17:15Z

This seems reasonable but we do need to add some tests to TestUpdatePartitionSpec to validate the behavior

I had to add the spec to TestTableUpdatePartitionSpec since that's the only one that actually commit the spec to the table, PLMK if there's anything else I need to do for this to get merged.

amogh-jahagirdar · 2024-07-24T21:15:13Z

api/src/main/java/org/apache/iceberg/UpdatePartitionSpec.java

+   * @param value whether to set the new partition spec as default
+   * @return this for method chaining
+   */
+  UpdatePartitionSpec setAsDefault(Boolean value);


I understand the intent and it's reasonable imo (don't think there's any other API where adding this would make sense). Typically we try avoid adding boolean APIs but to facilitate this case, it may be unavoidable. I'd like to think through more alternatives though.

On another note, at minimum I think this should be made a primitive boolean named setAsDefaultSpec (same with the API name imo)?

we could do a no arg method instead something like

notDefault()

Yeah couldn’t think of a good name for a no arg method so I discarded it initially but another one came to mind: addWithoutSettingCurrent (maybe a bit verbose)

amogh-jahagirdar · 2024-07-24T21:17:51Z

core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java

@@ -327,7 +336,12 @@ public PartitionSpec apply() {

  @Override
  public void commit() {
-    TableMetadata update = base.updatePartitionSpec(apply());
+    TableMetadata update;
+    if (this.setAsDefault) {


typically in the project, this is only used when writing to the field. Here we could just reference setAsDefault

shanielh · 2024-07-25T10:07:19Z

Hi (@amogh-jahagirdar, @RussellSpitzer), I've applied the changes you suggested within a new fixup commit, please review and let me know when it's good to squash and rebase.

RussellSpitzer

This looks good to me now, we can merge if @amogh-jahagirdar agrees

amogh-jahagirdar

I'm not very against this idea, but after some more thinking, I think I'm struggling to understand the use case and can be convinced otherwise. I thought I originally understood the intent, but now it's a bit confusing to me:

After the spec is added without making it the current, active table spec, how will it later be set as the current? With the current APIs a user has to again call updateSpec(), reconstruct the desired spec and then commit, and rely on the behavior of the implementation to essentially re-use the partition spec that was previously added.

At minimum though would advocate for a more clear API name which indicates what "nonDefault" is referring to; I'm proposing addNonDefaultSpec or addInactiveSpec

amogh-jahagirdar · 2024-07-27T01:47:01Z

api/src/main/java/org/apache/iceberg/UpdatePartitionSpec.java

+   *
+   * @return this for method chaining
+   */
+  UpdatePartitionSpec notDefault();


Could we call this addNonDefaultSpec or addInactiveSpec cc @RussellSpitzer @shanielh?

When I was going through the test code the API name felt a bit odd (the heuristic I try and aim for is does it sound right when speaking as a sentence).

table.updateSpec().addField("id").addNonDefaultSpec().commit();

reads more clear to me since it's more clear what the action is ("adding"), and what non-default is referring to ("spec")

inactive maybe a bit more extreme but technically it is not the active table spec if it's added, since every write to the table should just ignore that spec until it is made active.

amogh-jahagirdar

Ah just saw/remembered #10736 (comment), the write APIs Iceberg exposes allow passing in a given spec ID. So you're not really looking to set the spec as current but rather just leverage the existence of the spec in the write APIs. That's reasonable.

Cool, I think I'd still advocate for a different method name. #10736 (comment)

After that, I think the PR looks good to me.

shanielh · 2024-07-27T04:03:21Z

Ah just saw/remembered #10736 (comment), the write APIs Iceberg exposes allow passing in a given spec ID. So you're not really looking to set the spec as current but rather just leverage the existence of the spec in the write APIs. That's reasonable.

Cool, I think I'd still advocate for a different method name. #10736 (comment)

After that, I think the PR looks good to me.

Ok, I've modified the method name and squashed all commits into one.

Thanks!

RussellSpitzer · 2024-07-29T17:36:32Z

Revapi broken (as expected) please fix

amogh-jahagirdar

Overall LGTM, as @RussellSpitzer mentioned you'll need to fix RevAPI. Also @RussellSpitzer not sure if you saw the naming change to make the API called addNonDefaultSpec , just want to make sure you're also good with that!

RussellSpitzer · 2024-07-29T19:56:55Z

Overall LGTM, as @RussellSpitzer mentioned you'll need to fix RevAPI. Also @RussellSpitzer not sure if you saw the naming change to make the API called addNonDefaultSpec , just want to make sure you're also good with that!

A rose by any other name :) still +1

…as default This may be used in cases where we want to add a partition spec but not set the new partition spec as default. For example, if I want to make sure that there's an unpartitioned spec since I might want to commit specific data to it in some cases, I would first validate that there's an unpartition spec in the table, and if there isn't, I would try to add the unpartitioned spec but not set it as the new default partition spec

shanielh · 2024-07-31T13:34:57Z

Overall LGTM, as @RussellSpitzer mentioned you'll need to fix RevAPI. Also @RussellSpitzer not sure if you saw the naming change to make the API called addNonDefaultSpec , just want to make sure you're also good with that!

Fixed, thanks.

shanielh · 2024-08-04T05:53:26Z

@RussellSpitzer, @amogh-jahagirdar Any chance to merge this? All checks have passed and two approvals 😄

RussellSpitzer · 2024-08-05T02:44:45Z

@shanielh In general we want to wait extra time on API module changes since we are committing the project to support this new API for ever. That said I think we've had this open long enough so I'll merge today.

RussellSpitzer · 2024-08-05T02:46:19Z

Thanks @shanielh for the PR, and @amogh-jahagirdar for Reviewing

github-actions bot added API core labels Jul 21, 2024

shanielh force-pushed the feature/update-partition-spec-set-as-default branch from 2c2a54b to a2e1cd7 Compare July 21, 2024 10:05

RussellSpitzer requested changes Jul 23, 2024

View reviewed changes

amogh-jahagirdar requested changes Jul 24, 2024

View reviewed changes

amogh-jahagirdar reviewed Jul 24, 2024

View reviewed changes

RussellSpitzer approved these changes Jul 26, 2024

View reviewed changes

amogh-jahagirdar reviewed Jul 27, 2024

View reviewed changes

amogh-jahagirdar requested changes Jul 27, 2024

View reviewed changes

shanielh force-pushed the feature/update-partition-spec-set-as-default branch from 503f8fb to 175a391 Compare July 27, 2024 04:02

shanielh force-pushed the feature/update-partition-spec-set-as-default branch from 175a391 to 5d69bc4 Compare July 28, 2024 06:22

amogh-jahagirdar approved these changes Jul 29, 2024

View reviewed changes

amogh-jahagirdar changed the title ~~UpdatePartitionSpec: Added ability to not set the new partition spec as default~~ Core, API: Add addNonDefaultSpec to UpdatePartitionSpec to not set the new partition spec as default Jul 29, 2024

shanielh force-pushed the feature/update-partition-spec-set-as-default branch from 5d69bc4 to ac1a9df Compare July 31, 2024 13:34

RussellSpitzer merged commit d9aacd2 into apache:main Aug 5, 2024
61 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core, API: Add addNonDefaultSpec to UpdatePartitionSpec to not set the new partition spec as default #10736

Core, API: Add addNonDefaultSpec to UpdatePartitionSpec to not set the new partition spec as default #10736

shanielh commented Jul 21, 2024

RussellSpitzer commented Jul 22, 2024

shanielh commented Jul 22, 2024

RussellSpitzer commented Jul 22, 2024

shanielh commented Jul 23, 2024

RussellSpitzer commented Jul 23, 2024

RussellSpitzer left a comment

shanielh commented Jul 24, 2024

amogh-jahagirdar Jul 24, 2024

RussellSpitzer Jul 24, 2024

amogh-jahagirdar Jul 24, 2024

amogh-jahagirdar Jul 24, 2024

shanielh commented Jul 25, 2024 •

edited

Loading

RussellSpitzer left a comment

amogh-jahagirdar left a comment •

edited

Loading

amogh-jahagirdar Jul 27, 2024

amogh-jahagirdar left a comment •

edited

Loading

shanielh commented Jul 27, 2024

RussellSpitzer commented Jul 29, 2024

amogh-jahagirdar left a comment

RussellSpitzer commented Jul 29, 2024

shanielh commented Jul 31, 2024

shanielh commented Aug 4, 2024

RussellSpitzer commented Aug 5, 2024

RussellSpitzer commented Aug 5, 2024

Core, API: Add addNonDefaultSpec to UpdatePartitionSpec to not set the new partition spec as default #10736

Core, API: Add addNonDefaultSpec to UpdatePartitionSpec to not set the new partition spec as default #10736

Conversation

shanielh commented Jul 21, 2024

RussellSpitzer commented Jul 22, 2024

shanielh commented Jul 22, 2024

RussellSpitzer commented Jul 22, 2024

shanielh commented Jul 23, 2024

RussellSpitzer commented Jul 23, 2024

RussellSpitzer left a comment

Choose a reason for hiding this comment

shanielh commented Jul 24, 2024

amogh-jahagirdar Jul 24, 2024

Choose a reason for hiding this comment

RussellSpitzer Jul 24, 2024

Choose a reason for hiding this comment

amogh-jahagirdar Jul 24, 2024

Choose a reason for hiding this comment

amogh-jahagirdar Jul 24, 2024

Choose a reason for hiding this comment

shanielh commented Jul 25, 2024 • edited Loading

RussellSpitzer left a comment

Choose a reason for hiding this comment

amogh-jahagirdar left a comment • edited Loading

Choose a reason for hiding this comment

amogh-jahagirdar Jul 27, 2024

Choose a reason for hiding this comment

amogh-jahagirdar left a comment • edited Loading

Choose a reason for hiding this comment

shanielh commented Jul 27, 2024

RussellSpitzer commented Jul 29, 2024

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

RussellSpitzer commented Jul 29, 2024

shanielh commented Jul 31, 2024

shanielh commented Aug 4, 2024

RussellSpitzer commented Aug 5, 2024

RussellSpitzer commented Aug 5, 2024

shanielh commented Jul 25, 2024 •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading