You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@parquet.apache.org by "amousavigourabi (via GitHub)" <gi...@apache.org> on 2023/09/27 22:34:17 UTC
[GitHub] [parquet-mr] amousavigourabi opened a new pull request, #1157: PARQUET-2351: Set options with Configuration
amousavigourabi opened a new pull request, #1157:
URL: https://github.com/apache/parquet-mr/pull/1157
Make sure you have checked _all_ steps below.
### Jira
- [x] My PR addresses the following [Parquet Jira](https://issues.apache.org/jira/browse/PARQUET/) issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-XXX
- In case you are adding a dependency, check if the license complies with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Parameterizes existing fixture to instantiate writer options using Configuration instead.
### Commits
- [x] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
---
The functionality is implemented in a way in which options set using the `builder.withXXX(...)` method will _always_ override the options passed by the configuration. This is done in order to not make things break in unexpected ways. Having a configuration which used to not have any effect on these options override an explicit option would be a bit odd after all.
--
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: dev-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PARQUET-2351: Set options with Configuration [parquet-mr]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #1157:
URL: https://github.com/apache/parquet-mr/pull/1157#discussion_r1359912407
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java:
##########
@@ -341,7 +341,7 @@ public static void setMaxPaddingSize(Configuration conf, int maxPaddingSize) {
conf.setInt(MAX_PADDING_BYTES, maxPaddingSize);
}
- private static int getMaxPaddingSize(Configuration conf) {
+ public static int getMaxPaddingSize(Configuration conf) {
Review Comment:
Changing method from private to public would need to add documentation and unit tests
--
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: dev-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-mr] wgtmac commented on a diff in pull request #1157: PARQUET-2351: Set options with Configuration
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1157:
URL: https://github.com/apache/parquet-mr/pull/1157#discussion_r1339538597
##########
parquet-hadoop/src/test/java/org/apache/parquet/filter2/dictionarylevel/DictionaryFilterTest.java:
##########
@@ -195,10 +200,28 @@ private static void prepareFile(WriterVersion version, Path file) throws IOExcep
writeData(f, writer);
}
+ private static void prepareFileWithConf(WriterVersion version, Path file) throws IOException {
+ Configuration configuration = new Configuration();
Review Comment:
It seems that these specific configurations defined in the ParquetOutputFormat are solely used for ParquetOutputFormat to create a RecordWriter, which actually puts all of them into a ParquetProperties.
https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java#L457-L484
IIUC, relying on settings from Hadoop configuration is discouraged, we should use ParquetProperties to set all those things.
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetWriter.java:
##########
@@ -358,9 +358,29 @@ public abstract static class Builder<T, SELF extends Builder<T, SELF>> {
private long rowGroupSize = DEFAULT_BLOCK_SIZE;
private int maxPaddingSize = MAX_PADDING_SIZE_DEFAULT;
private boolean enableValidation = DEFAULT_IS_VALIDATING_ENABLED;
- private ParquetProperties.Builder encodingPropsBuilder =
+ private final ParquetProperties.Builder encodingPropsBuilder =
ParquetProperties.builder();
+ private boolean isPageSizeSet = false;
Review Comment:
TBH, these would make the code less maintainable. Not sure if Optional would make them more organized.
--
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: dev-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-mr] amousavigourabi commented on a diff in pull request #1157: PARQUET-2351: Set options with Configuration
Posted by "amousavigourabi (via GitHub)" <gi...@apache.org>.
amousavigourabi commented on code in PR #1157:
URL: https://github.com/apache/parquet-mr/pull/1157#discussion_r1341182236
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetWriter.java:
##########
@@ -358,9 +358,29 @@ public abstract static class Builder<T, SELF extends Builder<T, SELF>> {
private long rowGroupSize = DEFAULT_BLOCK_SIZE;
private int maxPaddingSize = MAX_PADDING_SIZE_DEFAULT;
private boolean enableValidation = DEFAULT_IS_VALIDATING_ENABLED;
- private ParquetProperties.Builder encodingPropsBuilder =
+ private final ParquetProperties.Builder encodingPropsBuilder =
ParquetProperties.builder();
+ private boolean isPageSizeSet = false;
Review Comment:
I agree that these booleans aren't amazing. Optional fields would not solve the issue of making withXXX methods always override the configuration, next to this, Optionals shouldn't be used as fields IMO, as that is [not their intended use](https://mail.openjdk.org/pipermail/jdk8-dev/2013-September/003274.html). Maybe an approach with a Set would work better?
##########
parquet-hadoop/src/test/java/org/apache/parquet/filter2/dictionarylevel/DictionaryFilterTest.java:
##########
@@ -195,10 +200,28 @@ private static void prepareFile(WriterVersion version, Path file) throws IOExcep
writeData(f, writer);
}
+ private static void prepareFileWithConf(WriterVersion version, Path file) throws IOException {
+ Configuration configuration = new Configuration();
Review Comment:
These configurations are used to build the `encodingProps` ParquetProperties within ParquetWriter's Builder, which are used elsewhere as well. While the usage of Configuration in this way may be discouraged, the current situation of settings sometimes not being picked up by the ParquetWriter is inconsistent with both ParquetReader behaviour and user expectations.
--
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: dev-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PARQUET-2351: Set options with Configuration [parquet-mr]
Posted by "amousavigourabi (via GitHub)" <gi...@apache.org>.
amousavigourabi closed pull request #1157: PARQUET-2351: Set options with Configuration
URL: https://github.com/apache/parquet-mr/pull/1157
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2351: Set options with Configuration [parquet-mr]
Posted by "amousavigourabi (via GitHub)" <gi...@apache.org>.
amousavigourabi commented on code in PR #1157:
URL: https://github.com/apache/parquet-mr/pull/1157#discussion_r1360519336
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java:
##########
@@ -341,7 +341,7 @@ public static void setMaxPaddingSize(Configuration conf, int maxPaddingSize) {
conf.setInt(MAX_PADDING_BYTES, maxPaddingSize);
}
- private static int getMaxPaddingSize(Configuration conf) {
+ public static int getMaxPaddingSize(Configuration conf) {
Review Comment:
👍 I'll add it for the rest of the publics in the class as well while I'm at it.
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java:
##########
@@ -341,7 +341,7 @@ public static void setMaxPaddingSize(Configuration conf, int maxPaddingSize) {
conf.setInt(MAX_PADDING_BYTES, maxPaddingSize);
}
- private static int getMaxPaddingSize(Configuration conf) {
+ public static int getMaxPaddingSize(Configuration conf) {
Review Comment:
👍 I'll add it for the rest of the publics in the class as well while I'm at 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: dev-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PARQUET-2351: Set options with Configuration [parquet-mr]
Posted by "amousavigourabi (via GitHub)" <gi...@apache.org>.
amousavigourabi commented on code in PR #1157:
URL: https://github.com/apache/parquet-mr/pull/1157#discussion_r1360519336
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java:
##########
@@ -341,7 +341,7 @@ public static void setMaxPaddingSize(Configuration conf, int maxPaddingSize) {
conf.setInt(MAX_PADDING_BYTES, maxPaddingSize);
}
- private static int getMaxPaddingSize(Configuration conf) {
+ public static int getMaxPaddingSize(Configuration conf) {
Review Comment:
👍 I'll add javadoc to the rest of the publics in the class as well while I'm at 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: dev-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org