ORC: Add configurable write properties

[hililiwei]

closes #3272

To be consistent with Parquet, it would be good to introduce write table properties for ORC too.
Also make sure that we have a configuration available for ORC separately.
Discussed here: #2935 (comment)

[hililiwei]

reopen rerun CI

[rdblue]

@omalley or @shardulm94, is this a config that we should expose as an Iceberg table setting? Is it still needed for object stores?

[pvary]

I think if we decide that we need the default configuration values set for Iceberg reads, then we should do this the same way as it has been done with the other file formats:

• Parquet:
  

  iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

  Lines 304 to 320 in 129cdfa

  	static Context dataContext(Map<String, String> config) {
  	int rowGroupSize = Integer.parseInt(config.getOrDefault(
  	PARQUET_ROW_GROUP_SIZE_BYTES, PARQUET_ROW_GROUP_SIZE_BYTES_DEFAULT));
  	int pageSize = Integer.parseInt(config.getOrDefault(
  	PARQUET_PAGE_SIZE_BYTES, PARQUET_PAGE_SIZE_BYTES_DEFAULT));
  	int dictionaryPageSize = Integer.parseInt(config.getOrDefault(
  	PARQUET_DICT_SIZE_BYTES, PARQUET_DICT_SIZE_BYTES_DEFAULT));
  	String codecAsString = config.getOrDefault(PARQUET_COMPRESSION, PARQUET_COMPRESSION_DEFAULT);
  	CompressionCodecName codec = toCodec(codecAsString);
  	String compressionLevel = config.getOrDefault(PARQUET_COMPRESSION_LEVEL, PARQUET_COMPRESSION_LEVEL_DEFAULT);
  	return new Context(rowGroupSize, pageSize, dictionaryPageSize, codec, compressionLevel);
  	}

• Avro:
  

  iceberg/core/src/main/java/org/apache/iceberg/avro/Avro.java

  Lines 208 to 213 in 303f925

  	static Context dataContext(Map<String, String> config) {
  	String codecAsString = config.getOrDefault(AVRO_COMPRESSION, AVRO_COMPRESSION_DEFAULT);
  	CodecFactory codec = toCodec(codecAsString);
  	return new Context(codec);
  	}

Basically we should create a Context object for the ORC as well.

[hililiwei]

I think if we decide that we need the default configuration values set for Iceberg reads, then we should do this the same way as it has been done with the other file formats:
…………
Basically we should create a Context object for the ORC as well.

added to the latest commiter. PTAL, thx.

[pvary]

@hililiwei: Do we have any tests for this in Parquet/Avro?
I think it would b good to add some tests, so we can be sure that these configurations are working as expected.

[hililiwei]

@hililiwei: Do we have any tests for this in Parquet/Avro? I think it would b good to add some tests, so we can be sure that these configurations are working as expected.

@pvary Checked, and seemed no relevant tests. I added one unit test for ORC this time. How about let me open another PR for Parquet\AVRO?

[pvary]

@szlta: Would you mind taking a look at the tests?

Thanks,
Peter

[openinx]

Almost looks good to me overall, just left several comments.

[openinx]

Looks good to me now, thanks @hililiwei for the contribution, and thanks @rdblue and @jackye1995 for the reviewing !

[hililiwei]

Thank you all for reviewing.

[rdblue]

@openinx, @hililiwei, I think we need to revert this. It looks like the dataContext and deleteContext methods accept Hadoop Configuration objects rather than property maps.

Iceberg doesn't use Hadoop Configuration. We can either directly fix this to work like the Parquet contexts with another PR, or I think we should revert and open a replacement PR. What do you think is the right way forward?

[hililiwei]

@openinx, @hililiwei, I think we need to revert this. It looks like the dataContext and deleteContext methods accept Hadoop Configuration objects rather than property maps.

Iceberg doesn't use Hadoop Configuration. We can either directly fix this to work like the Parquet contexts with another PR, or I think we should revert and open a replacement PR. What do you think is the right way forward?

I understand what you mean roughly. Let me raise a separate PR, it may goes beyond the this PR content.

[openinx]

@rdblue The ORC#WriterBuilder will copy all the key values from property map to the Hadoop configuration, please see [1] and [2]. So in my thought, we can just use the unified hadoop configuration to get all of the configure keys. Will this answer your concern ?

[1]. https://github.com/apache/iceberg/blob/master/orc/src/main/java/org/apache/iceberg/orc/ORC.java#L122
[2]. https://github.com/apache/iceberg/blob/master/orc/src/main/java/org/apache/iceberg/orc/ORC.java#L132

[hililiwei]

iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

Lines 153 to 156 in c6710cd

	public WriteBuilder set(String property, String value) {
	config.put(property, value);
	return this;
	}

iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

Lines 158 to 161 in c6710cd

	public WriteBuilder setAll(Map<String, String> properties) {
	config.putAll(properties);
	return this;
	}

iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

Lines 261 to 270 in c6710cd

	Configuration conf;
	if (file instanceof HadoopOutputFile) {
	conf = ((HadoopOutputFile) file).getConf();
	} else {
	conf = new Configuration();
	}
	for (Map.Entry<String, String> entry : config.entrySet()) {
	conf.set(entry.getKey(), entry.getValue());
	}

I mainly refer to Parquet's code above. Do you mean to use this way as well? @openinx

[openinx]

Let's make this more clear:

In the ORC class, we just copied all key values from property maps to hadoop configuration. And all the following configure keys will be parsed from hadoop configuration instance.

In the current Parquet class, we will just keep all the property maps into in-memory hash map, and then the dataContext & deleteContext will parse the hash map directly. Finally, we will copy the key values from HashMap to hadoop configuration.

In my mind, I just don't see the difference between the two approach. Both look good to me.

[zhongyujiang]

@openinx, I think what @rdblue means is that in current logic Iceberg configuration might be overridden by Hadoop configuration , for example:
when Hadoop configuraion is configed a property
"orc.compress" = "snappy"
and no corresponding property has been configed in Iceberg table props, In the present context ORC will use snappy for compress as configed in Hadoop. But the correct procedure is use default codec configed in TableProperties.

So we need a props map to receive table props passed in, then set corresponding ORC props in Hadoop configuration after parsing, just like Parquet.

[rdblue]

For Parquet the Hadoop configuration is used to pass options into the data file. It is not used as the source of table configuration. Table configuration properties should never come from the Hadoop Configuration.

The steps should be:

1. Get the Hadoop configuration, if it is present in the InputFile or OutputFile. If not, default it
2. Get config from the builder and the table properties
3. Set configuration from step 2 on the Hadoop config
4. Create the reader or writer with the Hadoop config

The only configuration coming from the Hadoop Configuration itself is whatever was in the environment.

One possibly confusing thing is that we also set config values directly in the Hadoop configuration. That handles cases where the user wants to pass config properties that are not standardized in Iceberg. So you could use set("parquet.bloom.filter.enabled#id", "true") for example. Standardized table settings should override these.

[openinx]

Okay, I think I get the drawback about the current ORC builder ( which copied the table properties map into hadoop conf and just use the hadoop conf as the source of truth):

• As @zhongyujiang said, the default value of iceberg standardized settings will be replaced by the hadoop config values.
• Non-standardized iceberg settings which are added in the iceberg table properties will be affected to the underlying ORC readers & writers.
• The standardized iceberg settings will be added to hadoop configuration, which may pollute the hadoop configuration.

I agree to make another PR to change the ORC configs parser as the @rdblue suggested.

Thanks.