Core: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables - #17960
Conversation
|
|
||
| String name = schema.findColumnName(field.sourceId()); | ||
| if (name != null) { | ||
| columnModes.put(name, MetricsModes.Full.get()); |
There was a problem hiding this comment.
Parquet technically allows for a configurable truncation for an entire file (not per column) but it defaults to Int max so we're OK for now. https://github.com/apache/parquet-java/blob/master/parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java#L60. Since there's no API for finer grained guarantees from Parquet , we practically rely on the default from the parquet java library.
What we can do separately is add a unit test which just writes a big string/binary and in case the Parquet library changes out from under us, tests would fail and we would know. I didn't want to pollute this PR with that.
3642a03 to
8b9ed20
Compare
For v4 tables, collect untruncated metrics on the source column of any order-preserving partition transform, so bounds are usable to derive a partition value from stats instead of storing it explicitly. This is one of the primitives needed to eventually drop the v4 partition tuple. Note that we are not yet collecting information on bucket outputs.
8b9ed20 to
237c719
Compare
amogh-jahagirdar
changed the title
| @@ -118,7 +118,12 @@ public static void validate(Map<String, String> props, Schema schema) { | |||
| * @return a metrics config for the given table | |||
| */ | |||
| public static MetricsConfig forTable(Table table) { | |||
There was a problem hiding this comment.
This is the only called method for passing through to writers as far as I can tell. Technically writers can always choose to write with any partition spec, but I don't see it in the code base so I didn't bother adding a from(props, schema, partitionSpec, SortOrder) schema.
| fileIO = new InMemoryFileIO(); | ||
|
|
||
| table = mock(Table.class); | ||
| table = mock(Table.class, withSettings().extraInterfaces(HasTableOperations.class)); |
There was a problem hiding this comment.
Flagging why I'm touching the kafka-connect module tests here:
- MetricsConfig.forTable(table) now calls TableUtil.formatVersion(table), which only resolves for a Table that's a SerializableTable, HasTableOperations, or BaseMetadataTable. Anything else throws.
- I think that's a reasonable requirement. MetricsConfig tells writers what to collect, and if you're writing, you're going to commit, which needs TableOperations anyway. I didn't add a Preconditions check for it since the formatVersion exception already covers it if this assumption ever breaks but we can add one. I can't imagine a MetricsConfig being more generalizable than this use case?
WriterTestBase mocked a Table with no operations backing it and never had to satisfy this before, so I updated it to implement HasTableOperations with a stubbed format version. I think it's also reasonable to update this test because this test is touching the writer path.
a9612b3 to
9c28189
Compare
1 participant
For v4 tables, collect untruncated metrics on the source column of any order-preserving partition transform, so bounds are usable to derive a partition value from stats instead of storing it explicitly. This is one of the primitives needed to eventually drop the v4 partition tuple. Note that we are not yet collecting information on bucket outputs.
Currently, all file format writers explicitly call MetricsConfig.forTable to build MetricsConfig instances. This change just adds the logic to the implementation of that API to require collecting full/untruncated stats for source columns of partition fields for V4 tables. If a user specified property says to not collect those, that property is ignored as it is ultimately neccessary to have these stats for the goal of dropping the partition tuple write requirement in V4.