You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/03/08 16:22:07 UTC
[GitHub] [iceberg] hililiwei opened a new pull request #4291: ORC:Optimize the use of table properties for ORC
hililiwei opened a new pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291
Based on https://github.com/apache/iceberg/pull/3810 comments.
Refer:https://github.com/apache/iceberg/pull/3810#issuecomment-1061197742
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823508563
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
It's true that the original name `iceberg.orc.vectorbatch.size` is not an iceberg name style. This config key was introduced in this PR: https://github.com/apache/iceberg/pull/139/files. In theory, people could add this config key-value in their enviroment hadoop configuration file. But after we merged this PR, the key won't take any effect even if we use the old key in hadoop environment.
I will suggest to make it deprecated and then introduce the correct table property. But I think we still need to be able to read the value from the old key.
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
It's true that the original name `iceberg.orc.vectorbatch.size` is not an iceberg name style. This config key was introduced in this PR: https://github.com/apache/iceberg/pull/139/files. In theory, people could add this config key-value in their enviroment hadoop configuration file. But after we merged this PR, the key won't take any effect even if we use the old key in hadoop environment.
I will suggest to make it deprecated and then introduce the correct table property. But I think we still need to be able to read the value from the old key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823764110
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
OK, I added the standardized table configuration and tried to get values from the old key for compatibility, please take a look, thx.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx merged pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx merged pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823762252
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -186,35 +198,47 @@ public long blockSize() {
return blockSize;
}
- private Context(long stripeSize, long blockSize) {
+ public int vectorizedRowBatchSize() {
+ return vectorizedRowBatchSize;
+ }
+
+ private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
+ this.vectorizedRowBatchSize = vectorizedRowBatchSize;
}
- static Context dataContext(Configuration conf) {
- long stripeSize = TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT;
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getAttribute(), stripeSize);
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getHiveConfName(), stripeSize);
- stripeSize = conf.getLong(TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ static Context dataContext(Map<String, String> config) {
+ long stripeSize = PropertyUtil.propertyAsLong(config,
+ TableProperties.ORC_STRIPE_SIZE_BYTES, TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT);
+ Preconditions.checkArgument(stripeSize > 0, "Stripe size must be > 0");
- long blockSize = TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT;
- blockSize = conf.getLong(OrcConf.BLOCK_SIZE.getAttribute(), blockSize);
- blockSize = conf.getLong(OrcConf.BLOCK_SIZE.getHiveConfName(), blockSize);
- blockSize = conf.getLong(TableProperties.ORC_BLOCK_SIZE_BYTES, blockSize);
+ long blockSize = PropertyUtil.propertyAsLong(config,
+ TableProperties.ORC_BLOCK_SIZE_BYTES, TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT);
+ Preconditions.checkArgument(blockSize > 0, "Block size must be > 0");
- return new Context(stripeSize, blockSize);
+ int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
+ TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, TableProperties.ORC_VECTOR_ROW_BATCH_SIZE_DEFAULT);
+ vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
+ VECTOR_ROW_BATCH_SIZE, vectorizedRowBatchSize);
Review comment:
For compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1066270725
@openinx One thing to note. Although the k-v of config is copied to hadoop conf, if a standardized iceberg key, such as "orc.stripe.size", is set in conf using only the origin key, instead of the standardized key "write.orc.stripe-size-bytes", it will eventually be overwritten by iceberg's default value.
@zhongyujiang seems to be talking about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823314117
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java
##########
@@ -123,7 +124,7 @@ public void writeFile() throws IOException {
// write in such a way that the file contains 10 stripes each with 100 rows
.set("iceberg.orc.vectorbatch.size", "100")
.set(OrcConf.ROWS_BETWEEN_CHECKS.getAttribute(), "100")
- .set(OrcConf.STRIPE_SIZE.getAttribute(), "1")
Review comment:
If the standard iceberg table properties is not found, do we feedback to try the origin ORC key?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825480666
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
I agree that we should deprecate and keep supporting `iceberg.orc.vectorbatch.size` from the Hadoop conf since that slipped through review. But we can still clean this up with a reasonable table property.
The read property is `read.orc.vectorization.batch-size` so I think this should match: `write.orc.vectorization.batch-size`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823521527
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
Okay, I'll restore it, and let's do it separately later if we think it's necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r824412105
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -186,35 +198,47 @@ public long blockSize() {
return blockSize;
}
- private Context(long stripeSize, long blockSize) {
+ public int vectorizedRowBatchSize() {
+ return vectorizedRowBatchSize;
+ }
+
+ private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
+ this.vectorizedRowBatchSize = vectorizedRowBatchSize;
}
- static Context dataContext(Configuration conf) {
- long stripeSize = TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT;
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getAttribute(), stripeSize);
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getHiveConfName(), stripeSize);
- stripeSize = conf.getLong(TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ static Context dataContext(Map<String, String> config) {
+ long stripeSize = PropertyUtil.propertyAsLong(config,
+ TableProperties.ORC_STRIPE_SIZE_BYTES, TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT);
+ Preconditions.checkArgument(stripeSize > 0, "Stripe size must be > 0");
- long blockSize = TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT;
- blockSize = conf.getLong(OrcConf.BLOCK_SIZE.getAttribute(), blockSize);
- blockSize = conf.getLong(OrcConf.BLOCK_SIZE.getHiveConfName(), blockSize);
- blockSize = conf.getLong(TableProperties.ORC_BLOCK_SIZE_BYTES, blockSize);
+ long blockSize = PropertyUtil.propertyAsLong(config,
+ TableProperties.ORC_BLOCK_SIZE_BYTES, TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT);
+ Preconditions.checkArgument(blockSize > 0, "Block size must be > 0");
- return new Context(stripeSize, blockSize);
+ int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
+ TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, TableProperties.ORC_VECTOR_ROW_BATCH_SIZE_DEFAULT);
+ vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
+ VECTOR_ROW_BATCH_SIZE, vectorizedRowBatchSize);
Review comment:
updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825616904
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
Hi Ryan, what does 'old config' mean?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r824360952
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -186,35 +198,47 @@ public long blockSize() {
return blockSize;
}
- private Context(long stripeSize, long blockSize) {
+ public int vectorizedRowBatchSize() {
+ return vectorizedRowBatchSize;
+ }
+
+ private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
+ this.vectorizedRowBatchSize = vectorizedRowBatchSize;
}
- static Context dataContext(Configuration conf) {
- long stripeSize = TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT;
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getAttribute(), stripeSize);
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getHiveConfName(), stripeSize);
- stripeSize = conf.getLong(TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ static Context dataContext(Map<String, String> config) {
+ long stripeSize = PropertyUtil.propertyAsLong(config,
+ TableProperties.ORC_STRIPE_SIZE_BYTES, TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT);
+ Preconditions.checkArgument(stripeSize > 0, "Stripe size must be > 0");
- long blockSize = TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT;
- blockSize = conf.getLong(OrcConf.BLOCK_SIZE.getAttribute(), blockSize);
- blockSize = conf.getLong(OrcConf.BLOCK_SIZE.getHiveConfName(), blockSize);
- blockSize = conf.getLong(TableProperties.ORC_BLOCK_SIZE_BYTES, blockSize);
+ long blockSize = PropertyUtil.propertyAsLong(config,
+ TableProperties.ORC_BLOCK_SIZE_BYTES, TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT);
+ Preconditions.checkArgument(blockSize > 0, "Block size must be > 0");
- return new Context(stripeSize, blockSize);
+ int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
+ TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, TableProperties.ORC_VECTOR_ROW_BATCH_SIZE_DEFAULT);
+ vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
+ VECTOR_ROW_BATCH_SIZE, vectorizedRowBatchSize);
Review comment:
The old key which was configured in environment hadoop configuration still won't be parsed in the current design ? Because we just parse the table properties and never consider the hadoop configuration file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825480809
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
Okay, I see. This looks fine to me.
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
Compatibility with what? This was unreleased. I don't think that Iceberg should source configuration from the Hadoop Configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1067814695
> @hililiwei Would you mind to fix those travis CI issues ?
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823297752
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java
##########
@@ -123,7 +124,7 @@ public void writeFile() throws IOException {
// write in such a way that the file contains 10 stripes each with 100 rows
.set("iceberg.orc.vectorbatch.size", "100")
.set(OrcConf.ROWS_BETWEEN_CHECKS.getAttribute(), "100")
- .set(OrcConf.STRIPE_SIZE.getAttribute(), "1")
Review comment:
So looks like the default value of table property `write.orc.stripe-size-bytes` will override the set value from original ORC config key (`orc.stripe.size` ) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825480666
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
I agree that we should deprecate and keep supporting `iceberg.orc.vectorbatch.size` from the Hadoop conf since that slipped through review. But we can still clean this up with a reasonable table property.
The read property is `read.orc.vectorization.batch-size` so I think this should match: `write.orc.vectorization.batch-size`. It would also be good to use similar variable names, like `ORC_WRITE_BATCH_SIZE` and `ORC_DELETE_BATCH_SIZE`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r824411966
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
Get the value from the Hadoop conf for compatibility. @openinx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823294017
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -191,28 +198,26 @@ private Context(long stripeSize, long blockSize) {
this.blockSize = blockSize;
}
- static Context dataContext(Configuration conf) {
- long stripeSize = TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT;
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getAttribute(), stripeSize);
- stripeSize = conf.getLong(OrcConf.STRIPE_SIZE.getHiveConfName(), stripeSize);
- stripeSize = conf.getLong(TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ static Context dataContext(Map<String, String> conf) {
Review comment:
Could we always use `config` to indicate that we are referring a configure `Map<String, String>` ? The `conf` looks like a hadoop configuration in my mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823392512
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
We are replacing the old `iceberg.orc.vectorbatch.size` with the newly introduced `writer.orc.vectorized-row-batch-size` here , right ? In this way, the old table using the old config key won't take any effect in the next release. I think we'd better to keep the original approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei edited a comment on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei edited a comment on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1066270725
@openinx One thing to note. Although the k-v of config is copied to hadoop conf, if a standardized iceberg key, such as "orc.stripe.size", is set in conf using only the origin key, instead of the standardized key "write.orc.stripe-size-bytes", it will eventually be overwritten by iceberg's default value.
@zhongyujiang seems to be talking about this. I raised a similar question above, which you can refer to:https://github.com/apache/iceberg/pull/4291#discussion_r825481735
That is, for the config, if the key has been standardized, it is invalid to use the origin of the orc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823306890
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java
##########
@@ -123,7 +124,7 @@ public void writeFile() throws IOException {
// write in such a way that the file contains 10 stripes each with 100 rows
.set("iceberg.orc.vectorbatch.size", "100")
.set(OrcConf.ROWS_BETWEEN_CHECKS.getAttribute(), "100")
- .set(OrcConf.STRIPE_SIZE.getAttribute(), "1")
Review comment:
Yes, using the original ORC config key directly here is invalid and will be overwritten by the table property or by the table property default value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] zhongyujiang commented on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1066021871
Sorry for the delay.
About backwards compatibility, there is another issue I worried about. Users used to be able to set ORC props in Iceberg table properties like "orc.stripe.size" = '1', which could be passed directly to ORC to control stripe size, now we introduced standardized Iceberg ORC_STRIPE_SIZE_BYTES, but I think still we should not ignore those ORC properties which already had been set in table properties by users. I think we can use priorities like #3810, just pull the props from `config` instead of `conf`. What do you think?
The rest looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825481735
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java
##########
@@ -123,7 +124,7 @@ public void writeFile() throws IOException {
// write in such a way that the file contains 10 stripes each with 100 rows
.set("iceberg.orc.vectorbatch.size", "100")
.set(OrcConf.ROWS_BETWEEN_CHECKS.getAttribute(), "100")
- .set(OrcConf.STRIPE_SIZE.getAttribute(), "1")
Review comment:
I think the table property should override the Hadoop configuration property. We should probably also support the old `iceberg.orc.vectorbatch.size` property passed in like this. I think the table property should override it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1067697029
@hililiwei Would you mind to fix those travis CI issues ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1066264609
@zhongyujiang As we will still copy the configure key&values to the hadoop configuration here https://github.com/apache/iceberg/pull/4291/files#diff-34d7fce4c1d9417fa9342247cf5ace0636cd86591efbed0555ae80197f79303dR172-R174 . So I think the current patch could still meet your requirement about `Users used to be able to set ORC props in Iceberg table properties like "orc.stripe.size" = '1', which could be passed directly to ORC to control stripe size`, right ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r830872460
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
thx. I tried to reverted unit test changes. and read it from `config`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1063586068
cc @rdblue @openinx @zhongyujiang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823314117
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java
##########
@@ -123,7 +124,7 @@ public void writeFile() throws IOException {
// write in such a way that the file contains 10 stripes each with 100 rows
.set("iceberg.orc.vectorbatch.size", "100")
.set(OrcConf.ROWS_BETWEEN_CHECKS.getAttribute(), "100")
- .set(OrcConf.STRIPE_SIZE.getAttribute(), "1")
Review comment:
If the standard iceberg table properties is not found, do we feedback to try the origin ORC key?
From another perspective, this is not a hadoop configuration, but a subjective intention of the developer. It's just not used in a proper way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823424545
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
`iceberg.orc.vectorbatch.size` doesn't fit our naming conventions. Previously, it was a private variable and was not exposed as a public table configuration. In my humble opinion, if we formally expose it as a table configuration,
, would it be better to use a standard name?
> Config naming
> 1.Use - to link words in one concept
> For example, preferred convection access-key-id rather than access.key.id
> 2.Use . to create a hierarchy of config groups
> For example, s3 in s3.access-key-id, s3.secret-access-key
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r823521527
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,10 @@ private TableProperties() {
public static final String DELETE_ORC_BLOCK_SIZE_BYTES = "write.delete.orc.block-size-bytes";
public static final long ORC_BLOCK_SIZE_BYTES_DEFAULT = 256L * 1024 * 1024; // 256 MB
+ public static final String ORC_VECTOR_ROW_BATCH_SIZE = "write.orc.vectorized-row-batch-size";
Review comment:
Okay, I'll restore it, and let's do it separately later if we think it's necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825480053
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
Compatibility with what? This was unreleased. I don't think that Iceberg should source configuration from the Hadoop Configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei edited a comment on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
hililiwei edited a comment on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1066270725
@openinx One thing to note. Although the k-v of config is copied to hadoop conf, if a standardized iceberg key, such as "orc.stripe.size", is set in conf using only the origin key, instead of the standardized key "write.orc.stripe-size-bytes", it will eventually be overwritten by iceberg's default value.
@zhongyujiang seems to be talking about this.
That is, for the config, if the key has been standardized, it is invalid to use the origin of the orc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825481566
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
It looks like the old config could also be used in `set` on this builder. Should this support that case as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r825480826
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
This looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#issuecomment-1073705720
Got this merged, thanks @hililiwei for contribution, and thanks all for reviewing !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #4291: ORC:Optimize table properties usage for ORC
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4291:
URL: https://github.com/apache/iceberg/pull/4291#discussion_r830820232
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -153,30 +154,46 @@ public WriteBuilder metricsConfig(MetricsConfig newMetricsConfig) {
}
// supposed to always be a private method used strictly by data and delete write builders
- private WriteBuilder createContextFunc(Function<Configuration, Context> newCreateContextFunc) {
+ private WriteBuilder createContextFunc(Function<Map<String, String>, Context> newCreateContextFunc) {
this.createContextFunc = newCreateContextFunc;
return this;
}
public <D> FileAppender<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ 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());
+ }
+
+ // for compatibility
+ if (conf.get(VECTOR_ROW_BATCH_SIZE) != null && config.get(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE) == null) {
+ config.put(TableProperties.ORC_VECTOR_ROW_BATCH_SIZE, conf.get(VECTOR_ROW_BATCH_SIZE));
+ }
Review comment:
I think @rdblue means: we can also set the `iceberg.orc.vectorbatch.size` key in the iceberg table properties (Because we just `set` this key value into the hadoop configuration in the original version and then read this key from the hadoop configuration.)
We should also be able to read this config from `config` table properties if people just set it as the old approach.
In summary, we shouldn't need to change those unit tests config setting ([1], [2], [3]). if we keep the old usage.
[1]. https://github.com/apache/iceberg/pull/4291/files#diff-37a827d65fdb4bbf9ce2247d6839a846e27c371eb0bad58a47a4bc924170e2f7L124-L126
[2]. https://github.com/apache/iceberg/pull/4291/files#diff-8878d128d226b30d2941cd46b0e8c9aa193d752355288a6dc928af00d35bf811L124-L126
[3], https://github.com/apache/iceberg/pull/4291/files#diff-86cc5e259c85e8e36c90347a4623fd49e2e7a79d81ef4d8d3c8527111bfc8335L124-L126
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org