Returns isUnpartitioned=true for VoidTransform on all fields #3059

xinbinhuang · 2021-09-01T06:54:40Z

closes: #3014

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

rdblue · 2021-09-20T00:08:42Z

I agree with Russell. It would be great to add some tests to this. The overall change looks good.

xinbinhuang · 2021-09-27T06:15:25Z

@RussellSpitzer @rdblue Added the tests but not sure if I put them at the right place/file. PTAL :)

core/src/test/java/org/apache/iceberg/TestPartitionSpecInfo.java

RussellSpitzer

Looks good to me, I only had a minor question about whether we should test v1 tables who have had spec columns added and then removed but I think this isn't strictly necessary.

xinbinhuang · 2021-11-06T00:33:42Z

Looks good to me, I only had a minor question about whether we should test v1 tables who have had spec columns added and then removed but I think this isn't strictly necessary.

@RussellSpitzer Sorry for taking this long. I've added the test to check unpartitioned spec after removing all columns. PTAL.

xinbinhuang · 2021-11-09T07:56:36Z

@RussellSpitzer spark is failing but I don't think it's relevant to this PR. I'm going to rebase and force push

RussellSpitzer · 2021-11-09T16:05:57Z

@xinbinhuang Let's double check that test, I'm worried that there may be some part of the code that assumes "isUnpartitioned" means empty partition spec when we actually define it now as "empty or all void". Let's make sure the error isn't because of that

RussellSpitzer · 2021-11-11T18:17:14Z

Ok so the test failure is real and is caused by the check here

iceberg/spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java

