dgvj-work commented on code in PR #1707:
URL: https://github.com/apache/iceberg-go/pull/1707#discussion_r3759814989
##########
cmd/iceberg/main.go:
##########
@@ -36,11 +36,13 @@ import (
"github.com/apache/iceberg-go/catalog/hadoop"
"github.com/apache/iceberg-go/catalog/hive"
"github.com/apache/iceberg-go/catalog/rest"
+ sqlcat "github.com/apache/iceberg-go/catalog/sql"
"github.com/apache/iceberg-go/config"
_ "github.com/apache/iceberg-go/io/gocloud"
"github.com/apache/iceberg-go/table"
awsconfig "github.com/aws/aws-sdk-go-v2/config"
+ _ "github.com/uptrace/bun/driver/sqliteshim"
Review Comment:
Went with the narrower docs path: the default binary still only
blank-imports `sqliteshim`, and the `--sql-driver` / `--sql-dialect` help plus
`cli.md` / `configuration.md` now say that explicitly. Other dialects remain
supported by the SQL catalog library, but need a custom build that
blank-imports their drivers.
##########
cmd/iceberg/main.go:
##########
@@ -422,6 +426,26 @@ func initCatalog(ctx context.Context, args Args)
catalog.Catalog {
}); err != nil {
log.Fatal(err)
}
+ case catalog.SQL:
+ // Always set uri/warehouse keys (even when empty) so
catalog.Load does not
+ // fill them from EnvConfig for a differently typed catalog of
the same name.
+ props := iceberg.Properties{
+ "type": string(catalog.SQL),
+ "uri": args.URI,
Review Comment:
`mergeConf` already copied `uri` / `warehouse` / `credential` from the file
when those flags were omitted; I tightened the tests around that path and also
always pass `credential` into the SQL props (same empty-key guard as `uri` /
`warehouse`) so a differently typed EnvConfig entry can't bleed through.
##########
cmd/iceberg/main.go:
##########
@@ -422,6 +426,26 @@ func initCatalog(ctx context.Context, args Args)
catalog.Catalog {
}); err != nil {
log.Fatal(err)
}
+ case catalog.SQL:
+ // Always set uri/warehouse keys (even when empty) so
catalog.Load does not
+ // fill them from EnvConfig for a differently typed catalog of
the same name.
+ props := iceberg.Properties{
+ "type": string(catalog.SQL),
+ "uri": args.URI,
+ "warehouse": args.Warehouse,
+ }
+ if len(args.SQLDriver) > 0 {
+ props[sqlcat.DriverKey] = args.SQLDriver
+ }
+ if len(args.SQLDialect) > 0 {
+ props[sqlcat.DialectKey] = args.SQLDialect
+ }
+
+ if cat, err = catalog.Load(ctx, args.CatalogName, props); err
!= nil {
+ log.Fatal(err)
+ }
+ case catalog.DynamoDB:
+ log.Fatal("dynamodb catalog is not implemented")
Review Comment:
Moved the DynamoDB rejection into `main` before `initCatalog`, so it goes
through `output.Error` + `os.Exit(1)` like the other user-facing checks.
##########
cmd/iceberg/main.go:
##########
@@ -422,6 +426,26 @@ func initCatalog(ctx context.Context, args Args)
catalog.Catalog {
}); err != nil {
log.Fatal(err)
}
+ case catalog.SQL:
Review Comment:
Left the missing-driver check to the registrar on purpose so the failure
stays `must provide driver` from `catalog.Load` — that also lets the new
negative test prove the SQL branch is actually wired through. Happy to add a
CLI-level preflight later if you'd rather fail earlier with a flag-oriented
message.
##########
cmd/iceberg/main.go:
##########
@@ -200,7 +202,7 @@ type Args struct {
Upgrade *UpgradeCmd `arg:"subcommand:upgrade"
help:"upgrade table format version"`
Rollback *RollbackCmd `arg:"subcommand:rollback"
help:"roll back to a previous snapshot"`
- Catalog string `arg:"--catalog" default:"rest" help:"catalog type"`
+ Catalog string `arg:"--catalog" default:"rest" help:"catalog type
(rest, glue, hive, hadoop, sql)"`
Review Comment:
Added `dynamodb` to the `--catalog` help with the not-implemented caveat so
it matches `cli.md`.
##########
cmd/iceberg/args_test.go:
##########
@@ -495,6 +503,48 @@ func TestCLIAcceptsMixedCaseCatalogType(t *testing.T) {
args: []string{"list", "--catalog", "Hive", "--uri",
hiveURI},
wantErr: true,
},
+ {
+ name: "sql lowercase",
Review Comment:
Added a `wantErr` case for `--catalog sql` without `--sql-driver` that
asserts `must provide driver` and still rejects `unrecognized catalog type`.
--
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]
