apache / iceberg

Apache Iceberg
https://iceberg.apache.org/
Apache License 2.0
6.22k stars 2.17k forks source link

A move after a rename fails #10830

Open Fokko opened 1 month ago

Fokko commented 1 month ago

Apache Iceberg version

1.6.0 (latest release)

Query engine

Other

Please describe the bug 🐞

When you try to move a renamed column, this fails:

  @Test
  public void testMoveAfterRename() {
    Schema schema =
      new Schema(
              required(1, "b", Types.IntegerType.get()),
              required(2, "c", Types.IntegerType.get()));

    Schema actual = new SchemaUpdate(schema, 4)
            .renameColumn("c", "a")
            .moveBefore("a", "b")
            .apply();

    Schema expected =
            new Schema(
                    required(2, "a", Types.IntegerType.get()),
                    required(1, "b", Types.IntegerType.get()));
    assertThat(actual.asStruct()).isEqualTo(expected.asStruct());
  }

It cannot find the column:

Cannot move missing column: a
java.lang.IllegalArgumentException: Cannot move missing column: a
    at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:220)

Should we allow this?

Willingness to contribute

RussellSpitzer commented 1 month ago

Can you move then rename? Just wondering how this is actually set up.

Fokko commented 1 month ago

Yes, this works:

  @Test
  public void testMoveAfterRename() {
    Schema schema =
      new Schema(
              required(1, "b", Types.IntegerType.get()),
              required(2, "c", Types.IntegerType.get()));

    Schema actual = new SchemaUpdate(schema, 4)
            .moveBefore("c", "b")
            .renameColumn("c", "a")
            .apply();

    Schema expected =
            new Schema(
                    required(2, "a", Types.IntegerType.get()),
                    required(1, "b", Types.IntegerType.get()));
    assertThat(actual.asStruct()).isEqualTo(expected.asStruct());
  }

To make the other way around work, we have to track the in-flight renames, so it is essentially more bookkeeping. Just wanted to check if this is intentional.

RussellSpitzer commented 1 month ago

I doubt we intend it to work that way, but I'm not sure it's worth fixing?

Fokko commented 1 month ago

I ran into it when you evolve an existing schema, the move operation is different when you add new fields or evolve existing fields (you can move newly added fields). But you're right, it's probably something nice to have.

RussellSpitzer commented 1 month ago

This may be a good first issue for someone @Fokko In case you want to leave it for someone else to tinker with :)

Fokko commented 1 month ago

@RussellSpitzer For sure!

FANNG1 commented 1 month ago

@Fokko , I'd like to take this issue, could you assign it to me?