[DISCUSS] Spark configuration

[Anton Okolnychyi <ao...@apple.com.INVALID>]

Hey devs,

I think we can improve the way we handle our Spark configuration right now. Specifically, I see a few issues.

Our SQL configs are scattered across a number of classes.

For example, we have some SQL configs in SparkUtil, IcebergSource, SparkWriteBuilder, Spark3Util. I think having a separate class for such constants would make sense, given that we already have SparkReadOptions and SparkWriteOptions.

We repeat the same code to parse the read/write options, session conf, table metadata in multiple places.

For example, we duplicate the code for getting the write file format, split related configs, whether to check nullability and order of incoming columns, etc. This happens partially because we have Spark 2 and Spark 3 integrations but also because we have multiple scans/writers in Spark 3. With upcoming support for merge-on-read, there will be even more classes where we would need to parse the same arguments.

The config resolution logic complicates classes where it is defined.

Our writer and scan builders are already complicated so removing the config resolution logic and dedicating a separate class for that seems like a promising idea.

We have inconsistent precedence order for Spark configs.

Historically, we interpreted whether a read/write option should take precedence over the corresponding session config inconsistently. At some point, we had a discussion [1] and reached consensus that the most common way to think about it is read option -> session conf -> table metadata. There are still places where the session config overrides options. I think we should finally fix that and be consistent even though it may be a behavior change.

I’ve put together a PR to cover these points and I’d appreciate your feedback. I mention this on the dev list because the last point is a behavior change and I want to make sure everyone is OK with that.

Anton

[1] - https://github.com/apache/iceberg/pull/2248#discussion_r580693954

Re: [DISCUSS] Spark configuration

[Anton Okolnychyi <ao...@apple.com.INVALID>]

Here is the PR:

https://github.com/apache/iceberg/pull/3132 <https://github.com/apache/iceberg/pull/3132>

- Anton

> On 16 Sep 2021, at 10:45, Anton Okolnychyi <ao...@apple.com.INVALID> wrote:
> 
> Hey devs,
> 
> I think we can improve the way we handle our Spark configuration right now. Specifically, I see a few issues.
> 
> Our SQL configs are scattered across a number of classes.
> 
> For example, we have some SQL configs in SparkUtil, IcebergSource, SparkWriteBuilder, Spark3Util. I think having a separate class for such constants would make sense, given that we already have SparkReadOptions and SparkWriteOptions.
> 
> We repeat the same code to parse the read/write options, session conf, table metadata in multiple places.
> 
> For example, we duplicate the code for getting the write file format, split related configs, whether to check nullability and order of incoming columns, etc. This happens partially because we have Spark 2 and Spark 3 integrations but also because we have multiple scans/writers in Spark 3. With upcoming support for merge-on-read, there will be even more classes where we would need to parse the same arguments.
> 
> The config resolution logic complicates classes where it is defined.
> 
> Our writer and scan builders are already complicated so removing the config resolution logic and dedicating a separate class for that seems like a promising idea.
> 
> We have inconsistent precedence order for Spark configs.
> 
> Historically, we interpreted whether a read/write option should take precedence over the corresponding session config inconsistently. At some point, we had a discussion [1] and reached consensus that the most common way to think about it is read option -> session conf -> table metadata. There are still places where the session config overrides options. I think we should finally fix that and be consistent even though it may be a behavior change.
> 
> I’ve put together a PR to cover these points and I’d appreciate your feedback. I mention this on the dev list because the last point is a behavior change and I want to make sure everyone is OK with that.
> 
> Anton
> 
> [1] - https://github.com/apache/iceberg/pull/2248#discussion_r580693954 <https://github.com/apache/iceberg/pull/2248#discussion_r580693954>