You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "ahmedabu98 (via GitHub)" <gi...@apache.org> on 2025/06/10 13:55:57 UTC

[PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

ahmedabu98 opened a new pull request, #35223:
URL: https://github.com/apache/beam/pull/35223

   ## Motivation
   
   Modern data architectures, particularly data lakes and lakehouses (e.g., Apache Iceberg), heavily rely on catalogs for centralized metadata management. Beam SQL currently lacks this concept, limiting its interoperability and ease of use within these ecosystems.
   
   While Beam SQL has a [MetaStore](https://github.com/apache/beam/blob/master/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/store/MetaStore.java) for managing table definitions and providers, it operates as a single, flat namespace. Users cannot create or configure multiple, distinct metastores or easily switch between them. This PR introduces the concept of a Catalog as a higher-level organizational unit that leverages existing MetaStore capabilities for table management.
   
   ## Usage
   
   This PR introduces the following DDL commands for managing catalogs in Beam SQL:
   
   ### Create a new catalog
   
   ```sql
   CREATE CATALOG my_catalog 
   TYPE 'local'
   PROPERTIES (
     'foo', 'bar',
     'abc', 'xyz'
   )
   ```
   
   ### Set the current catalog
   
   ```sql
   SET CATALOG my_catalog
   ```
   
   ### Drop a catalog
   
   ```sql
   DROP CATALOG my_catalog
   ```
   
   ## Changes
   This change preserves backwards compatibility. It introduces two new interfaces and their in-memory implementations:
   
   ### _1. Catalog_
   
   This generally represents the aforementioned catalog, and includes the following attributes:
   - name (`string`): Unique identifier for the catalog
   - type (`string`): The catalog's implementation (e.g. `local`, `iceberg`, etc.)
   - properties (`map<string, string>`): The catalog's configuration
   - ([MetaStore](https://github.com/apache/beam/blob/master/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/store/MetaStore.java)): Manages the catalog's tables and providers
   
   Tables are now scoped within a specific Catalog rather than a global scope. For example, a Beam SQL Table created in Catalog A will not be available when switching to Catalog B. Catalog B would have to create a new reference for its scope.
   
   A default in-memory catalog named `'default'` is automatically initialized.
   
   
   ### _2. CatalogManager_
   
   This interface is effectively the new root schema in Beam SQL's Calcite integration. Previously, an [InMemoryMetaStore](https://github.com/apache/beam/blob/master/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/store/InMemoryMetaStore.java) served as the root. With CatalogManager as the top-level container, individual Catalog instances can be created and registered as sub-schemas. This allows CatalogManager to do the following:
   
   - create, managed, and drop catalogs
   - switch between different active catalogs
   - register global TableProviders that become available to all catalogs
   
   The PR includes implementations for these two interfaces:
   - `InMemoryCatalog` implements `Catalog`
   - `InMemoryCatalogManager` implements `CatalogManager`


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138245665


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/Catalog.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+
+public interface Catalog {

Review Comment:
   One thing to note though -- in other places, DDL methods are directly accessible via the Catalog (even if they may be redirected to an internal metastore/catalog). The current PR expects us to drill down an extra step (i.e. `.metaStore()` to access such methods). 
   
   I'm slightly leaning towards keeping it this way, but am also open to surfacing all those methods to the Catalog interface. For us, it means the following methods:
   - createTable()
   - dropTable()
   - getTable()
   - getTables()
   
   Note that our MetaStore is missing the following:
   - createNamespace()
   - listNamespaces()
   - dropNamespace()
   - alterTable()
   - renameTable()



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138259428


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/ReadOnlyCatalogManager.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.provider.TableProvider;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableMap;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public class ReadOnlyCatalogManager implements CatalogManager {

Review Comment:
   Renamed



##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/CatalogManager.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.provider.TableProvider;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public interface CatalogManager {
+  void createCatalog(String name, String type, Map<String, String> properties);
+
+  void useCatalog(String name);
+
+  Catalog currentCatalog();
+
+  @Nullable
+  Catalog getCatalog(String name);
+
+  void removeCatalog(String name);

Review Comment:
   Done



##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/BeamSqlCli.java:
##########
@@ -35,14 +36,19 @@ public class BeamSqlCli {
   /** The store which persists all the table meta data. */
   private MetaStore metaStore;
 
-  public BeamSqlCli metaStore(MetaStore metaStore) {
-    return metaStore(metaStore, false, PipelineOptionsFactory.create());
+  public BeamSqlCli catalogManager(CatalogManager catalogManager) {

Review Comment:
   I'm not familiar with the entry points of Beam SQL, but our public docs [1], [2] seem to show only `SqlTransform`. I'm on board with marking it `@Internal`
   
   [1] https://beam.apache.org/documentation/dsls/sql/walkthrough/
   [2] https://beam.apache.org/documentation/dsls/sql/overview/



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138376723


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/Catalog.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+
+public interface Catalog {
+  String type();
+
+  MetaStore metaStore();
+
+  String name();
+
+  Map<String, String> properties();
+
+  void initialize(String name, Map<String, String> properties);

Review Comment:
   Done, PTAL



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35223:
URL: https://github.com/apache/beam/pull/35223#issuecomment-2959959320

   Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138261636


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/Catalog.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+
+public interface Catalog {
+  String type();
+
+  MetaStore metaStore();
+
+  String name();
+
+  Map<String, String> properties();
+
+  void initialize(String name, Map<String, String> properties);

Review Comment:
   Implemented it this way to more easily work with ServiceLoader, which requires a public no-arg constructor. 
   
   But I agree it's not the cleanest way to do things. Will try to find another way



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35223:
URL: https://github.com/apache/beam/pull/35223#issuecomment-2960237905

   ## [Codecov](https://app.codecov.io/gh/apache/beam/pull/35223?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: Patch coverage is `67.00680%` with `97 lines` in your changes missing coverage. Please review.
   > Project coverage is 54.49%. Comparing base [(`14ac371`)](https://app.codecov.io/gh/apache/beam/commit/14ac3711e656298036cfd8208c46892f8df36cb7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`0335523`)](https://app.codecov.io/gh/apache/beam/commit/0335523c217d7d4f8c8b3f6197d0d2922622c0f6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 55 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/beam/pull/35223?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...k/extensions/sql/impl/parser/SqlCreateCatalog.java](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&filepath=sdks%2Fjava%2Fextensions%2Fsql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbeam%2Fsdk%2Fextensions%2Fsql%2Fimpl%2Fparser%2FSqlCreateCatalog.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2V4dGVuc2lvbnMvc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9iZWFtL3Nkay9leHRlbnNpb25zL3NxbC9pbXBsL3BhcnNlci9TcWxDcmVhdGVDYXRhbG9nLmphdmE=) | 42.85% | [39 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...sdk/extensions/sql/impl/parser/SqlDropCatalog.java](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&filepath=sdks%2Fjava%2Fextensions%2Fsql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbeam%2Fsdk%2Fextensions%2Fsql%2Fimpl%2Fparser%2FSqlDropCatalog.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2V4dGVuc2lvbnMvc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9iZWFtL3Nkay9leHRlbnNpb25zL3NxbC9pbXBsL3BhcnNlci9TcWxEcm9wQ2F0YWxvZy5qYXZh) | 55.00% | [15 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [.../sdk/extensions/sql/impl/parser/SqlSetCatalog.java](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&filepath=sdks%2Fjava%2Fextensions%2Fsql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbeam%2Fsdk%2Fextensions%2Fsql%2Fimpl%2Fparser%2FSqlSetCatalog.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2V4dGVuc2lvbnMvc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9iZWFtL3Nkay9leHRlbnNpb25zL3NxbC9pbXBsL3BhcnNlci9TcWxTZXRDYXRhbG9nLmphdmE=) | 54.83% | [11 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...sions/sql/meta/catalog/InMemoryCatalogManager.java](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&filepath=sdks%2Fjava%2Fextensions%2Fsql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbeam%2Fsdk%2Fextensions%2Fsql%2Fmeta%2Fcatalog%2FInMemoryCatalogManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2V4dGVuc2lvbnMvc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9iZWFtL3Nkay9leHRlbnNpb25zL3NxbC9tZXRhL2NhdGFsb2cvSW5NZW1vcnlDYXRhbG9nTWFuYWdlci5qYXZh) | 77.27% | [7 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...tensions/sql/meta/catalog/EmptyCatalogManager.java](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&filepath=sdks%2Fjava%2Fextensions%2Fsql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbeam%2Fsdk%2Fextensions%2Fsql%2Fmeta%2Fcatalog%2FEmptyCatalogManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2V4dGVuc2lvbnMvc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9iZWFtL3Nkay9leHRlbnNpb25zL3NxbC9tZXRhL2NhdGFsb2cvRW1wdHlDYXRhbG9nTWFuYWdlci5qYXZh) | 20.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...pache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&filepath=sdks%2Fjava%2Fextensions%2Fsql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbeam%2Fsdk%2Fextensions%2Fsql%2Fimpl%2FBeamSqlEnv.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2V4dGVuc2lvbnMvc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9iZWFtL3Nkay9leHRlbnNpb25zL3NxbC9pbXBsL0JlYW1TcWxFbnYuamF2YQ==) | 91.30% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...eam/sdk/extensions/sql/impl/BeamCalciteSchema.java](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&filepath=sdks%2Fjava%2Fextensions%2Fsql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbeam%2Fsdk%2Fextensions%2Fsql%2Fimpl%2FBeamCalciteSchema.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2V4dGVuc2lvbnMvc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9iZWFtL3Nkay9leHRlbnNpb25zL3NxbC9pbXBsL0JlYW1DYWxjaXRlU2NoZW1hLmphdmE=) | 95.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/beam/pull/35223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #35223      +/-   ##
   ============================================
   - Coverage     56.44%   54.49%   -1.95%     
   + Complexity     3299     1533    -1766     
   ============================================
     Files          1186     1022     -164     
     Lines        181981   161147   -20834     
     Branches       3409     1113    -2296     
   ============================================
   - Hits         102715    87822   -14893     
   + Misses        75995    71206    -4789     
   + Partials       3271     2119    -1152     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/beam/pull/35223/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [java](https://app.codecov.io/gh/apache/beam/pull/35223/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.40% <67.00%> (-2.20%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/beam/pull/35223?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 merged PR #35223:
URL: https://github.com/apache/beam/pull/35223


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138254821


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/InMemoryCatalog.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import com.google.auto.service.AutoService;
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.InMemoryMetaStore;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+import org.apache.beam.sdk.util.Preconditions;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+@AutoService(Catalog.class)

Review Comment:
   Hmmm do you mean something similar to [`PipelineOptionsRegistrar`](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsRegistrar.java) ?
   
   Can certainly do that, but out of curiosity what is the motivation?



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138377208


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/InMemoryCatalog.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import com.google.auto.service.AutoService;
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.InMemoryMetaStore;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+import org.apache.beam.sdk.util.Preconditions;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+@AutoService(Catalog.class)

Review Comment:
   Added a CatalogRegistrar interface and implementation



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138135219


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/BeamSqlCli.java:
##########
@@ -35,14 +36,19 @@ public class BeamSqlCli {
   /** The store which persists all the table meta data. */
   private MetaStore metaStore;
 
-  public BeamSqlCli metaStore(MetaStore metaStore) {
-    return metaStore(metaStore, false, PipelineOptionsFactory.create());
+  public BeamSqlCli catalogManager(CatalogManager catalogManager) {

Review Comment:
   This "public" interface is getting a bit crufty. Is it actually meant for users? I'm thinking we can retroactively mark it `@Internal` which has no effect on existing users but just to make it clear. (and I think we might exclude from Javadoc which would be useful)



##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/PubsubToBigqueryIT.java:
##########
@@ -52,6 +52,8 @@ public class PubsubToBigqueryIT implements Serializable {
 
   @Test
   public void testSimpleInsert() throws Exception {
+    String createCatalog = "CREATE CATALOG my_catalog TYPE `local`";

Review Comment:
   Is this optional just to exercise the new code or is it required now? I presume optional.



##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/CatalogManager.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.provider.TableProvider;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public interface CatalogManager {
+  void createCatalog(String name, String type, Map<String, String> properties);
+
+  void useCatalog(String name);
+
+  Catalog currentCatalog();
+
+  @Nullable
+  Catalog getCatalog(String name);
+
+  void removeCatalog(String name);

Review Comment:
   Is this `DROP CATALOG`? Let's call it that, if so. And some more extensive Javadoc on the class and methods.



##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlCatalogTest.java:
##########
@@ -0,0 +1,20 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.impl.parser;
+
+public class SqlCatalogTest {}

Review Comment:
   Should this... have tests?



##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/Catalog.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+
+public interface Catalog {
+  String type();
+
+  MetaStore metaStore();
+
+  String name();
+
+  Map<String, String> properties();
+
+  void initialize(String name, Map<String, String> properties);

Review Comment:
   Can we avoid having a stateful API with a non-initialized state? This method / these parameters seem like they should be parameters of a static factory method and/or factory object.
   
   UPDATE: I just noticed this is the same as the `create` method of the `CatalogManager` so we should not have this method here.



##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/InMemoryCatalog.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import com.google.auto.service.AutoService;
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.InMemoryMetaStore;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+import org.apache.beam.sdk.util.Preconditions;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+@AutoService(Catalog.class)

Review Comment:
   Prefer to make the AutoService a narrowing "Registrar" API, not combined with the overall class that has the logic.



##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/ReadOnlyCatalogManager.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.provider.TableProvider;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableMap;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public class ReadOnlyCatalogManager implements CatalogManager {

Review Comment:
   This is not just ReadOnly but empty, right?



##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/Catalog.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+
+public interface Catalog {

Review Comment:
   How does this compare to other Catalog impls in related places? Just to check that we can learn from them what all the eventual fields it will need might be.
   
   Also javadoc the class and methods.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138242652


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/Catalog.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.meta.catalog;
+
+import java.util.Map;
+import org.apache.beam.sdk.extensions.sql.meta.store.MetaStore;
+
+public interface Catalog {

Review Comment:
   Added javadocs to Catalog and CatalogManager
   
   [Flink Catalog](https://github.com/apache/flink/blob/master/flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/AbstractCatalog.java) (include [Apache Hudi](https://github.com/apache/hudi/blob/f9555910e5dcb0380569591e6b20f02c1433f24f/hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/catalog/HoodieHiveCatalog.java#L126))
   - name
   - default database
   
   [Spark Catalog](https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/CatalogPlugin.java#L45)
   - name
   - default namespace
   
   [Iceberg Catalog](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/catalog/Catalog.java)
   - name
   - properties (implicit)
   
   [Delta Lake Catalog](https://github.com/delta-io/delta/blob/master/connectors/flink/src/main/java/io/delta/flink/internal/table/DeltaCatalog.java)
   - name
   - properties (in the form of a Hadoop Conf)
   - metastore (they call it decoratedCatalog)
   
   [Snowflake Polaris Catalog](https://github.com/apache/polaris/blob/main/client/python/docs/Catalog.md)
   - name
   - type
   - properties
   - create_timestamp
   - last_update_timestamp
   
   I think we're good with the attributes we have here.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [Beam SQL] Implement Catalog and CatalogManager [beam]

Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35223:
URL: https://github.com/apache/beam/pull/35223#discussion_r2138259974


##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/PubsubToBigqueryIT.java:
##########
@@ -52,6 +52,8 @@ public class PubsubToBigqueryIT implements Serializable {
 
   @Test
   public void testSimpleInsert() throws Exception {
+    String createCatalog = "CREATE CATALOG my_catalog TYPE `local`";

Review Comment:
   Correct, it is optional. 



##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlCatalogTest.java:
##########
@@ -0,0 +1,20 @@
+/*
+ * 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.
+ */
+package org.apache.beam.sdk.extensions.sql.impl.parser;
+
+public class SqlCatalogTest {}

Review Comment:
   Actual tests are in `BeamSqlCliTest`, just forgot to clean this up



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org