Lines 543 to 544 in f5a7537

    
           if (spec.isUnpartitioned()) { 
        
             return new UnpartitionedDataWriter(writerFactory, fileFactory, io, spec, format, targetFileSize);

Which uses "isUnpartitioned" to check whether or not to use UnpartitionedDataWriter. UnpartitionedWriter passes through "null" as the value of the Spec which causes an NPE

https://github.com/apache/iceberg/blame/9f07b83725e05206219368c020c3c772771a63d0/core/src/main/java/org/apache/iceberg/DataFiles.java#L50

Where we attempt to copy through the values into the spec (which is all void transforms).

Before this patch V1 Tables with only void transforms would be considered "partitioned" by this code and deal with this correctly (but perhaps more confusingly.)

As far as I can tell we have a few ways to go forward here.

The Unpartitioned Writer can get an empty version of the PartitionSpec passed through as the value, basically a struct like where all the values are empty.
The if statement between Unpartitioned and Partitioned can check whether there are no fields in the spec rather than whether the spec is unpartitioned
We change the copy method to always return early if the spec is unpartitioned, rather than trying to reset partition values

I think I am leaning towards 1 or 3. I feel like we are dealing with an unpartitioned spec and because of V1 this means we have to be a bit careful about how we deal with that.

api/src/main/java/org/apache/iceberg/transforms/VoidTransform.java

xinbinhuang

@RussellSpitzer @rdblue I did it slightly different from what Russell suggested, as I think this's slightly cleaner and easier to reason about. WDYT?

xinbinhuang · 2021-11-13T18:12:58Z

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

+      if (isPartitioned) {
+        this.partitionData = copyPartitionData(spec, newPartition, partitionData);
+      }


Only copy if the spec is partitionable.

I would rather we fix this in one of the other ways we discussed. This ends up feeling a little strange to me since you can call "withPartition" and the arg is ignored. I don't like when functions can take arguments but then not doing anything with them.

@aokolnychyi Do you mind taking a look at this change?

I have a chat with @RussellSpitzer offline, and would love to have your thoughts on this.

The current implementation

works similar to how we do check for other methods inside the builder

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

Lines 238 to 245 in 73ee251

public Builder withPartitionPath(String newPartitionPath) {

Preconditions.checkArgument(isPartitioned || newPartitionPath.isEmpty(),

"Cannot add partition data for an unpartitioned table");

if (!newPartitionPath.isEmpty()) {

this.partitionData = fillFromPath(spec, newPartitionPath, partitionData);

}

return this;

}

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

Lines 259 to 266 in 73ee251

public Builder withSplitOffsets(List<Long> offsets) {

if (offsets != null) {

this.splitOffsets = copyList(offsets);

} else {

this.splitOffsets = null;

}

return this;

}

Compatible with builder's whole building logics

builder assign partitionData to be null if the spec is not partitioned:

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

Line 141 in 73ee251

this.partitionData = isPartitioned ? newPartitionData(spec) : null;

And it do another check and pass in null to the final GenericDataFile:

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

Line 294 in 73ee251

specId, filePath, format, isPartitioned ? partitionData.copy() : null,

Alternatively, we are thinking:

GenericRecord emptyRecord = GenericRecord.create(spec.schema()); delegate = writerFactory.newDataWriter(outputFile, spec, emptyRecord);

at around here:

iceberg/spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java

Line 594 in 1e5abce

delegate = writerFactory.newDataWriter(outputFile, spec, null);

xinbinhuang · 2021-11-13T18:13:21Z

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

      this.spec = spec;
      this.specId = spec.specId();
-      this.isPartitioned = spec.fields().size() > 0;
+      this.isPartitioned = !spec.isUnpartitioned();


More inclusive check for isPartitioned to includes V1 table

xinbinhuang · 2021-11-13T18:14:37Z

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

  }

  static PartitionData copyPartitionData(PartitionSpec spec, StructLike partitionData, PartitionData reuse) {
+    Preconditions.checkArgument(!spec.isUnpartitioned(), "Can't copy partition data to a unpartitioned table");


The underlying method explicitly check for isUnpartitioned() in case this method is called outside the builder. (FileMetadata.java)

xinbinhuang · 2021-11-13T18:14:56Z

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

      this.spec = spec;
      this.specId = spec.specId();
-      this.isPartitioned = spec.fields().size() > 0;
+      this.isPartitioned = !spec.isUnpartitioned();


Do similar thing to FileMetadata.java

xinbinhuang · 2021-11-13T18:15:00Z

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

+      if (isPartitioned) {
+        this.partitionData = DataFiles.copyPartitionData(spec, newPartition, partitionData);
+      }


Do similar thing to FileMetadata.java

xinbinhuang · 2021-11-30T00:57:11Z

@rdblue @aokolnychyi @kbendick would you help take a look when you have time? Would love to get this merged before it goes into stale. thank you :)

RussellSpitzer · 2022-02-22T16:27:12Z

Sorry it took me so long to get to this, I think this is good to go once the PR has been rebased

xinbinhuang · 2022-12-09T20:25:58Z

