Repository navigation
Conversation
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
With AddDataFilesByPath mapped back to AddFiles, the only differences from the base are the new godoc and a line wrap in transaction_internal_test.go.
| } | ||
|
|
||
| func (t *Transaction) AddFiles(ctx context.Context, filePaths []string, snapshotProps iceberg.Properties, ignoreDuplicates bool, opts ...AddFilesOption) error { | ||
| // AddDataFilesByPath adds data files at the provided paths to the table. |
There was a problem hiding this comment.
Should this godoc also say how it differs from AddDataFiles? Today only AddDataFiles' own godoc spells it out ("this method does not read files from storage").
| // AddDataFilesByPath adds data files at the provided paths to the table. | |
| // AddDataFilesByPath adds data files at the provided paths to the table. | |
| // Unlike AddDataFiles, it opens each file to read its schema and statistics. |
|
|
||
| func (t *Transaction) AddFiles(ctx context.Context, filePaths []string, snapshotProps iceberg.Properties, ignoreDuplicates bool, opts ...AddFilesOption) error { | ||
| // AddDataFilesByPath adds data files at the provided paths to the table. | ||
| func (t *Transaction) AddDataFilesByPath(ctx context.Context, filePaths []string, snapshotProps iceberg.Properties, ignoreDuplicates bool, opts ...AddFilesOption) error { |
There was a problem hiding this comment.
Could we rename AddFilesOption and WithAddFilesConcurrency along with the method, e.g. to AddDataFilesByPathOption / WithAddDataFilesByPathConcurrency (like OverwriteOption / WithOverwriteConcurrency)? git grep finds both only in this file. Renaming them after v1 would mean keeping the old names as aliases.
|
@fallintoplace thx! :D i failed to fix it via GH ui |
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
d93225a to
481dd3f
Compare
…61023 Resolve the collectGeoColumns doc conflict with apache#2303, rename the AddFiles call added by apache#2288's test, and restore the compatibility note from 3b5bc47 dropped by the release-notes sync.
The note for this change lands in a follow-up release-notes PR so the pre-v1 renames can merge in parallel.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Rename is complete, and the godoc now says how AddDataFilesByPath differs from AddDataFiles, per @laskoviymishka's suggestions. I pushed a merge of main (345948a); no AddFiles/AddFilesOption/WithAddFilesConcurrency references remain, and go vet ./... (including -tags integration) and go test ./table/... pass. The compatibility note for this change (dropped by the "sync release notes with main" commit) will land in a single follow-up release-notes PR covering this batch of pre-v1 renames, so these can merge in parallel without releases.md conflicts. LGTM.
Resolve the test overlaps with the renames already on main (apache#2308, apache#2314, apache#2317, apache#2319): each conflict hunk keeps this PR's sequence-number names with main's other renames applied.
…g changes Add release notes for the rename batch (apache#2308, apache#2314, apache#2315, apache#2316, apache#2317, apache#2319, apache#2322, apache#2323) and for apache#2067, apache#2290, apache#2294, apache#2307, apache#2310 and apache#2313. Remove stray blank lines inside the compatibility-notes list.
What
Rename
Transaction.AddFilestoAddDataFilesByPath.Why
Make the path-based API explicit next to
AddDataFilesbefore the v1 freeze.Implementation
Breaking API: callers must use
AddDataFilesByPath.Related: #2062