laskoviymishka commented on code in PR #1707:
URL: https://github.com/apache/iceberg-go/pull/1707#discussion_r3749983403
##########
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:
There's no pre-flight check that `--sql-driver`/`--sql-dialect` are set when
`--catalog sql`; a missing driver surfaces as a log.Fatal from deep inside the
registrar. We validate other flag combos up front (the rollback selector), so
I'd add an early check here that catches both missing flags at once and keeps
the message consistent. Not a blocker.
##########
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:
Every other user-facing error here goes through output.Error + os.Exit(1) so
it respects `--output json`; this one log.Fatals straight to stderr.
initCatalog doesn't take the output writer today so it may be more than a
one-liner, but I'd at least route it through the same path as the other
graceful errors, or leave a note that this bypasses the JSON layer.
##########
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:
These cases prove the happy path (list against an empty sqlite exits 0),
nice. Could we also add a `wantErr` case for `--catalog sql` with no
`--sql-driver`, asserting it reaches the registrar's `must provide driver`
rather than being rejected as an unrecognized type? That's the bit that proves
the new branch is actually wired through catalog.Load.
##########
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:
Setting `uri`/`warehouse` unconditionally fixes the bleed for the
flag-driven path, but it also breaks the config-file-only workflow: if someone
puts `uri` under this catalog name in `~/.iceberg-go.yaml` and omits `--uri` to
inherit it, they now get an empty DSN instead.
The cleaner fix is probably to have mergeConf populate
args.URI/args.Warehouse from the config file before initCatalog runs, so the
value reaching here is never empty when the file has it, then this guard isn't
load-bearing. Same reasoning applies to `credential`, which isn't in the map at
all and can still get pulled from EnvConfig. wdyt?
##########
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:
Small one: this lists rest, glue, hive, hadoop, sql but not dynamodb, even
though it's recognized now and cli.md mentions it. I'd add it here with the
not-implemented caveat so `--help` and the docs agree.
##########
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:
This blank-imports sqliteshim unconditionally but no other driver, so a
prebuilt binary run with `--sql-driver pgx --sql-dialect postgres` dies with
`sql: unknown driver` and no hint. The `--sql-dialect` help plus cli.md both
advertise postgres/mysql/mssql/oracle as if they work.
Since #1692 is really about the production SQL catalogs, I'd either
blank-import the common drivers (lib/pq or pgx, go-sql-driver/mysql) so the CLI
works out of the box, or narrow the help text and docs to say only sqliteshim
is compiled in and anything else needs a custom build. Either is fine, but the
advertised-vs-available gap needs to close one way or the other. wdyt?
--
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]
