RussellSpitzer commented on a change in pull request #3959:
URL: https://github.com/apache/iceberg/pull/3959#discussion_r791301333
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -194,7 +194,8 @@ private TableProperties() {
public static final String METRICS_MODE_COLUMN_CONF_PREFIX =
"write.metadata.metrics.column.";
public static final String DEFAULT_WRITE_METRICS_MODE =
"write.metadata.metrics.default";
- public static final String DEFAULT_WRITE_METRICS_MODE_DEFAULT =
"truncate(16)";
Review comment:
This is changing a public API and to a new type. May need to reconsider
this. Would probably be safer to just add the new variable below with a
slightly different name?
Although if other folks don't mind, I think that's fine.
##########
File path: core/src/main/java/org/apache/iceberg/MetricsConfig.java
##########
@@ -127,24 +135,39 @@ public static MetricsConfig forPositionDelete(Table
table) {
return new MetricsConfig(columnModes.build(), defaultMode);
}
- private static MetricsConfig from(Map<String, String> props, SortOrder
order) {
+ /**
+ * Generate a MetricsConfig for all columns based on overrides, sortOrder,
and defaultMode.
+ * @param props will be read for metrics overrides
(write.metadata.metrics.column.*) and default
+ * (write.metadata.metrics.default)
+ * @param order sort order columns, will be promoted to truncate(16)
+ * @param defaultMode MetricsConfig default, if not set by user property
+ * @return metrics configuration
+ */
+ private static MetricsConfig from(Map<String, String> props, SortOrder
order, MetricsMode defaultMode) {
Map<String, MetricsMode> columnModes = Maps.newHashMap();
- MetricsMode defaultMode;
- String defaultModeAsString =
props.getOrDefault(DEFAULT_WRITE_METRICS_MODE,
DEFAULT_WRITE_METRICS_MODE_DEFAULT);
- try {
- 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);
- defaultMode =
MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT);
+
+ // Handle user override of default mode
+ MetricsMode finalDefaultMode;
+ String defaultModeProp = props.get(DEFAULT_WRITE_METRICS_MODE);
+ if (defaultModeProp != null) {
+ try {
+ finalDefaultMode = MetricsModes.fromString(defaultModeProp);
+ } catch (IllegalArgumentException err) {
+ // User override was invalid, log the error and use the default
+ LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeProp,
err);
Review comment:
I'm not sure why we had this as a log warn before, seems like this
should just fail ... but I don't think we need to change the behavior yet
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -194,7 +194,8 @@ private TableProperties() {
public static final String METRICS_MODE_COLUMN_CONF_PREFIX =
"write.metadata.metrics.column.";
public static final String DEFAULT_WRITE_METRICS_MODE =
"write.metadata.metrics.default";
- public static final String DEFAULT_WRITE_METRICS_MODE_DEFAULT =
"truncate(16)";
Review comment:
This is changing a public API and to a new type. May need to reconsider
this. Would probably be safer to just add the new variable below with a
slightly different name?
Although if other folks don't mind, I think leaving this as is is also 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
