andr-sokolov commented on code in PR #1842:
URL: https://github.com/apache/cloudberry/pull/1842#discussion_r3557092247
##########
src/bin/psql/tab-complete.c:
##########
@@ -3819,7 +3854,15 @@ psql_completion(const char *text, int start, int end)
else if (Matches("DROP", "AGGREGATE|FUNCTION|PROCEDURE|ROUTINE",
MatchAny, "("))
COMPLETE_WITH_FUNCTION_ARG(prev2_wd);
else if (Matches("DROP", "FOREIGN"))
- COMPLETE_WITH("DATA WRAPPER", "TABLE");
+ COMPLETE_WITH("CATALOG", "DATA WRAPPER", "TABLE", "VOLUME");
+ else if (Matches("DROP", "FOREIGN", "CATALOG"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_foreign_catalogs);
+ else if (Matches("DROP", "FOREIGN", "VOLUME"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_foreign_volumes);
+ else if (Matches("DROP", "ICEBERG"))
+ COMPLETE_WITH("TABLE");
+ else if (Matches("DROP", "ICEBERG", "TABLE"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
Review Comment:
Is it possible to write a query to get here iceberg tables only?
##########
src/include/commands/laketablecmds.h:
##########
@@ -0,0 +1,61 @@
+/*-------------------------------------------------------------------------
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ * laketablecmds.h
+ * prototypes for laketablecmds.c.
+ *
+ * src/include/commands/laketablecmds.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef LAKETABLECMDS_H
+#define LAKETABLECMDS_H
+
+#include "catalog/pg_lake_table.h"
+#include "nodes/parsenodes.h"
+#include "utils/guc.h"
+#include "utils/rel.h"
+
+/*
+ * Name of the table access method lake tables are created with. The
+ * kernel only provides the DDL scaffolding; the access method itself is
+ * provided by a datalake extension.
+ */
+#define ICEBERG_TABLE_AM_NAME "iceberg"
+
+/* GUC variables */
+extern char *iceberg_default_catalog;
+extern char *iceberg_default_volume;
+
+/* GUC check hooks */
+extern bool check_iceberg_default_catalog(char **newval, void **extra,
GucSource source);
+extern bool check_iceberg_default_volume(char **newval, void **extra,
GucSource source);
+
+/* Functions to get default values */
+extern const char *GetDefaultIcebergCatalog(void);
+extern const char *GetDefaultIcebergVolume(void);
+
+/* Lake table management */
+extern Oid GetIcebergTableAmOid(bool missing_ok);
+extern bool RelationIsIcebergTable(Relation rel);
+extern void ValidateLakeTableOptions(CreateLakeTableStmt *stmt);
Review Comment:
I think the `Options` word in the function name is not correct because
`CreateLakeTableStmt` contains the `options` field and reading the function
name I get a false expectation that the function validates the `options` field
only. I suggest to rename the function. For example, `ValidateLakeTableStmt`,
because it validates all fields of the structure
```suggestion
extern void ValidateLakeTableStmt(CreateLakeTableStmt *stmt);
```
##########
src/test/regress/sql/lake_table.sql:
##########
@@ -0,0 +1,150 @@
+--
+-- Test lake table DDL: FOREIGN CATALOG, FOREIGN VOLUME, ICEBERG TABLE
+--
+
+-- Display the lake table catalogs
+\d+ pg_foreign_catalog
+\d+ pg_foreign_volume
+\d+ pg_lake_table
+
+-- Setup: foreign servers for the catalogs and volumes to hang off
+CREATE FOREIGN DATA WRAPPER lake_test_fdw;
+CREATE SERVER lake_test_srv FOREIGN DATA WRAPPER lake_test_fdw;
+CREATE SERVER lake_test_srv2 FOREIGN DATA WRAPPER lake_test_fdw;
+
+-- CREATE FOREIGN CATALOG: TYPE is a required first-class property
+CREATE FOREIGN CATALOG lake_test_cat SERVER lake_test_srv TYPE 'hive' OPTIONS
(uri 'thrift://localhost:9083');
+CREATE FOREIGN CATALOG lake_test_notype SERVER lake_test_srv; --
fail, TYPE is required
+CREATE FOREIGN CATALOG lake_test_cat SERVER lake_test_srv TYPE 'hive';
-- fail, duplicate
+CREATE FOREIGN CATALOG IF NOT EXISTS lake_test_cat SERVER lake_test_srv TYPE
'hive'; -- skip with notice
+-- catalog names are global: the same name on another server is still a
duplicate
+CREATE FOREIGN CATALOG lake_test_cat SERVER lake_test_srv2 TYPE 'hive';
-- fail, duplicate
+CREATE FOREIGN CATALOG IF NOT EXISTS lake_test_cat SERVER lake_test_srv2 TYPE
'hive'; -- skip with notice
+CREATE FOREIGN CATALOG lake_test_bad SERVER no_such_server TYPE 'hive';
-- fail, no server
+SELECT fcname, fctype, fcoptions FROM pg_foreign_catalog WHERE fcname LIKE
'lake\_test%' ORDER BY 1;
+
+-- CREATE FOREIGN VOLUME
+CREATE FOREIGN VOLUME lake_test_vol SERVER lake_test_srv OPTIONS (path
's3://bucket/prefix');
Review Comment:
The proposal says that volume is "Where data files physically live". Is it
possible that a volume can be without path? What about moving path from options
to required first-class properties?
##########
src/backend/foreign/foreign.c:
##########
@@ -982,6 +984,98 @@ get_foreign_server_oid(const char *servername, bool
missing_ok)
return oid;
}
+/*
+ * get_foreign_catalog_oid - given a foreign catalog name, look up the OID
+ *
+ * If missing_ok is false, throw an error if name not found. If true, just
+ * return InvalidOid.
+ */
+Oid
+get_foreign_catalog_oid(const char *catalogname, bool missing_ok)
+{
+ Oid oid;
+
+ oid = GetSysCacheOid1(FOREIGNCATALOGNAME,
+ Anum_pg_foreign_catalog_oid,
+ CStringGetDatum(catalogname));
+ if (!OidIsValid(oid) && !missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("foreign catalog \"%s\" does not exist",
+ catalogname)));
+
+ return oid;
+}
+
+/*
+ * get_foreign_volume_oid - given a foreign volume name, look up the OID
+ *
+ * If missing_ok is false, throw an error if name not found. If true, just
+ * return InvalidOid.
+ */
+Oid
+get_foreign_volume_oid(const char *volumename, bool missing_ok)
+{
+ Oid oid;
+
+ oid = GetSysCacheOid1(FOREIGNVOLUMENAME,
+ Anum_pg_foreign_volume_oid,
+ CStringGetDatum(volumename));
+ if (!OidIsValid(oid) && !missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("foreign volume \"%s\" does not exist",
+ volumename)));
+
+ return oid;
+}
+
+/*
+ * GetForeignVolumeByName - look up a foreign volume by name
+ */
+ForeignVolume *
+GetForeignVolumeByName(const char *volumename, bool missing_ok)
+{
+ HeapTuple tp;
+ Form_pg_foreign_volume fvform;
+ ForeignVolume *volume;
+ Datum datum;
+ bool isnull;
+
+ tp = SearchSysCache1(FOREIGNVOLUMENAME,
+ PointerGetDatum(volumename));
+ if (!HeapTupleIsValid(tp))
+ {
+ if (!missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("foreign volume \"%s\" does not
exist",
+ volumename)));
+ return NULL;
+ }
+
+ fvform = (Form_pg_foreign_volume) GETSTRUCT(tp);
+
+ volume = (ForeignVolume *) palloc(sizeof(ForeignVolume));
+ volume->volumeid = fvform->oid;
+ volume->serverid = fvform->fvserver;
+ volume->volumename = pstrdup(NameStr(fvform->fvname));
+
+ /* Extract the volume options */
+ datum = SysCacheGetAttr(FOREIGNVOLUMENAME,
+ tp,
+
Anum_pg_foreign_volume_fvoptions,
+ &isnull);
+ if (isnull)
+ volume->options = NIL;
+ else
+ volume->options = untransformRelOptions(datum);
Review Comment:
I suggest using ternary operator to make the code more compact and to
explicitly write that for any value of the condition, we change one variable
```suggestion
volume->options = (isnull) ? NIL : untransformRelOptions(datum);
```
--
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]
