Core: add sort order id to content file #1975

yyanyy · 2020-12-23T02:45:19Z

This change adds sort_order_id to content file, and allow read and write of this attribute in manifest entry. The logic of populating sort order id in writers from table attribute is not included here and will be the main focus for the next PR, as doing so will likely requires a lot of signature changes.

Questions

Not sure if sort order should be nullable by default or 0 (fromunsorted_order): decided as nullable for now, as I think positional delete files shouldn't have sort order (since they should be sorted by file_path and position per spec)
Do we want only sort order id, or actual sort order struct? Id itself is good for comparison but when merging data/delete files I think we do need the actual sort order struct. Without including it in manifest entries, I think we may need to do an additional lookup of the table to fetch it, which could slow down the query.
For the next PR, do we assume the table's current sort order id is the authoritative place to get sort order information when adding a new file? I wonder if engine is ever possible/allowed to override the default sort order to unsorted and pass it back to data/delete writer.

rdblue · 2020-12-28T19:00:12Z

Not sure if sort order should be nullable by default or 0 (from unsorted_order)

The field should be optional because v1 manifests will not have the order field. Iceberg will read the value as null, so I think it makes sense to use null. And you're right about not storing it for position deletes.

Do we want only sort order id, or actual sort order struct?

We want the ID. Sort orders are attached to table metadata, so loading the order should be a simple hash map lookup.

For the next PR, do we assume the table's current sort order id is the authoritative place to get sort order information when adding a new file?

No. Engines must specify which sort order was used to write a file explicitly. So this needs to be exposed in the DataFile and DeleteFile builders. By default, we should write either null or 0 (unordered). Probably null.

rdblue · 2020-12-28T19:03:02Z

core/src/main/java/org/apache/iceberg/DataFiles.java

@@ -268,6 +271,11 @@ public Builder withEncryptionKeyMetadata(EncryptionKeyMetadata newKeyMetadata) {
      return withEncryptionKeyMetadata(newKeyMetadata.buffer());
    }

