Review with Devin Part 3

deephaven · Mar 6, 2025 · 0e72eab · 0e72eab
1 parent b98123e
commit 0e72eab
Show file tree

Hide file tree

Showing 6 changed files with 302 additions and 91 deletions.
diff --git a/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergTableWriter.java b/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergTableWriter.java
@@ -299,8 +299,9 @@ private List<SortColumn> computeSortColumns(@NotNull final SortOrder sortOrder,
                 ascending = false;
             } else {
                 if (failOnUnmapped) {
-                    throw new IllegalArgumentException("Cannot apply sort order " + sortOrder + " since Deephaven" +
-                            " currently only supports sorting by {ASC, NULLS FIRST} or {DESC, NULLS LAST}");
+                    throw new UnsupportedOperationException(
+                            "Cannot apply sort order " + sortOrder + " since Deephaven currently only supports " +
+                                    "sorting by {ASC, NULLS FIRST} or {DESC, NULLS LAST}");
                 }
                 return List.of();
             }

diff --git a/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/SchemaProviderInternal.java b/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/SchemaProviderInternal.java
@@ -12,15 +12,15 @@
  */
 class SchemaProviderInternal {
 
-    interface SchemaProviderImpl {
+    interface SchemaProviderImpl extends SchemaProvider {
         /**
          * Returns the schema for the given table based on this {@link SchemaProvider}.
          */
         Schema getSchema(Table table);
     }
 
     // Implementations of SchemaProvider
-    enum CurrentSchemaProvider implements SchemaProvider, SchemaProviderImpl {
+    enum CurrentSchemaProvider implements SchemaProviderImpl {
         INSTANCE;
 
         @Override
@@ -29,7 +29,7 @@ public Schema getSchema(final Table table) {
         }
     }
 
-    static class IdSchemaProvider implements SchemaProvider, SchemaProviderImpl {
+    static class IdSchemaProvider implements SchemaProviderImpl {
         private final int schemaId;
 
         IdSchemaProvider(final int schemaId) {
@@ -42,7 +42,7 @@ public Schema getSchema(final Table table) {
         }
     }
 
-    static class DirectSchemaProvider implements SchemaProvider, SchemaProviderImpl {
+    static class DirectSchemaProvider implements SchemaProviderImpl {
         private final Schema schema;
 
         DirectSchemaProvider(final Schema schema) {
@@ -55,7 +55,7 @@ public Schema getSchema(final Table table) {
         }
     }
 
-    static class SnapshotIdSchemaProvider implements SchemaProvider, SchemaProviderImpl {
+    static class SnapshotIdSchemaProvider implements SchemaProviderImpl {
         private final int snapshotId;
 
         SnapshotIdSchemaProvider(final int snapshotId) {
@@ -68,7 +68,7 @@ public Schema getSchema(final Table table) {
         }
     }
 
-    enum CurrentSnapshotSchemaProvider implements SchemaProvider, SchemaProviderImpl {
+    enum CurrentSnapshotSchemaProvider implements SchemaProviderImpl {
         INSTANCE;
 
         @Override

diff --git a/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/SortOrderProvider.java b/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/SortOrderProvider.java
@@ -35,7 +35,16 @@ static SortOrderProvider fromSortId(final int id) {
     }
 
     /**
-     * Return a sort order provider that delegates to this provider for computing the columns to sort on, but writes a
+     * Use the given sort order directly to sort new data while writing to the iceberg table. Note that the provided
+     * sort order must either have a valid {@link SortOrder#orderId()}, else this provider should be chained with an
+     * {@link #as(int)} call to set a valid order ID.
+     */
+    static SortOrderProvider fromSortOrder(final SortOrder sortOrder) {
+        return new SortOrderProviderInternal.DirectSortOrderProvider(sortOrder);
+    }
+
+    /**
+     * Returns a sort order provider that delegates to this provider for computing the columns to sort on, but writes a
      * different sort order ID to the iceberg table. Note that the sort order returned by the caller must
      * {@link SortOrder#satisfies(SortOrder) satisfy} the sort order corresponding to the provided sort order ID.
      * <p>
@@ -44,9 +53,7 @@ static SortOrderProvider fromSortId(final int id) {
      *
      * @param sortOrderId the sort order ID to write to the iceberg table
      */
-    default SortOrderProvider as(final int sortOrderId) {
-        return new SortOrderProviderInternal.DelegatingSortOrderProvider(this, sortOrderId);
-    }
+    SortOrderProvider as(final int sortOrderId);
 
     /**
      * Returns a sort order provider which will fail, if for any reason, the sort order cannot be applied to the tables

diff --git a/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/SortOrderProviderInternal.java b/extensions/iceberg/src/main/java/io/deephaven/iceberg/util/SortOrderProviderInternal.java
@@ -12,12 +12,12 @@
  */
 class SortOrderProviderInternal {
 
-    interface SortOrderProviderImpl {
+    interface SortOrderProviderImpl extends SortOrderProvider {
         /**
          * Returns the {@link SortOrder} to use for sorting the data when writing to the provided iceberg table.
          */
         @NotNull
-        SortOrder getSortOrderToUse(Table table);
+        SortOrder getSortOrderToUse(@NotNull Table table);
 
         /**
          * Returns the sort order to write down to the iceberg table when appending new data. This may be different from
@@ -26,23 +26,28 @@ interface SortOrderProviderImpl {
          * order.
          */
         @NotNull
-        default SortOrder getSortOrderToWrite(final Table table) {
+        default SortOrder getSortOrderToWrite(@NotNull final Table table) {
             return getSortOrderToUse(table);
         }
 
         /**
          * Whether to fail if the sort order cannot be applied to the tables being written.
          */
         boolean failOnUnmapped();
+
+        @NotNull
+        default SortOrderProvider as(final int sortOrderId) {
+            return new SortOrderProviderInternal.DelegatingSortOrderProvider(this, sortOrderId);
+        }
     }
 
     // Implementations of SortOrderProvider
-    enum DisableSorting implements SortOrderProviderImpl, SortOrderProvider {
+    enum DisableSorting implements SortOrderProviderImpl {
         INSTANCE;
 
         @Override
         @NotNull
-        public SortOrder getSortOrderToUse(final Table table) {
+        public SortOrder getSortOrderToUse(@NotNull final Table table) {
             return SortOrder.unsorted();
         }
 
@@ -58,16 +63,14 @@ public boolean failOnUnmapped() {
         }
     }
 
-    static class TableDefaultSortOrderProvider implements SortOrderProvider, SortOrderProviderImpl {
+    static class TableDefaultSortOrderProvider implements SortOrderProviderImpl {
         private boolean failOnUnmapped;
 
-        TableDefaultSortOrderProvider() {
-            failOnUnmapped = false;
-        }
+        TableDefaultSortOrderProvider() {}
 
         @Override
         @NotNull
-        public SortOrder getSortOrderToUse(final Table table) {
+        public SortOrder getSortOrderToUse(@NotNull final Table table) {
             final SortOrder sortOrder = table.sortOrder();
             return sortOrder != null ? sortOrder : SortOrder.unsorted();
         }
@@ -84,18 +87,17 @@ public boolean failOnUnmapped() {
         }
     }
 
-    static class IdSortOrderProvider implements SortOrderProvider, SortOrderProviderImpl {
+    static class IdSortOrderProvider implements SortOrderProviderImpl {
         private boolean failOnUnmapped;
         private final int sortOrderId;
 
         IdSortOrderProvider(final int sortOrderId) {
-            super();
             this.sortOrderId = sortOrderId;
         }
 
         @Override
         @NotNull
-        public SortOrder getSortOrderToUse(final Table table) {
+        public SortOrder getSortOrderToUse(@NotNull final Table table) {
             return getSortOrderForId(table, sortOrderId);
         }
 
@@ -111,58 +113,95 @@ public boolean failOnUnmapped() {
         }
     }
 
+    static class DirectSortOrderProvider implements SortOrderProviderImpl {
+        private boolean failOnUnmapped;
+        private final SortOrder sortOrder;
+
+        DirectSortOrderProvider(@NotNull final SortOrder sortOrder) {
+            this.sortOrder = sortOrder;
+        }
+
+        @Override
+        @NotNull
+        public SortOrder getSortOrderToUse(@NotNull final Table table) {
+            return sortOrder;
+        }
+
+        @Override
+        @NotNull
+        public SortOrder getSortOrderToWrite(@NotNull final Table table) {
+            // Check if provided sort order is included in the table's sort orders
+            if (!sortOrder.equals(getSortOrderForId(table, sortOrder.orderId()))) {
+                throw new IllegalArgumentException("Provided sort order with id " + sortOrder.orderId() + " is not " +
+                        "included in the table's sort orders");
+            }
+            return sortOrder;
+        }
+
+        @Override
+        public SortOrderProvider withFailOnUnmapped(final boolean failOnUnmapped) {
+            this.failOnUnmapped = failOnUnmapped;
+            return this;
+        }
+
+        @Override
+        public boolean failOnUnmapped() {
+            return failOnUnmapped;
+        }
+    }
+
     /**
      * A {@link SortOrderProvider} that delegates to another {@link SortOrderProvider} for computing the sort order,
      * while providing a custom sort order ID.
      */
-    static class DelegatingSortOrderProvider implements SortOrderProvider, SortOrderProviderImpl {
-        private SortOrderProvider sortOrderProvider;
+    private static class DelegatingSortOrderProvider implements SortOrderProviderImpl {
+        private SortOrderProvider delegateProvider;
         private final int sortOrderId;
 
         DelegatingSortOrderProvider(final SortOrderProvider sortOrderProvider, final int sortOrderId) {
-            this.sortOrderProvider = sortOrderProvider;
+            this.delegateProvider = sortOrderProvider;
             this.sortOrderId = sortOrderId;
         }
 
         @Override
         @NotNull
-        public SortOrder getSortOrderToUse(final Table table) {
-            return ((SortOrderProviderImpl) sortOrderProvider).getSortOrderToUse(table);
+        public SortOrder getSortOrderToUse(final @NotNull Table table) {
+            return ((SortOrderProviderImpl) delegateProvider).getSortOrderToUse(table);
         }
 
         @Override
         @NotNull
-        public SortOrder getSortOrderToWrite(final Table table) {
+        public SortOrder getSortOrderToWrite(final @NotNull Table table) {
             final SortOrder sortOrderFromDelegate = getSortOrderToUse(table);
             final SortOrder sortOrderForId = getSortOrderForId(table, sortOrderId);
             if (!sortOrderFromDelegate.satisfies(sortOrderForId)) {
-                throw new IllegalArgumentException("Sort order with ID " + sortOrderId + " does not satisfy the " +
-                        "table's sort order: " + sortOrderFromDelegate);
+                throw new IllegalArgumentException(
+                        "Provided sort order " + sortOrderFromDelegate + " does not satisfy the table's sort order " +
+                                "with id " + sortOrderId + ": " + sortOrderForId);
             }
             return sortOrderForId;
         }
 
         @Override
         @NotNull
         public SortOrderProvider as(final int sortOrderId) {
-            return new DelegatingSortOrderProvider(sortOrderProvider, sortOrderId);
+            return new DelegatingSortOrderProvider(delegateProvider, sortOrderId);
         }
 
         @Override
         public SortOrderProvider withFailOnUnmapped(final boolean failOnUnmapped) {
-            sortOrderProvider = sortOrderProvider.withFailOnUnmapped(failOnUnmapped);
+            delegateProvider = delegateProvider.withFailOnUnmapped(failOnUnmapped);
             return this;
         }
 
         @Override
         public boolean failOnUnmapped() {
-            return ((SortOrderProviderImpl) sortOrderProvider).failOnUnmapped();
+            return ((SortOrderProviderImpl) delegateProvider).failOnUnmapped();
         }
     }
 
     // --------------------------------------------------------------------------------------------------
 
-    // Methods for extracting the sort order from the table
     private static SortOrder getSortOrderForId(final Table table, final int sortOrderId) {
         if (!table.sortOrders().containsKey(sortOrderId)) {
             throw new IllegalArgumentException("Sort order with ID " + sortOrderId + " not found for table " +