rdblue commented on code in PR #18269:
URL: https://github.com/apache/iceberg/pull/18269#discussion_r4125496930
##########
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);
Review Comment:
My reasoning was that the default should be handled by the default mode,
rather than producing a configured per-column mode that matches the default.
After looking at that failing test in `TestMetricsModes`, I think this is
the right approach. In that test, `col2` is part of the sort order, but also
has a configured mode in table properties. The problem is that configured mode
is `invalid`. Filling in the default because some mode was configured is
actually the wrong choice since the test shows that the expected mode was
`counts` instead of `truncate(16)` that is used for sort columns. So this
change allows the sort column mode to pass through when the mode is invalid.
--
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]
