aokolnychyi commented on a change in pull request #2214:
URL: https://github.com/apache/iceberg/pull/2214#discussion_r598998969
##########
File path: core/src/main/java/org/apache/iceberg/io/DataWriter.java
##########
@@ -37,16 +38,23 @@
private final PartitionSpec spec;
private final StructLike partition;
private final ByteBuffer keyMetadata;
+ private final SortOrder sortOrder;
private DataFile dataFile = null;
public DataWriter(FileAppender<T> appender, FileFormat format, String
location,
PartitionSpec spec, StructLike partition,
EncryptionKeyMetadata keyMetadata) {
+ this(appender, format, location, spec, partition, keyMetadata, null);
+ }
+
+ public DataWriter(FileAppender<T> appender, FileFormat format, String
location,
Review comment:
We instantiate `DataWriter` in `FileAppenderFactory` implementations. It
is absolutely correct that we don't propagate the sort order id now. However,
should we consider adding a comment that explains why? Like we cannot guarantee
the incoming records are sorted as needed?
##########
File path: core/src/main/java/org/apache/iceberg/avro/Avro.java
##########
@@ -284,6 +286,11 @@ public DeleteWriteBuilder equalityFieldIds(int...
fieldIds) {
return this;
}
+ public DeleteWriteBuilder withSortOrder(SortOrder newSortOrder) {
Review comment:
If we decide to do that, we should probably cover ORC.
##########
File path: core/src/main/java/org/apache/iceberg/avro/Avro.java
##########
@@ -284,6 +286,11 @@ public DeleteWriteBuilder equalityFieldIds(int...
fieldIds) {
return this;
}
+ public DeleteWriteBuilder withSortOrder(SortOrder newSortOrder) {
Review comment:
We add an API for setting the sort order for equality deletes. Shall we
also add the ability to set an order for regular data files? We won't
immediately use it but it will make the code more consistent.
##########
File path:
core/src/main/java/org/apache/iceberg/deletes/EqualityDeleteWriter.java
##########
@@ -39,17 +40,25 @@
private final StructLike partition;
private final ByteBuffer keyMetadata;
private final int[] equalityFieldIds;
+ private final SortOrder sortOrder;
private DeleteFile deleteFile = null;
public EqualityDeleteWriter(FileAppender<T> appender, FileFormat format,
String location,
PartitionSpec spec, StructLike partition,
EncryptionKeyMetadata keyMetadata,
int... equalityFieldIds) {
+ this(appender, format, location, spec, partition, keyMetadata, null,
equalityFieldIds);
+ }
+
+ public EqualityDeleteWriter(FileAppender<T> appender, FileFormat format,
String location,
Review comment:
Do we want to deprecate the old constructor? Not a big deal but I am bit
worried about maintaining a new constructor each time we modify this class. At
the same time, converting this into a builder is probably an overkill. This is
a pretty internal API too so I'd be fine with breaking it.
Thoughts, @openinx @rdblue @yyanyy?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
