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 2021/12/01 09:19:22 UTC
[GitHub] [iceberg] findepi opened a new pull request #3638: Ensure MetricsConfig is immutable
findepi opened a new pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638
`MetricsConfig` was effectively immutable from user perspective, because
all mutations were done in factory methods. Still, making the class
declare & enforce immutability helps the class users determine whether
it can be safety kept.
--
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] szehon-ho commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760329426
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -35,21 +38,22 @@
import static org.apache.iceberg.TableProperties.DEFAULT_WRITE_METRICS_MODE_DEFAULT;
import static org.apache.iceberg.TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX;
-public class MetricsConfig implements Serializable {
+@Immutable
+public final class MetricsConfig implements Serializable {
private static final Logger LOG = LoggerFactory.getLogger(MetricsConfig.class);
private static final Joiner DOT = Joiner.on('.');
- private Map<String, MetricsMode> columnModes = Maps.newHashMap();
- private MetricsMode defaultMode;
+ private final Map<String, MetricsMode> columnModes;
+ private final MetricsMode defaultMode;
- private MetricsConfig() {
+ private MetricsConfig(Map<String, MetricsMode> columnModes, MetricsMode defaultMode) {
+ this.columnModes = ImmutableMap.copyOf(columnModes);
Review comment:
can it take the immutableMap itself?
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
+ MetricsMode defaultMode;
String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT);
try {
- spec.defaultMode = MetricsModes.fromString(defaultModeAsString);
+ defaultMode = MetricsModes.fromString(defaultModeAsString);
} catch (IllegalArgumentException err) {
// Mode was invalid, log the error and use the default
LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeAsString, err);
- spec.defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
+ defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
}
// First set sorted column with sorted column default (can be overridden by user)
- MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(spec.defaultMode);
+ MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(defaultMode);
Set<String> sortedCols = SortOrderUtil.orderPreservingSortedColumns(order);
- sortedCols.forEach(sc -> spec.columnModes.put(sc, sortedColDefaultMode));
+ sortedCols.forEach(sc -> columnModes.put(sc, sortedColDefaultMode));
+ MetricsMode defaultModeFinal = defaultMode;
Review comment:
I guess overall it is not so clean, but given we already set the defaultMode before and below, we can just continue using the same variable instead of introducing a new one?
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
Review comment:
can we use ImmutableMap.of here?
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
Review comment:
Why not use the ImmutableMap builder like previous method?
--
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] findepi commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
findepi commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760588686
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -35,21 +38,22 @@
import static org.apache.iceberg.TableProperties.DEFAULT_WRITE_METRICS_MODE_DEFAULT;
import static org.apache.iceberg.TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX;
-public class MetricsConfig implements Serializable {
+@Immutable
+public final class MetricsConfig implements Serializable {
private static final Logger LOG = LoggerFactory.getLogger(MetricsConfig.class);
private static final Joiner DOT = Joiner.on('.');
- private Map<String, MetricsMode> columnModes = Maps.newHashMap();
- private MetricsMode defaultMode;
+ private final Map<String, MetricsMode> columnModes;
+ private final MetricsMode defaultMode;
- private MetricsConfig() {
+ private MetricsConfig(Map<String, MetricsMode> columnModes, MetricsMode defaultMode) {
+ this.columnModes = ImmutableMap.copyOf(columnModes);
Review comment:
`ImmutableMap.copyOf` is O(1) when value provided is an immutablemap already
--
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] jackye1995 commented on pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#issuecomment-988210539
@aokolnychyi ah totally forgot that. We can either:
1. change to use hash map, to keep it Kyro serializable
2. just make it not implement `Serializable`
3. keep it as is
IMO (1) seems like the preferred way, because we cannot change public interface to achieve (2), and if we go with (3) it might create a false impression that the object is Kyro serializable.
--
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] findepi commented on pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
findepi commented on pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#issuecomment-984389430
AC
--
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] jackye1995 commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760532987
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
Review comment:
+1, since there are only 2 values.
--
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] jackye1995 commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760602262
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
Review comment:
in that case could you move to `Maps.newHashMap`?
--
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] jackye1995 commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760467624
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -20,12 +20,15 @@
package org.apache.iceberg;
import java.io.Serializable;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import javax.annotation.concurrent.Immutable;
Review comment:
We have used `javax.annotation.Nullable` in `mr` and `spark` modules.
Based on https://stackoverflow.com/questions/27101978/what-is-the-advantage-of-annotating-an-immutable-java-class-with-immutable, looks like the main benefit is documentation and findbug.
So I think this should be fine.
--
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] jackye1995 commented on pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#issuecomment-985156977
I think there is no additional comment unresolved, merge it, thanks for the work Piotr!
--
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] aokolnychyi commented on pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#issuecomment-985782605
I think it is fine for Spark right now but I am not sure about other query engines. It is worth checking with @openinx and @pvary.
--
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] RussellSpitzer commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760503492
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -35,21 +38,22 @@
import static org.apache.iceberg.TableProperties.DEFAULT_WRITE_METRICS_MODE_DEFAULT;
import static org.apache.iceberg.TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX;
-public class MetricsConfig implements Serializable {
+@Immutable
+public final class MetricsConfig implements Serializable {
private static final Logger LOG = LoggerFactory.getLogger(MetricsConfig.class);
private static final Joiner DOT = Joiner.on('.');
- private Map<String, MetricsMode> columnModes = Maps.newHashMap();
- private MetricsMode defaultMode;
+ private final Map<String, MetricsMode> columnModes;
+ private final MetricsMode defaultMode;
- private MetricsConfig() {
+ private MetricsConfig(Map<String, MetricsMode> columnModes, MetricsMode defaultMode) {
+ this.columnModes = ImmutableMap.copyOf(columnModes);
+ this.defaultMode = defaultMode;
}
public static MetricsConfig getDefault() {
Review comment:
Not important for now, but seems like this should return a singleton since these are immutable, so there is only ever one 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] findepi commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
findepi commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760588979
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
Review comment:
we cannot use ImmutableMap builder, because we write and then overwrite certain keys in the map
--
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] aokolnychyi commented on pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#issuecomment-985776835
Will usage of Guava affect Kryo serialization? We may be reconstructing `MetricsConfig` after table deserialization right now but we may have use cases in the future as this class is marked as serializable.
Any thoughts, @RussellSpitzer @jackye1995 @findepi?
--
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] aokolnychyi commented on pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#issuecomment-987987248
@jackye1995, it is marked serializable as we previously did not have a serializable table and passed such things separately.
--
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] jackye1995 commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r761600091
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
Review comment:
I think this will need to be a builder because it adds additional column modes at L123, resolve the comment.
--
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] jackye1995 commented on pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#issuecomment-985822820
Actually I was just discussing this with Russell offline in the morning. I searched the codebase and did not see any place that this is used for serialization. The data writer always construct the configs from the serialized table.
@aokolnychyi do you have any context why this was marked as serializable?
--
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] RussellSpitzer commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760370554
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -20,12 +20,15 @@
package org.apache.iceberg;
import java.io.Serializable;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import javax.annotation.concurrent.Immutable;
Review comment:
Is this the first usage of the annotation in the project? What are the benefits of using 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] jackye1995 merged pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638
--
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] findepi commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
findepi commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760591397
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -20,12 +20,15 @@
package org.apache.iceberg;
import java.io.Serializable;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import javax.annotation.concurrent.Immutable;
Review comment:
> What are the benefits of using it?
documentation
> Is this the first usage of the annotation in the project?
i didn't check, but i can remove.
still, users of a class are interested in immutability/mutability properties of the class -- not only current properties, as implemented, but also about the intent and so: guarantees -- so some way to signal the intent would be nice. can be annotation, or can be a note in javadoc.
--
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] RussellSpitzer commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760506598
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
+ MetricsMode defaultMode;
String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT);
try {
- spec.defaultMode = MetricsModes.fromString(defaultModeAsString);
+ defaultMode = MetricsModes.fromString(defaultModeAsString);
} catch (IllegalArgumentException err) {
// Mode was invalid, log the error and use the default
LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeAsString, err);
- spec.defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
+ defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
}
// First set sorted column with sorted column default (can be overridden by user)
- MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(spec.defaultMode);
+ MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(defaultMode);
Set<String> sortedCols = SortOrderUtil.orderPreservingSortedColumns(order);
- sortedCols.forEach(sc -> spec.columnModes.put(sc, sortedColDefaultMode));
+ sortedCols.forEach(sc -> columnModes.put(sc, sortedColDefaultMode));
+ MetricsMode defaultModeFinal = defaultMode;
Review comment:
I thought it just has to be effectively final, this makes it a bit confusing below where we use "defaultModeFinal" and "defaultMode". I think this is fine if the compiler wants it but let's use the same variable in both spots below
```java
mode = defaultModeFinal
```
and
```java
return new MetricsConfig(columnModes, defaultModeFinal);
```
--
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] jackye1995 commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760468133
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
Review comment:
+1, and we prefer `Maps.newHashMap` over raw constructor for hash map.
--
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] findepi commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
findepi commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760590028
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
+ MetricsMode defaultMode;
String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT);
try {
- spec.defaultMode = MetricsModes.fromString(defaultModeAsString);
+ defaultMode = MetricsModes.fromString(defaultModeAsString);
} catch (IllegalArgumentException err) {
// Mode was invalid, log the error and use the default
LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeAsString, err);
- spec.defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
+ defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
}
// First set sorted column with sorted column default (can be overridden by user)
- MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(spec.defaultMode);
+ MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(defaultMode);
Set<String> sortedCols = SortOrderUtil.orderPreservingSortedColumns(order);
- sortedCols.forEach(sc -> spec.columnModes.put(sc, sortedColDefaultMode));
+ sortedCols.forEach(sc -> columnModes.put(sc, sortedColDefaultMode));
+ MetricsMode defaultModeFinal = defaultMode;
Review comment:
> I thought it just has to be effectively final
correct. and seems it needs to be assigned once, and here it's assigned in an `if`.
> `return new MetricsConfig(columnModes, defaultModeFinal);`
the variable `defaultModeFinal` exists for lambda only. it's not meant to change logic flow, or supersede the existing variable, once assigned. that's why i'd use it only for the lambda and "forget" later.
--
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] jackye1995 commented on a change in pull request #3638: Ensure MetricsConfig is immutable
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3638:
URL: https://github.com/apache/iceberg/pull/3638#discussion_r760468758
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -104,38 +108,40 @@ public static MetricsConfig forTable(Table table) {
* @param table an Iceberg table
*/
public static MetricsConfig forPositionDelete(Table table) {
- MetricsConfig config = new MetricsConfig();
+ ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
- config.columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
- config.columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
+ columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
MetricsConfig tableConfig = forTable(table);
- config.defaultMode = tableConfig.defaultMode;
+ MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach((columnAlias, mode) -> {
String positionDeleteColumnAlias = DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
- config.columnModes.put(positionDeleteColumnAlias, mode);
+ columnModes.put(positionDeleteColumnAlias, mode);
});
- return config;
+ return new MetricsConfig(columnModes.build(), defaultMode);
}
private static MetricsConfig from(Map<String, String> props, SortOrder order) {
- MetricsConfig spec = new MetricsConfig();
+ Map<String, MetricsMode> columnModes = new HashMap<>();
+ MetricsMode defaultMode;
String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT);
try {
- spec.defaultMode = MetricsModes.fromString(defaultModeAsString);
+ defaultMode = MetricsModes.fromString(defaultModeAsString);
} catch (IllegalArgumentException err) {
// Mode was invalid, log the error and use the default
LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeAsString, err);
- spec.defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
+ defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
}
// First set sorted column with sorted column default (can be overridden by user)
- MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(spec.defaultMode);
+ MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(defaultMode);
Set<String> sortedCols = SortOrderUtil.orderPreservingSortedColumns(order);
- sortedCols.forEach(sc -> spec.columnModes.put(sc, sortedColDefaultMode));
+ sortedCols.forEach(sc -> columnModes.put(sc, sortedColDefaultMode));
+ MetricsMode defaultModeFinal = defaultMode;
Review comment:
I think it's because the object passed to lambda must not be mutated.
--
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