(@RussellSpitzer sorry didn't see the last message from you)
@RussellSpitzer @rdblue Just rebased. PTAL

RussellSpitzer · 2022-12-09T20:32:23Z

@xinbinhuang Looks good to me, once tests pass I think we are good to go.

RussellSpitzer · 2022-12-09T22:53:06Z

My commit title was inverted, mia culpa. For anyone looking this up in the future I meant that "all void transforms should be false"

…rms (apache#3059)

github-actions bot added the API label Sep 1, 2021

RussellSpitzer reviewed Sep 1, 2021

View reviewed changes

api/src/main/java/org/apache/iceberg/PartitionSpec.java Outdated Show resolved Hide resolved

github-actions bot added the core label Sep 26, 2021

xinbinhuang changed the title ~~Returns UnpartitionedSpec for VoidTransform on all fields~~ Returns isUnpartitioned=true for VoidTransform on all fields Sep 27, 2021

xinbinhuang requested review from RussellSpitzer and rdblue September 27, 2021 06:14

RussellSpitzer reviewed Sep 27, 2021

View reviewed changes

core/src/test/java/org/apache/iceberg/TestPartitionSpecInfo.java Show resolved Hide resolved

RussellSpitzer approved these changes Sep 27, 2021

View reviewed changes

xinbinhuang force-pushed the patch-1 branch from 6f8847b to 5750346 Compare November 6, 2021 00:30

xinbinhuang force-pushed the patch-1 branch from cffe21a to 79c1a35 Compare November 9, 2021 08:04

xinbinhuang force-pushed the patch-1 branch from 79c1a35 to 4bfa662 Compare November 11, 2021 06:10

aokolnychyi reviewed Nov 11, 2021

View reviewed changes

api/src/main/java/org/apache/iceberg/transforms/VoidTransform.java Outdated Show resolved Hide resolved

xinbinhuang commented Nov 13, 2021

View reviewed changes

xinbinhuang requested a review from RussellSpitzer November 13, 2021 22:04

xinbinhuang requested a review from aokolnychyi November 21, 2021 08:09

flyrain mentioned this pull request Apr 14, 2022

Hive: Expose default partition spec and sort order in HMS. #4546

Merged

xinbinhuang force-pushed the patch-1 branch 2 times, most recently from 3a5de40 to a415f3a Compare December 9, 2022 18:29

Returns true for VoidTransform on all fields

bd88bd9

xinbinhuang force-pushed the patch-1 branch from cf12a35 to bd88bd9 Compare December 9, 2022 19:34

RussellSpitzer merged commit d350c9b into apache:master Dec 9, 2022

nastra mentioned this pull request Dec 13, 2022

Core: Don't produce partition summaries on unpartitioned table #6411

Merged

nastra mentioned this pull request Jan 16, 2023

Update Iceberg to 1.2.0 RC1 trinodb/trino#15726

Closed

electrum mentioned this pull request Mar 15, 2023

Update to Iceberg 1.2.1 trinodb/trino#16557

Merged

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

API: IsPartitioned to return true for V1 Tables with all void transfo…

4b49e14

…rms (apache#3059)

	public Builder withPartitionPath(String newPartitionPath) {
	Preconditions.checkArgument(isPartitioned \|\| newPartitionPath.isEmpty(),
	"Cannot add partition data for an unpartitioned table");
	if (!newPartitionPath.isEmpty()) {
	this.partitionData = fillFromPath(spec, newPartitionPath, partitionData);
	}
	return this;
	}

	public Builder withSplitOffsets(List<Long> offsets) {
	if (offsets != null) {
	this.splitOffsets = copyList(offsets);
	} else {
	this.splitOffsets = null;
	}
	return this;
	}

Returns isUnpartitioned=true for VoidTransform on all fields #3059

Returns isUnpartitioned=true for VoidTransform on all fields #3059

Uh oh!

Conversation

xinbinhuang commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rdblue commented Sep 20, 2021

Uh oh!

xinbinhuang commented Sep 27, 2021

Uh oh!

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

xinbinhuang commented Nov 6, 2021

Uh oh!

xinbinhuang commented Nov 9, 2021

Uh oh!

RussellSpitzer commented Nov 9, 2021

Uh oh!

RussellSpitzer commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

xinbinhuang 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!

xinbinhuang Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Nov 17, 2021 • 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!

xinbinhuang Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

xinbinhuang Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

xinbinhuang Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

xinbinhuang Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

xinbinhuang Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

xinbinhuang commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussellSpitzer commented Feb 22, 2022

Uh oh!

xinbinhuang commented Dec 9, 2022

Uh oh!

RussellSpitzer commented Dec 9, 2022

Uh oh!

RussellSpitzer commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xinbinhuang commented Sep 1, 2021 •

edited

Loading

RussellSpitzer commented Nov 11, 2021 •

edited

Loading

xinbinhuang left a comment •

edited

Loading

RussellSpitzer Nov 17, 2021 •

edited

Loading

xinbinhuang commented Nov 30, 2021 •

edited

Loading