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 16:07:03 UTC
[GitHub] [iceberg] szehon-ho commented on a change in pull request #3638: Ensure MetricsConfig is immutable
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