rdblue commented on code in PR #18269:
URL: https://github.com/apache/iceberg/pull/18269#discussion_r4125596925
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -255,77 +255,112 @@ public Set<Integer> map(
* @return metrics configuration
*/
public static MetricsConfig from(Map<String, String> props, Schema schema,
SortOrder order) {
- int maxInferredDefaultColumns = maxInferredColumnDefaults(props);
- Map<Integer, String> idToName = Maps.newHashMap();
- Map<String, MetricsMode> columnModes = Maps.newHashMap();
+ int maxDefaultColumns = maxInferredColumnDefaults(props);
- // Handle user override of default mode
- MetricsMode defaultMode;
- String configuredDefault = props.get(DEFAULT_WRITE_METRICS_MODE);
+ // Handle configured default mode
+ MetricsMode configuredDefault = configuredDefault(props);
+ Map<String, MetricsMode> defaultColumnConf = defaultColumnModes(schema,
maxDefaultColumns);
+ MetricsMode defaultMode;
if (configuredDefault != null) {
- // a user-configured default mode is applied for all columns
- defaultMode = parseMode(configuredDefault, DEFAULT_MODE, "default");
- } else if (schema == null) {
+ defaultMode = configuredDefault;
+ } else if (defaultColumnConf.size() < maxDefaultColumns) {
+ // an additional column should use the default mode
defaultMode = DEFAULT_MODE;
} else {
- Set<Integer> ids = TypeUtil.getProjectedIds(schema);
- if (ids.size() <= maxInferredDefaultColumns) {
- for (int id : ids) {
- idToName.put(id, schema.findColumnName(id));
- }
+ // an additional column should not store metrics
+ defaultMode = MetricsModes.None.get();
+ }
- // there are less than the inferred limit (including structs), so the
default is used
- // everywhere
- defaultMode = DEFAULT_MODE;
- } else {
- for (Integer id : limitFieldIds(schema, maxInferredDefaultColumns)) {
- String name = schema.findColumnName(id);
- idToName.put(id, name);
- columnModes.put(name, DEFAULT_MODE);
- }
+ Map<String, MetricsMode> columnModes = Maps.newHashMap();
+
+ if (configuredDefault == null) {
+ columnModes.putAll(defaultColumnConf);
+ }
+
+ // Default sort columns to at least truncate (overridden by config)
+ columnModes.putAll(sortColumnModes(order, configuredDefault));
+
+ // Override automatic modes with configured modes
+ columnModes.putAll(configuredColumnModes(props));
+
+ Map<Integer, String> idToName = idToName(schema, columnModes);
+
+ return new MetricsConfig(columnModes, defaultMode, idToName);
+ }
+
+ private static MetricsMode configuredDefault(Map<String, String> props) {
+ String configuredDefault = props.get(DEFAULT_WRITE_METRICS_MODE);
+ if (configuredDefault != null) {
+ // a user-configured default mode is applied for all columns
+ return parseMode(configuredDefault, null, "default");
+ }
+
+ return null;
+ }
- // all other columns don't use metrics
- defaultMode = MetricsModes.None.get();
+ private static Map<String, MetricsMode> defaultColumnModes(Schema schema,
int maxColumns) {
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ if (schema != null) {
+ for (int id : limitFieldIds(schema, maxColumns)) {
+ builder.put(schema.findColumnName(id), DEFAULT_MODE);
}
}
- // First set sorted column with sorted column default (can be overridden
by user)
- MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(defaultMode);
- Set<String> sortedCols = SortOrderUtil.orderPreservingSortedColumns(order);
- sortedCols.forEach(
- name -> {
- columnModes.put(name, sortedColDefaultMode);
- Types.NestedField field = schema != null ? schema.findField(name) :
null;
- if (field != null) {
- idToName.put(field.fieldId(), name);
- }
- });
+ return builder.build();
+ }
+
+ private static Map<String, MetricsMode> sortColumnModes(
+ SortOrder order, MetricsMode configuredDefault) {
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ MetricsMode sortDefault = promoteToIncludeBounds(configuredDefault);
+ for (String name : SortOrderUtil.orderPreservingSortedColumns(order)) {
+ builder.put(name, sortDefault);
+ }
- // Handle user overrides of defaults
+ return builder.build();
+ }
+
+ private static Map<String, MetricsMode> configuredColumnModes(Map<String,
String> props) {
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
for (String key : props.keySet()) {
if (key.startsWith(METRICS_MODE_COLUMN_CONF_PREFIX)) {
String columnAlias = key.replaceFirst(METRICS_MODE_COLUMN_CONF_PREFIX,
"");
- MetricsMode mode = parseMode(props.get(key), defaultMode, "column " +
columnAlias);
- columnModes.put(columnAlias, mode);
- Types.NestedField field = schema != null ?
schema.findField(columnAlias) : null;
- if (field != null) {
- idToName.put(field.fieldId(), columnAlias);
+ MetricsMode mode = parseMode(props.get(key), null, "column " +
columnAlias);
+ if (mode != null) {
+ builder.put(columnAlias, mode);
}
}
}
- return new MetricsConfig(columnModes, defaultMode, idToName);
+ return builder.build();
+ }
+
+ private static Map<Integer, String> idToName(
+ Schema schema, Map<String, MetricsMode> columnModes) {
+ if (schema != null) {
+ ImmutableMap.Builder<Integer, String> builder = ImmutableMap.builder();
Review Comment:
I'll update this to `buildKeepingLast`, just in case there's a configured
field using a short name and a sort field using a canonical name.
I should mention that after this refactor, I think we should modify
`MetricsConfig` to be id-based rather than name-based. That should be an easier
change once we know that all of the callers (including tests) are using the
`forTable` 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: [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]
