zeroshade commented on code in PR #774:
URL: https://github.com/apache/iceberg-go/pull/774#discussion_r2943135149
##########
table/transaction.go:
##########
@@ -820,6 +865,13 @@ func WithOverwriteCaseInsensitive() OverwriteOption {
}
}
+// WithOverwriteBranch sets the target branch for the overwrite. Default is
main.
+func WithOverwriteBranch(branch string) OverwriteOption {
+ return func(op *overwriteOperation) {
+ op.branch = branch
+ }
+}
Review Comment:
why do we need separate types for `OverwriteOption` and `WriteOption`? Can't
we reuse the same option type?
##########
table/transaction.go:
##########
@@ -687,7 +730,7 @@ func (t *Transaction) ReplaceDataFilesWithDataFiles(ctx
context.Context, filesTo
return t.apply(updates, reqs)
}
-func (t *Transaction) AddFiles(ctx context.Context, files []string,
snapshotProps iceberg.Properties, ignoreDuplicates bool) error {
+func (t *Transaction) AddFiles(ctx context.Context, files []string,
snapshotProps iceberg.Properties, ignoreDuplicates bool, opts ...WriteOpt)
error {
Review Comment:
should more of these parameters get added as Options instead if we're going
to go down this road? We should make all non-required parameters into options
then
##########
table/transaction.go:
##########
@@ -40,28 +40,61 @@ import (
"golang.org/x/sync/errgroup"
)
+// WriteOpt is an option for write operations (Append, AppendTable, AddFiles,
etc.).
+type WriteOpt interface {
+ applyWriteOpt(*writeOpts)
+}
+
+type writeOpts struct {
+ branch string
+}
+
+func (o *writeOpts) applyWriteOpt(target *writeOpts) {
+ if o.branch != "" {
+ target.branch = o.branch
+ }
+}
+
+// WithBranch sets the target branch for the write. Default is main.
+func WithBranch(branch string) WriteOpt {
+ return &writeOpts{branch: branch}
+}
+
+func resolveBranch(opts []WriteOpt) string {
+ var o writeOpts
+ for _, opt := range opts {
+ opt.applyWriteOpt(&o)
+ }
+ if o.branch == "" {
+ return MainBranch
+ }
+
+ return o.branch
+}
Review Comment:
instead of resolveBranch, we should have a common `config` struct and just
apply all the options to it and then we grab the values by the properties
directly. That would make this more reusable in general
--
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]
