API: add StructTransform base class for PartitionKey and SortKey. add SortOrderComparators #7798

stevenzwu · 2023-06-07T22:41:40Z

see more context from dev ML thread: https://www.mail-archive.com/[email protected]/msg04424.html

stevenzwu · 2023-06-07T22:42:38Z

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


 /**
 * A struct of partition values.
 *
 * <p>Instances of this class can produce partition values from a data row passed to {@link
 * #partition(StructLike)}.
 */
-public class PartitionKey implements StructLike, Serializable {
+public class PartitionKey extends StructTransform {


A lot of code has been moved to the non-public StructTransform class so that it can be reused for the new SortKey class

stevenzwu · 2023-06-07T22:44:11Z

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

+  }
+
+  private static class SortOrderComparator implements Comparator<StructLike> {
+    private final SortKey leftKey;


included SortOrderComparator impl in this PR to demonstrate the usage of SortKey

pvary · 2023-09-13T11:13:37Z

@stevenzwu: Extracting the partition key from the row is on the hot path of writes. Based on the code I do not see any obvious issues, but it might worth to check the performance implications of this change?

WDYT?

stevenzwu · 2023-09-19T23:09:43Z

@stevenzwu: Extracting the partition key from the row is on the hot path of writes. Based on the code I do not see any obvious issues, but it might worth to check the performance implications of this change?

@pvary there is no change in the logic. simple refactor with a common base class extracted. so I don't expect performance implications

pvary · 2023-09-20T07:24:07Z

@stevenzwu: Extracting the partition key from the row is on the hot path of writes. Based on the code I do not see any obvious issues, but it might worth to check the performance implications of this change?

@pvary there is no change in the logic. simple refactor with a common base class extracted. so I don't expect performance implications

Previous code had 1 less redirection/inheritance. Optimized loops can be a thing in this type of code paths, where the compiler could deduce that the different iterations of the loops are independent, and could be executed in batch. Adding more redirection/inheritance could break the optimization algorithm and instead of a parallel loop we can end up executing the loop sequentially. This could cause performance degradation. This is the only effect which might effect other users of the classes touched by this change, otherwise I think the change keeps the old behaviour correctly and introduces a new one, which we need.

RussellSpitzer · 2023-10-23T16:00:24Z

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

+import java.util.stream.Collectors;
+
+/**
+ * A struct of flattened sort field values.


not sure I understand flattened in this context

let's assume the table schema is like

Schema schema = new Schema( Types.NestedField.required(1, "id", Types.StringType.get()), Types.NestedField.optional( 2, "location", Types.StructType.of( Types.NestedField.required(11, "lat", Types.FloatType.get()), Types.NestedField.required(12, "long", Types.FloatType.get()))));

if the SortKey is applied on id and lat, the StructTransform only produces a flattened tuple. nested structure is not maintained anymore.

private final Object[] transformedTuple;

RussellSpitzer · 2023-10-23T16:02:20Z

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

+/**
+ * A struct of flattened transformed values.
+ *
+ * <p>Instances of this class can produce sort values from a data row passed to {@link


Typo, can produce "transformed values"?

… SortOrderComparators

RussellSpitzer · 2023-10-24T15:47:00Z

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

+    this.sortOrder = toCopy.sortOrder;
+  }
+
+  public SortKey copy() {


This is the old method but we aren't actually copying here. Schema and Sort Order are not copies, just noting.

You are right it is not deep copy of everything. but it is a still copy/clone implementation of the SortKey that we deemed correct.

RussellSpitzer · 2023-10-24T15:58:50Z

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

+      Transform<?, ?> transform = fieldTransforms.get(i).transform();
+      Accessor<StructLike> accessor = schema.accessorForField(sourceFieldId);
+      Preconditions.checkArgument(
+          accessor != null, "Cannot build accessor for field: " + schema.findField(sourceFieldId));


minor nit: checkArgs has a "condition, string, args" version so you don't have to do a string concat

RussellSpitzer · 2023-10-24T16:02:32Z

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

+
+  /**
+   * Simple POJO for source field id and transform function. {@code Pair} class is not usable here
+   * in API module, as it has Avro dep and lives in core module.


as it has a Avro dep and is in the core module.

thx. will update to as it has an Avro dep and is in the core module

RussellSpitzer

Looks good to me

… SortOrderComparators (apache#7798)

stevenzwu requested review from rdblue, RussellSpitzer, aokolnychyi and pvary June 7, 2023 22:41

github-actions bot added the API label Jun 7, 2023

stevenzwu commented Jun 7, 2023

View reviewed changes

stevenzwu force-pushed the sort-key-comparator branch 2 times, most recently from 3970b55 to f93b012 Compare June 10, 2023 19:01

stevenzwu force-pushed the sort-key-comparator branch from f93b012 to 55123d4 Compare June 23, 2023 17:13

stevenzwu force-pushed the sort-key-comparator branch from 55123d4 to c824402 Compare August 15, 2023 05:34

stevenzwu force-pushed the sort-key-comparator branch from c824402 to 78606fc Compare September 30, 2023 20:01

RussellSpitzer reviewed Oct 23, 2023

View reviewed changes

stevenzwu added 2 commits October 23, 2023 09:31

API: add StructTransform base class for PartitionKey and SortKey. add…

61b630c

… SortOrderComparators

fix typo in comment as Russle pointed out

b671d27

stevenzwu force-pushed the sort-key-comparator branch from 78606fc to b671d27 Compare October 23, 2023 16:33

RussellSpitzer reviewed Oct 24, 2023

View reviewed changes

RussellSpitzer approved these changes Oct 24, 2023

View reviewed changes

address Russel's comment of error msg and javadoc

a755c67

stevenzwu merged commit b21a8ce into apache:main Nov 28, 2023
46 checks passed

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024

API: add StructTransform base class for PartitionKey and SortKey. add…

2456c6e

… SortOrderComparators (apache#7798)

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

API: add StructTransform base class for PartitionKey and SortKey. add…

49186f6

… SortOrderComparators (apache#7798)

stevenzwu mentioned this pull request Jun 19, 2024

API: add resultSchema() method to StructTransform #10496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: add StructTransform base class for PartitionKey and SortKey. add SortOrderComparators #7798

API: add StructTransform base class for PartitionKey and SortKey. add SortOrderComparators #7798

stevenzwu commented Jun 7, 2023 •

edited

Loading

stevenzwu Jun 7, 2023

stevenzwu Jun 7, 2023 •

edited

Loading

pvary commented Sep 13, 2023

stevenzwu commented Sep 19, 2023

pvary commented Sep 20, 2023

RussellSpitzer Oct 23, 2023

stevenzwu Oct 23, 2023 •

edited

Loading

RussellSpitzer Oct 23, 2023

RussellSpitzer Oct 24, 2023

stevenzwu Oct 24, 2023

RussellSpitzer Oct 24, 2023

RussellSpitzer Oct 24, 2023

stevenzwu Oct 24, 2023

RussellSpitzer left a comment

API: add StructTransform base class for PartitionKey and SortKey. add SortOrderComparators #7798

API: add StructTransform base class for PartitionKey and SortKey. add SortOrderComparators #7798

Conversation

stevenzwu commented Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

stevenzwu Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

pvary commented Sep 13, 2023

stevenzwu commented Sep 19, 2023

pvary commented Sep 20, 2023

Choose a reason for hiding this comment

stevenzwu Oct 23, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

stevenzwu commented Jun 7, 2023 •

edited

Loading

stevenzwu Jun 7, 2023 •

edited

Loading

stevenzwu Oct 23, 2023 •

edited

Loading