rdblue commented on code in PR #6965:
URL: https://github.com/apache/iceberg/pull/6965#discussion_r1126893023


##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java:
##########
@@ -1017,4 +1019,41 @@ public String unknown(
       return String.format("%s(%s) %s %s", transform, sourceName, direction, 
nullOrder);
     }
   }
+
+  public static Map<String, String> buildWriteOptions(String branch, 
LogicalWriteInfo info) {
+    ImmutableMap.Builder<String, String> builder =
+        ImmutableMap.<String, String>builder().putAll(info.options());
+
+    if (branch != null) {
+      String optionBranch = info.options().get(SparkReadOptions.BRANCH);
+      if (optionBranch == null) {
+        builder.put(SparkWriteOptions.BRANCH, branch);
+      } else if (!optionBranch.equals(branch)) {
+        throw new ValidationException(
+            "Cannot override branch to write more than once, received %s in 
identifier and %s in write options",
+            branch, optionBranch);
+      }
+    }
+
+    return builder.build();
+  }
+
+  public static CaseInsensitiveStringMap buildScanOptions(
+      String branch, CaseInsensitiveStringMap options) {
+    ImmutableMap.Builder<String, String> builder =
+        ImmutableMap.<String, String>builder().putAll(options);
+
+    if (branch != null) {
+      String optionBranch = options.get(SparkReadOptions.BRANCH);
+      if (optionBranch == null) {
+        builder.put(SparkReadOptions.BRANCH, branch);
+      } else if (!optionBranch.equals(branch)) {
+        throw new ValidationException(
+            "Cannot override branch to read more than once, received %s in 
identifier and %s in scan options",

Review Comment:
   I think it would be cleaner to just pass the branch directly into 
`SparkReadConf` and detect when to use it, rather than creating a copy of 
`CaseInsensitiveStringMap`.
   
   Since we've already shipped with `branch` and `tag` options, we should leave 
them in. Spark considers those part of the table definition, so I think that it 
is safe to continue to allow them. But because of the issues with needing to 
produce scans based on write options, we should avoid adding places that depend 
on them.



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to