C-Loftus commented on code in PR #1280:
URL: https://github.com/apache/iceberg-go/pull/1280#discussion_r3492004118
##########
catalog/hadoop/hadoop.go:
##########
@@ -137,33 +139,45 @@ func NewCatalog(name, warehouse string, props
iceberg.Properties) (*Catalog, err
return nil, fmt.Errorf("hadoop catalog: when using warehouse
scheme %q, `allow-unsafe-commits` must be set to true", u.Scheme)
}
- if u.Opaque != "" {
- warehouse = u.Opaque
- } else {
- warehouse = u.Path
- }
+ if isLocal {
+ if u.Opaque != "" {
+ warehouse = u.Opaque
+ } else {
+ warehouse = u.Path
+ }
- if warehouse == "" || warehouse == "/" {
- return nil, errors.New("hadoop catalog requires a non-root
warehouse path")
- }
+ if warehouse == "" || warehouse == "/" {
+ return nil, errors.New("hadoop catalog: local
filesystem requires a non-root warehouse path")
+ }
- warehouse = strings.TrimRight(warehouse, "/")
+ warehouse = strings.TrimRight(warehouse, "/")
- // Normalize to absolute path so the synthetic "location" property
- // always produces a valid file:// URI.
- absWarehouse, err := filepath.Abs(warehouse)
+ // Normalize to absolute path so the synthetic "location"
property
+ // always produces a valid file:// URI.
+ absWarehouse, err := filepath.Abs(warehouse)
+ if err != nil {
+ return nil, fmt.Errorf("hadoop catalog: failed to
resolve absolute warehouse path: %w", err)
+ }
+
+ warehouse = absWarehouse
+ }
+ // TODO: propagate caller context once NewCatalog accepts one
Review Comment:
Added the requested TODO comment
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -75,6 +76,25 @@ func (f *barrierRenameNoReplaceFS) RenameNoReplace(oldpath,
newpath string) erro
return f.LocalFS.RenameNoReplace(oldpath, newpath)
}
+// a mock hadoop catalog filesystem to ensure that we
+// can load arbitrary new filesystem implementations
+// as long as they full the HadoopCatalogFS interface
+type stubHadoopCatalogFS struct {
+ icebergio.LocalFS
+}
Review Comment:
Requested stub helper for testing against another mock implementation
##########
catalog/hadoop/hadoop.go:
##########
@@ -172,24 +186,50 @@ func (c *Catalog) CatalogType() catalog.Type {
return catalog.Hadoop
}
+// joinPath is a helper that allows paths to be joined as both local filesystem
+// paths or as remote URIs needed for filesystems like blob stores.
+func joinPath(isLocal bool, base string, parts ...string) string {
Review Comment:
Condensed all joinPath logic into this function here. This is private to the
package and tested.
--
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]