+    public Builder withSortOrderId(int newSortOrderId) {


What about withSortOrder? Is there a case where we would only have the ID and not the SortOrder? Just trying to think of what will be convenient. We could always expose both methods.

I think if we change this to withSortOrder and store sortOrder directly, copy() of this Builder might have problem since DataFile only has sortOrderId() but no sortOrder() so we couldn't copy directly; we probably also couldn't add sortOrder() to ContentFile/DataFile since the file created from Avro reflection will not have the struct without looking up table metadata?

But if withSortOrder essentially only extracts the id and does the same thing as withSortOrderId here, then it will definitely be more convenient. I'll update to do that but please let me know if I didn't understand correctly!

rdblue · 2020-12-28T19:07:19Z

api/src/main/java/org/apache/iceberg/DataFile.java


  int PARTITION_ID = 102;
  String PARTITION_NAME = "partition";
  String PARTITION_DOC = "Partition data tuple, schema based on the partition spec";
-  // NEXT ID TO ASSIGN: 140
+  // NEXT ID TO ASSIGN: 141


We will also need to add this to the spec.

rdblue · 2020-12-28T19:08:15Z

api/src/main/java/org/apache/iceberg/ContentFile.java

+   * when they share the same sort order id.
+   */
+  default Integer sortOrderId() {
+    return null;


Could you add this after equalityFieldIds instead of here? The copy methods should be last because they aren't fields in the file.

rdblue · 2020-12-28T19:08:33Z

api/src/main/java/org/apache/iceberg/ContentFile.java

+   * This information will be useful for merging data and equality delete files more efficiently
+   * when they share the same sort order id.
+   */
+  default Integer sortOrderId() {


Do we need a default?

I was thinking to add it to ensure backward compatibility in case people implement their own ContentFile, do you think it's unlikely so it's safe for us to not have default?

yyanyy · 2021-01-06T03:27:12Z

Not sure if sort order should be nullable by default or 0 (from unsorted_order)

The field should be optional because v1 manifests will not have the order field. Iceberg will read the value as null, so I think it makes sense to use null. And you're right about not storing it for position deletes.

Do we want only sort order id, or actual sort order struct?

We want the ID. Sort orders are attached to table metadata, so loading the order should be a simple hash map lookup.

For the next PR, do we assume the table's current sort order id is the authoritative place to get sort order information when adding a new file?

No. Engines must specify which sort order was used to write a file explicitly. So this needs to be exposed in the DataFile and DeleteFile builders. By default, we should write either null or 0 (unordered). Probably null.

Thank you for the response!

We want the ID. Sort orders are attached to table metadata, so loading the order should be a simple hash map lookup.

I guess in order to do that, we may need to add the file's sort order map in FileScanTask, as it seems like in readers (e.g. RowDataReader) we rely on it for reading rows, meanwhile we don't have the table available for metadata lookup?

Engines must specify which sort order was used to write a file explicitly.

(Sorry for the naive question) I guess the sort order needs to be decided when building the writer (e.g. add a sortOrder parameter in SparkWriter writer factory), and how does the engine know which sort order to use when writing files? Maybe the sort order could be an optional thing to specify when a job is created (e.g. as part of the the sql command for ingesting data) , and thus the engine will already know the sort order to use when it creates the writer, although some validations might need to be done against table metadata before that (e.g. check for such sort order exists, create one or abort if not); and if nothing is specified for this job/command, the engine will look for table's default sort order, and use it for creating the writer?

jackye1995

looks good to me, the only question I had was about returning null or 0, but Ryan's comment answered my question.

jackye1995 · 2021-01-14T00:26:25Z

api/src/main/java/org/apache/iceberg/ContentFile.java

@@ -124,6 +124,14 @@
   */
  List<Integer> equalityFieldIds();

+  /**
+   * Returns the sort order id of this file, which describes how the file is ordered.


nit: <p> after the line

rdblue · 2021-02-03T01:36:13Z

api/src/main/java/org/apache/iceberg/ContentFile.java

@@ -141,4 +149,5 @@
   *         null value counts, or nan value counts
   */
  F copyWithoutStats();
+


Nit: Can you remove this whitespace change? It isn't needed and could cause conflicts.

Nevermind, this is the only issue and it isn't worth blocking.

core/src/main/java/org/apache/iceberg/BaseFile.java

rdblue · 2021-02-03T01:41:25Z

core/src/main/java/org/apache/iceberg/DataFiles.java

@@ -268,6 +271,13 @@ public Builder withEncryptionKeyMetadata(EncryptionKeyMetadata newKeyMetadata) {
      return withEncryptionKeyMetadata(newKeyMetadata.buffer());
    }

+    public Builder withSortOrder(SortOrder newSortOrder) {


Looks good.

rdblue · 2021-02-03T01:47:07Z

Looks great, thanks @yyanyy!

Core: add sort order id to content file interface

d8cf852

github-actions bot added API core spark labels Dec 23, 2020

rdblue reviewed Dec 28, 2020

View reviewed changes

address comments

cda3d99

jackye1995 approved these changes Jan 14, 2021

View reviewed changes

rdblue mentioned this pull request Jan 19, 2021

Flink: Support write.distribution-mode. #2064

Merged

rdblue reviewed Feb 3, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseFile.java Show resolved Hide resolved

rdblue reviewed Feb 3, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseFile.java Show resolved Hide resolved

rdblue reviewed Feb 3, 2021

View reviewed changes

rdblue approved these changes Feb 3, 2021

View reviewed changes

rdblue merged commit 9bd18f4 into apache:master Feb 3, 2021

yyanyy mentioned this pull request Feb 4, 2021

Add sort order to writer classes #2214

Merged

coolderli pushed a commit to coolderli/iceberg that referenced this pull request Apr 26, 2021

Core: Add sort order id to DataFile and DeleteFile (apache#1975)

7543a18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: add sort order id to content file #1975

Core: add sort order id to content file #1975

yyanyy commented Dec 23, 2020

rdblue commented Dec 28, 2020

rdblue Dec 28, 2020

yyanyy Jan 6, 2021 •

edited

Loading

rdblue Dec 28, 2020

rdblue Dec 28, 2020

rdblue Dec 28, 2020

yyanyy Jan 6, 2021

yyanyy commented Jan 6, 2021 •

edited

Loading

jackye1995 left a comment

jackye1995 Jan 14, 2021

rdblue Feb 3, 2021

rdblue Feb 3, 2021

rdblue Feb 3, 2021

rdblue commented Feb 3, 2021

Core: add sort order id to content file #1975

Core: add sort order id to content file #1975

Conversation

yyanyy commented Dec 23, 2020

rdblue commented Dec 28, 2020

Choose a reason for hiding this comment

yyanyy Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyanyy commented Jan 6, 2021 • edited Loading

jackye1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Feb 3, 2021

yyanyy Jan 6, 2021 •

edited

Loading

yyanyy commented Jan 6, 2021 •

edited

Loading