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/05 15:43:25 UTC
[GitHub] [iceberg] zhongyujiang opened a new pull request #4273: ORC: Add compression properties
zhongyujiang opened a new pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273
This PR adds compression-codec and compression-strategy of ORC in `TableProperties`, @openinx @hililiwei would you help review this? Thanks!
--
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 a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r838140470
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,14 @@ 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_COMPRESSION = "write.orc.compression-codec";
+ public static final String DELETE_ORC_COMPRESSION = "write.delete.orc.compression-codec";
+ public static final String ORC_COMPRESSION_DEFAULT = "zlib";
Review comment:
Although I believe ZSTD has been used in production by many users already, I personally prefer to align with the ORC default. Besides, I think we'd better start a new thread to discuss changing default codec.
--
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 a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r838140470
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,14 @@ 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_COMPRESSION = "write.orc.compression-codec";
+ public static final String DELETE_ORC_COMPRESSION = "write.delete.orc.compression-codec";
+ public static final String ORC_COMPRESSION_DEFAULT = "zlib";
Review comment:
Although I believe ZSTD has been used in production by many users already, I personally prefer to align with the ORC default. Besides, I think we'd better start a new thread to discuss 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] zhongyujiang commented on pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#issuecomment-1082635432
Updated based on comments.
--
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] amogh-jahagirdar commented on a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r821193851
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,14 @@ 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_COMPRESSION = "write.orc.compression-codec";
+ public static final String DELETE_ORC_COMPRESSION = "write.delete.orc.compression-codec";
+ public static final String ORC_COMPRESSION_DEFAULT = "zlib";
Review comment:
I think ORC as a format does not support GZIP. https://orc.apache.org/api/orc-core/org/apache/orc/CompressionKind.html
Engines like Presto/Trino tend to have ZLIB as a default compression mode for ORC. Under the hood for both GZIP/Zlib is the same "Deflate" compression algorithm.
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#issuecomment-1080588488
Updated based on #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] rdblue commented on a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r821148502
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,14 @@ 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_COMPRESSION = "write.orc.compression-codec";
+ public static final String DELETE_ORC_COMPRESSION = "write.delete.orc.compression-codec";
+ public static final String ORC_COMPRESSION_DEFAULT = "zlib";
Review comment:
Other formats use "gzip" for this. Is this what ORC requires?
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r836999320
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,14 @@ 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_COMPRESSION = "write.orc.compression-codec";
+ public static final String DELETE_ORC_COMPRESSION = "write.delete.orc.compression-codec";
+ public static final String ORC_COMPRESSION_DEFAULT = "zlib";
Review comment:
Given `ZSTD`'s good performance, are we considering it as the default?
--
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 pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#issuecomment-1061198500
I don't think that we can merge this until we fix the ORC configuration PR that recently went in. See https://github.com/apache/iceberg/pull/3810#issuecomment-1061197742 for more details. The configuration context should not use 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] openinx commented on a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r836994947
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -206,43 +227,64 @@ public int vectorizedRowBatchSize() {
return vectorizedRowBatchSize;
}
- private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize) {
+ public String codecAsString() {
+ return codecAsString;
+ }
+
+ public String compressionStrategy() {
+ return compressionStrategy;
+ }
+
+ private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize,
+ String codecAsString, String compressionStrategy) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
this.vectorizedRowBatchSize = vectorizedRowBatchSize;
+ this.codecAsString = codecAsString;
+ this.compressionStrategy = compressionStrategy;
}
static Context dataContext(Map<String, String> config) {
long stripeSize = PropertyUtil.propertyAsLong(config, OrcConf.STRIPE_SIZE.getAttribute(),
- TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT);
- stripeSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ ORC_STRIPE_SIZE_BYTES_DEFAULT);
+ stripeSize = PropertyUtil.propertyAsLong(config, ORC_STRIPE_SIZE_BYTES, stripeSize);
Preconditions.checkArgument(stripeSize > 0, "Stripe size must be > 0");
long blockSize = PropertyUtil.propertyAsLong(config, OrcConf.BLOCK_SIZE.getAttribute(),
- TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT);
- blockSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_BLOCK_SIZE_BYTES, blockSize);
+ ORC_BLOCK_SIZE_BYTES_DEFAULT);
+ blockSize = PropertyUtil.propertyAsLong(config, ORC_BLOCK_SIZE_BYTES, blockSize);
Preconditions.checkArgument(blockSize > 0, "Block size must be > 0");
int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
- TableProperties.ORC_WRITE_BATCH_SIZE, TableProperties.ORC_WRITE_BATCH_SIZE_DEFAULT);
+ ORC_WRITE_BATCH_SIZE, ORC_WRITE_BATCH_SIZE_DEFAULT);
Preconditions.checkArgument(vectorizedRowBatchSize > 0, "VectorizedRow batch size must be > 0");
- return new Context(stripeSize, blockSize, vectorizedRowBatchSize);
+ String codecAsString = config.getOrDefault(OrcConf.COMPRESS.getAttribute(), ORC_COMPRESSION_DEFAULT);
+ codecAsString = config.getOrDefault(ORC_COMPRESSION, codecAsString);
Review comment:
I will suggest to use the similar approach as `parquet` did: try to parse the string into a `CompressionKind` so that we can validate whether this value is a valid one in the correct place.
See: https://github.com/apache/iceberg/blob/master/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L392
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r821150365
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -186,35 +204,63 @@ public long blockSize() {
return blockSize;
}
- private Context(long stripeSize, long blockSize) {
+ public String codecAsString() {
+ return codecAsString;
+ }
+
+ public String compressionStrategy() {
+ return compressionStrategy;
+ }
+
+ private Context(long stripeSize, long blockSize, String codecAsString, String compressionStrategy) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
+ this.codecAsString = codecAsString;
+ this.compressionStrategy = compressionStrategy;
}
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);
+ String stripeSizeStr = lookupValue(conf, OrcConf.STRIPE_SIZE, ORC_STRIPE_SIZE_BYTES);
+ long stripeSize = stripeSizeStr != null ? Long.parseLong(stripeSizeStr) : ORC_STRIPE_SIZE_BYTES_DEFAULT;
+
+ String blockSizeStr = lookupValue(conf, OrcConf.BLOCK_SIZE, ORC_BLOCK_SIZE_BYTES);
+ long blockSize = blockSizeStr != null ? Long.parseLong(blockSizeStr) : ORC_BLOCK_SIZE_BYTES_DEFAULT;
- 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);
+ String codecAsString = lookupValue(conf, OrcConf.COMPRESS, ORC_COMPRESSION);
+ if (codecAsString == null) {
+ codecAsString = ORC_COMPRESSION_DEFAULT;
+ }
- return new Context(stripeSize, blockSize);
+ String compressionStrategy = lookupValue(conf, OrcConf.COMPRESSION_STRATEGY, ORC_COMPRESSION_STRATEGY);
+ if (compressionStrategy == null) {
+ compressionStrategy = ORC_COMPRESSION_STRATEGY_DEFAULT;
+ }
+
+ return new Context(stripeSize, blockSize, codecAsString, compressionStrategy);
}
static Context deleteContext(Configuration conf) {
Context dataContext = dataContext(conf);
- long stripeSize =
- conf.getLong(TableProperties.DELETE_ORC_STRIPE_SIZE_BYTES, dataContext.stripeSize());
+ long stripeSize = conf.getLong(DELETE_ORC_STRIPE_SIZE_BYTES, dataContext.stripeSize());
+ long blockSize = conf.getLong(DELETE_ORC_BLOCK_SIZE_BYTES, dataContext.stripeSize());
+ String codecAsString = conf.get(DELETE_ORC_COMPRESSION, dataContext.codecAsString());
+ String compressionStrategy = conf.get(DELETE_ORC_COMPRESSION_STRATEGY, dataContext.compressionStrategy());
- long blockSize =
- conf.getLong(TableProperties.DELETE_ORC_BLOCK_SIZE_BYTES, dataContext.stripeSize());
+ return new Context(stripeSize, blockSize, codecAsString, compressionStrategy);
+ }
- return new Context(stripeSize, blockSize);
+ static String lookupValue(Configuration conf, OrcConf orcConf, String icebergPropertyKey) {
Review comment:
Something is wrong if this is looking up an Iceberg table property like `ORC_STRIPE_SIZE_BYTES` in a Hadoop Configuration. Iceberg settings should be pulled from table properties, not from Hadoop.
--
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 a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r838130379
##########
File path: orc/src/test/java/org/apache/iceberg/orc/TestTableProperties.java
##########
@@ -82,19 +93,29 @@ public void testOrcTableProperties() throws Exception {
DynFields.builder().hiddenImpl(writer.getClass(), "conf").build(writer);
Configuration configuration = confField.get();
- Assert.assertEquals(String.valueOf(blockSizeBytes), configuration.get(OrcConf.BLOCK_SIZE.getAttribute()));
- Assert.assertEquals(String.valueOf(stripeSizeBytes), configuration.get(OrcConf.STRIPE_SIZE.getAttribute()));
Review comment:
Changed this to using OrcConf to get value directly so no need to know property key detail.
--
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] amogh-jahagirdar commented on a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r821193851
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,14 @@ 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_COMPRESSION = "write.orc.compression-codec";
+ public static final String DELETE_ORC_COMPRESSION = "write.delete.orc.compression-codec";
+ public static final String ORC_COMPRESSION_DEFAULT = "zlib";
Review comment:
I think ORC as a format does not support GZIP. https://orc.apache.org/api/orc-core/org/apache/orc/CompressionKind.html
Engines like Presto/Trino tend to have ZLIB as a default compression mode for 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] zhongyujiang commented on a change in pull request #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r838133910
##########
File path: orc/src/test/java/org/apache/iceberg/orc/TestTableProperties.java
##########
@@ -53,12 +56,20 @@
@Test
public void testOrcTableProperties() throws Exception {
- Long stripeSizeBytes = 32L * 1024 * 1024;
- Long blockSizeBytes = 128L * 1024 * 1024;
Review comment:
Changed `Long` to `long` because what `OrcConf.XXX#getLong` returns is a `long`, and I got
> Ambiguous method call. Both assertEquals(Object, Object) in Assert and assertEquals(long, long) in Assert match
when using both `Long` and `long` as parameters for `Assert#assertEquals`.
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
openinx merged pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r838158725
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -143,6 +143,14 @@ 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_COMPRESSION = "write.orc.compression-codec";
+ public static final String DELETE_ORC_COMPRESSION = "write.delete.orc.compression-codec";
+ public static final String ORC_COMPRESSION_DEFAULT = "zlib";
Review comment:
Yeah, I agree with @zhongyujiang, it's good to align with the apache orc default value.
According to the document https://orc.apache.org/docs/hive-config.html .
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r836996457
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -206,43 +227,64 @@ public int vectorizedRowBatchSize() {
return vectorizedRowBatchSize;
}
- private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize) {
+ public String codecAsString() {
+ return codecAsString;
+ }
+
+ public String compressionStrategy() {
+ return compressionStrategy;
+ }
+
+ private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize,
+ String codecAsString, String compressionStrategy) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
this.vectorizedRowBatchSize = vectorizedRowBatchSize;
+ this.codecAsString = codecAsString;
+ this.compressionStrategy = compressionStrategy;
}
static Context dataContext(Map<String, String> config) {
long stripeSize = PropertyUtil.propertyAsLong(config, OrcConf.STRIPE_SIZE.getAttribute(),
- TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT);
- stripeSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ ORC_STRIPE_SIZE_BYTES_DEFAULT);
+ stripeSize = PropertyUtil.propertyAsLong(config, ORC_STRIPE_SIZE_BYTES, stripeSize);
Preconditions.checkArgument(stripeSize > 0, "Stripe size must be > 0");
long blockSize = PropertyUtil.propertyAsLong(config, OrcConf.BLOCK_SIZE.getAttribute(),
- TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT);
- blockSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_BLOCK_SIZE_BYTES, blockSize);
+ ORC_BLOCK_SIZE_BYTES_DEFAULT);
+ blockSize = PropertyUtil.propertyAsLong(config, ORC_BLOCK_SIZE_BYTES, blockSize);
Preconditions.checkArgument(blockSize > 0, "Block size must be > 0");
int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
- TableProperties.ORC_WRITE_BATCH_SIZE, TableProperties.ORC_WRITE_BATCH_SIZE_DEFAULT);
+ ORC_WRITE_BATCH_SIZE, ORC_WRITE_BATCH_SIZE_DEFAULT);
Preconditions.checkArgument(vectorizedRowBatchSize > 0, "VectorizedRow batch size must be > 0");
- return new Context(stripeSize, blockSize, vectorizedRowBatchSize);
+ String codecAsString = config.getOrDefault(OrcConf.COMPRESS.getAttribute(), ORC_COMPRESSION_DEFAULT);
+ codecAsString = config.getOrDefault(ORC_COMPRESSION, codecAsString);
+
+ String compressionStrategy = config.getOrDefault(OrcConf.COMPRESSION_STRATEGY.getAttribute(),
+ ORC_COMPRESSION_STRATEGY_DEFAULT);
+ compressionStrategy = config.getOrDefault(ORC_COMPRESSION_STRATEGY, compressionStrategy);
+
+ return new Context(stripeSize, blockSize, vectorizedRowBatchSize, codecAsString, compressionStrategy);
}
static Context deleteContext(Map<String, String> config) {
Context dataContext = dataContext(config);
- long stripeSize = PropertyUtil.propertyAsLong(config,
- TableProperties.DELETE_ORC_STRIPE_SIZE_BYTES, dataContext.stripeSize());
+ long stripeSize = PropertyUtil.propertyAsLong(config, DELETE_ORC_STRIPE_SIZE_BYTES, dataContext.stripeSize());
- long blockSize = PropertyUtil.propertyAsLong(config,
- TableProperties.DELETE_ORC_BLOCK_SIZE_BYTES, dataContext.blockSize());
+ long blockSize = PropertyUtil.propertyAsLong(config, DELETE_ORC_BLOCK_SIZE_BYTES, dataContext.blockSize());
int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
- TableProperties.DELETE_ORC_WRITE_BATCH_SIZE, dataContext.vectorizedRowBatchSize());
+ DELETE_ORC_WRITE_BATCH_SIZE, dataContext.vectorizedRowBatchSize());
+
+ String codecAsString = config.getOrDefault(DELETE_ORC_COMPRESSION, dataContext.codecAsString());
+
+ String compressionStrategy = config.getOrDefault(DELETE_ORC_COMPRESSION_STRATEGY,
+ dataContext.compressionStrategy());
Review comment:
Nit: Please use the same approach to parse the `codec` & `compression-strategy` .
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r836995620
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -206,43 +227,64 @@ public int vectorizedRowBatchSize() {
return vectorizedRowBatchSize;
}
- private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize) {
+ public String codecAsString() {
+ return codecAsString;
+ }
+
+ public String compressionStrategy() {
+ return compressionStrategy;
+ }
+
+ private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize,
+ String codecAsString, String compressionStrategy) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
this.vectorizedRowBatchSize = vectorizedRowBatchSize;
+ this.codecAsString = codecAsString;
+ this.compressionStrategy = compressionStrategy;
}
static Context dataContext(Map<String, String> config) {
long stripeSize = PropertyUtil.propertyAsLong(config, OrcConf.STRIPE_SIZE.getAttribute(),
- TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT);
- stripeSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ ORC_STRIPE_SIZE_BYTES_DEFAULT);
+ stripeSize = PropertyUtil.propertyAsLong(config, ORC_STRIPE_SIZE_BYTES, stripeSize);
Preconditions.checkArgument(stripeSize > 0, "Stripe size must be > 0");
long blockSize = PropertyUtil.propertyAsLong(config, OrcConf.BLOCK_SIZE.getAttribute(),
- TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT);
- blockSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_BLOCK_SIZE_BYTES, blockSize);
+ ORC_BLOCK_SIZE_BYTES_DEFAULT);
+ blockSize = PropertyUtil.propertyAsLong(config, ORC_BLOCK_SIZE_BYTES, blockSize);
Preconditions.checkArgument(blockSize > 0, "Block size must be > 0");
int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
- TableProperties.ORC_WRITE_BATCH_SIZE, TableProperties.ORC_WRITE_BATCH_SIZE_DEFAULT);
+ ORC_WRITE_BATCH_SIZE, ORC_WRITE_BATCH_SIZE_DEFAULT);
Preconditions.checkArgument(vectorizedRowBatchSize > 0, "VectorizedRow batch size must be > 0");
- return new Context(stripeSize, blockSize, vectorizedRowBatchSize);
+ String codecAsString = config.getOrDefault(OrcConf.COMPRESS.getAttribute(), ORC_COMPRESSION_DEFAULT);
+ codecAsString = config.getOrDefault(ORC_COMPRESSION, codecAsString);
+
+ String compressionStrategy = config.getOrDefault(OrcConf.COMPRESSION_STRATEGY.getAttribute(),
+ ORC_COMPRESSION_STRATEGY_DEFAULT);
+ compressionStrategy = config.getOrDefault(ORC_COMPRESSION_STRATEGY, compressionStrategy);
Review comment:
Besides, please also use the `CompressionStrategy` enum to validate whether this configured value is a valid one ? ( Just as this comment said: https://github.com/apache/iceberg/pull/4273/files#r836994947 )
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#discussion_r836993222
##########
File path: orc/src/main/java/org/apache/iceberg/orc/ORC.java
##########
@@ -206,43 +227,64 @@ public int vectorizedRowBatchSize() {
return vectorizedRowBatchSize;
}
- private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize) {
+ public String codecAsString() {
+ return codecAsString;
+ }
+
+ public String compressionStrategy() {
+ return compressionStrategy;
+ }
+
+ private Context(long stripeSize, long blockSize, int vectorizedRowBatchSize,
+ String codecAsString, String compressionStrategy) {
this.stripeSize = stripeSize;
this.blockSize = blockSize;
this.vectorizedRowBatchSize = vectorizedRowBatchSize;
+ this.codecAsString = codecAsString;
+ this.compressionStrategy = compressionStrategy;
}
static Context dataContext(Map<String, String> config) {
long stripeSize = PropertyUtil.propertyAsLong(config, OrcConf.STRIPE_SIZE.getAttribute(),
- TableProperties.ORC_STRIPE_SIZE_BYTES_DEFAULT);
- stripeSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_STRIPE_SIZE_BYTES, stripeSize);
+ ORC_STRIPE_SIZE_BYTES_DEFAULT);
+ stripeSize = PropertyUtil.propertyAsLong(config, ORC_STRIPE_SIZE_BYTES, stripeSize);
Preconditions.checkArgument(stripeSize > 0, "Stripe size must be > 0");
long blockSize = PropertyUtil.propertyAsLong(config, OrcConf.BLOCK_SIZE.getAttribute(),
- TableProperties.ORC_BLOCK_SIZE_BYTES_DEFAULT);
- blockSize = PropertyUtil.propertyAsLong(config, TableProperties.ORC_BLOCK_SIZE_BYTES, blockSize);
+ ORC_BLOCK_SIZE_BYTES_DEFAULT);
+ blockSize = PropertyUtil.propertyAsLong(config, ORC_BLOCK_SIZE_BYTES, blockSize);
Preconditions.checkArgument(blockSize > 0, "Block size must be > 0");
int vectorizedRowBatchSize = PropertyUtil.propertyAsInt(config,
- TableProperties.ORC_WRITE_BATCH_SIZE, TableProperties.ORC_WRITE_BATCH_SIZE_DEFAULT);
+ ORC_WRITE_BATCH_SIZE, ORC_WRITE_BATCH_SIZE_DEFAULT);
Preconditions.checkArgument(vectorizedRowBatchSize > 0, "VectorizedRow batch size must be > 0");
- return new Context(stripeSize, blockSize, vectorizedRowBatchSize);
+ String codecAsString = config.getOrDefault(OrcConf.COMPRESS.getAttribute(), ORC_COMPRESSION_DEFAULT);
+ codecAsString = config.getOrDefault(ORC_COMPRESSION, codecAsString);
+
+ String compressionStrategy = config.getOrDefault(OrcConf.COMPRESSION_STRATEGY.getAttribute(),
+ ORC_COMPRESSION_STRATEGY_DEFAULT);
+ compressionStrategy = config.getOrDefault(ORC_COMPRESSION_STRATEGY, compressionStrategy);
Review comment:
Why not just follow the same config parse approach as the above config keys did ? I'm saying using the generic `PropertyUtil.propertyAsString`.
--
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 #4273: ORC: Add compression properties
Posted by GitBox <gi...@apache.org>.
zhongyujiang commented on pull request #4273:
URL: https://github.com/apache/iceberg/pull/4273#issuecomment-1083990842
Thanks all for your review!
--
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