amogh-jahagirdar commented on code in PR #18269:
URL: https://github.com/apache/iceberg/pull/18269#discussion_r4115612993
##########
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:
I think CI is probably failing though due to this behavior change
e.g(`TestMetricsModes.testMetricsConfigSortedColsDefaultByInvalid`) so we'll
have to update that
##########
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:
This changes the fallback behavior, not sure if it's intentional (though I
think it's a better behavior now); previously, we'd fallback to the table level
default in case the configured value was invalid. I think this behavior change
is specific to sort columns where the table level default was `none` (which
presumably is pretty rare) or `counts` (also probably rare for a table level
default).
Anyways the case we're talking about is an in valid value, and if it's a
sort column then counts/none is a bad choice anyways so arguably the new
behavior is better.
##########
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();
+ for (String name : columnModes.keySet()) {
+ builder.put(schema.findField(name).fieldId(), name);
Review Comment:
if a field with `name` doesn't exist, this will NPE; think we just need to
add to the builder if the field exists, otherwise just ignore it. That looks
like that was the previous behavior as well, which is what I would expect.
##########
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:
This isn't a practical issue but if I follow the new logic is that there can
be duplicate field names across configured columns and columns whose metrics
mode we've inferred, and ImmutableMap would throw whereas before with the
regular Map, we'd override.
Since this only applies for short names for struct elements in lists (e.g.
points.x vs points.element.x), which there's no notion of bounds anyways, then
I think it's a non-issue (users wouldn't configure this anyways) but it is a
behavior difference compared to the current behavior which would just silently
accept rather than completely fail.
Here's a minimal test that I think demonstrates this:
```
@Test
public void shortNameColumnOverride() {
Schema schema =
new Schema(
optional(
1,
"points",
Types.ListType.ofOptional(
2, Types.StructType.of(optional(3, "x",
Types.LongType.get())))));
MetricsConfig config =
MetricsTestUtil.from(
ImmutableMap.of(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX +
"points.x", "none"),
schema);
assertThat(config.columnMode(3)).isEqualTo(MetricsModes.None.get());
}
```
--
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]
