Deprecate and remove GenericAppenderFactory from tests #14353
Conversation
| Map<String, String> properties = table == null ? ImmutableMap.of() : table.properties(); | ||
| MetricsConfig metricsConfig = | ||
| table == null ? MetricsConfig.getDefault() : MetricsConfig.forTable(table); |
There was a problem hiding this comment.
need to handle null table, as a few use-cases for FileAppanderFactory uses it without an actual table
| import org.apache.iceberg.util.CharSequenceSet; | ||
| import org.apache.iceberg.util.CharSequenceWrapper; | ||
|
|
||
| class SortedPosDeleteWriter<T> implements FileWriter<PositionDelete<T>, DeleteWriteResult> { |
There was a problem hiding this comment.
not used, non public class.
Removed
| STRUCT_FIELD); | ||
|
|
||
| protected abstract FileAppender<T> writeAndGetAppender(List<Record> records) throws Exception; | ||
| protected abstract DataFile writeAndGetDataFile(List<Record> records) throws Exception; |
There was a problem hiding this comment.
Because of this change, we need to commit Spark 3.4/3.5, Flink 2.0/1.20 together, as all of them uses this as a base test. The other Spark and Flink versions are clean backports
| appender.addAll(records); | ||
| } finally { | ||
| appender.close(); | ||
| List<Record> records, File file, FileFormat fileFormat, Schema schema) throws IOException { |
There was a problem hiding this comment.
instead of the factory as an input, we create the factory internally using the schema information
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| private static EncryptedOutputFile encrypt(OutputFile out) { |
There was a problem hiding this comment.
Made the one in FileHelpers public, and used it everywhere
| new GenericFileWriterFactory.Builder() | ||
| .dataSchema(table.schema()) | ||
| .dataFileFormat(format) | ||
| .writerProperties(writeProperties) |
There was a problem hiding this comment.
The sad truth is that this test is not checking if the bloom filters are actually configured/created or not.
| return DEFAULT; | ||
| } | ||
|
|
||
| public static MetricsConfig getPositionDelete() { |
There was a problem hiding this comment.
nit: maybe forPositionDelete()?
There was a problem hiding this comment.
I was debating the same. The getPositionDelete won narrowly, because of the getDefault.
But taking a look at it with fresh eyes the forPositionDelete seems better.
Updated.
|
Merged to main. |
2 participants
No description provided.