Spark 3.5: Don't change table distribution when only altering local order

[manuzhang]

As discussed with @aokolnychyi @szehon-ho and @RussellSpitzer in #10647 (comment)

[manuzhang]

@RussellSpitzer could you please take another look?

[manuzhang]

@RussellSpitzer @aokolnychyi @szehon-ho would you mind taking another look?

[szehon-ho]

Why not just return None when distributionSpec not set here? I thought that's more inline with the goal of the pr (to not override default unless explicitly set).

[szehon-ho]

I realize it would break a few tests and is a big behavior change. So @RussellSpitzer @aokolnychyi let me know if you disagree?

[manuzhang]

Which do you mean will break a few tests and is a big change?

[szehon-ho]

 val distributionMode = if (distributionSpec != null) {
      DistributionMode.HASH
    } else if (orderingSpec.UNORDERED != null || orderingSpec.LOCALLY != null) {
      DistributionMode.NONE
    } else {
      DistributionMode.RANGE
    }

makes more sense to me as:

    val distributionMode = if (distributionSpec != null) {
       Some(DistributionMode.HASH)
    } else {
      None
    }

[RussellSpitzer]

@szehon-ho I'm not sure I follow

[szehon-ho]

I thought the goal is to not set distributionMode , if its not in the ALTER statement. Hence why I thought the above code, which returns None in those cases unless its explicitly present in ALTER statement.

The current code works for the exact statement ALTER TABLE LOCALLY UNORDERED, but not for other ALTER's that dont have distributionMode?

[RussellSpitzer]

I believe this function is used by other pieces of code. Specifically the Create statements where RANGE is set if there is an ordering. Maybe it makes sense to separate it out into a Distribution and ordering for ALTER and one for CREATE?

[manuzhang]

@RussellSpitzer This method is for ALTER only.

iceberg/spark/v3.5/spark-extensions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4

Line 73 in c16cefa

| ALTER TABLE multipartIdentifier WRITE writeSpec #setWriteDistributionAndOrdering

@szehon-ho The result of this method can be one of HASH, NONE, RANGE or unchanged depending on the combination of distributionSpec and orderSpec. Thus, we cannot just return None (which mean unchanged) when distributionSpec is not set.

[manuzhang]

@RussellSpitzer Could you please take another look? It might be valuable to be included in 1.7 since it's a long-standing issue.

[RussellSpitzer]

Yes I think we are good here! Thanks @manuzhang for the patch and @szehon-ho for the review