Sql catalog by JanKaul · Pull Request #229 · apache/iceberg-rust

JanKaul · 2024-03-04T15:14:04Z

This PR implements the basic operations for a Sql catalog. The implementation uses the sqlx crate which enables Postgres, MySQL and Sqlite.

The update_table method is to be implemented later.

JanKaul · 2024-03-04T16:40:53Z

PTAL @liurenjie1024 @Xuanwo @ZENOTME @Fokko

crates/catalog/sql/Cargo.toml

crates/catalog/sql/src/catalog.rs

martin-g · 2024-03-05T21:38:21Z

crates/catalog/sql/src/catalog.rs

+        let rows = connection.transaction(|txn|{
+            let name = self.name.clone();
+            Box::pin(async move {
+            sqlx::query(&format!("select distinct table_namespace from iceberg_tables where catalog_name = '{}';",&name)).fetch_all(&mut **txn).await


Should you care about SQL injections ? Or the catalog / namespace / table names are assumed to be safe ?

Good point, it's better to use prepare statement here.

There are several points with this implementation:

If parent is None, we should list all namespaces.

We should also count namespaces in iceberg_namespace_properties

We should list only sub namespaces.

See java implementation here.

crates/catalog/sql/src/error.rs

crates/catalog/sql/src/catalog.rs

crates/catalog/sql/Cargo.toml

crates/catalog/sql/src/catalog.rs

crates/catalog/sql/Cargo.toml

crates/catalog/sql/src/catalog.rs

odysa · 2024-03-05T21:54:20Z

crates/catalog/sql/src/catalog.rs

+    }
+
+    async fn load_table(&self, identifier: &TableIdent) -> Result<Table> {
+        let metadata_location = {


Do we need to check the cache first? Given that it's inserted later.

Also the insertion should not blind, we need to check its version first. My suggestion is to remove the cache for now so that things don't get too complicated.

The cache is only intended for update_table. Optimistically assuming that the metadata_location hasn't been changed since loading the table, the metadata and metadata_location from the cache can directly be used to perform the update. This way the database has to be queried only once for the optimistic case.
If the metadata_location changed, the update method has to be more involved.

I would not use the cache for loading tables.

I'm thinking maybe we should have a standalone data structure for caching, just like CachingCatalog in java

odysa · 2024-03-05T21:59:57Z

crates/catalog/sql/src/catalog.rs

+                Box::pin(async move {
+                    sqlx::query(
+                        "create table if not exists iceberg_namespace_properties (
+                                catalog_name text not null,


VARCHAR(255) here. Maybe we can copy&paste SQL from java iceberg?

+ CATALOG_NAME + " VARCHAR(255) NOT NULL," + NAMESPACE_NAME + " VARCHAR(255) NOT NULL," + NAMESPACE_PROPERTY_KEY + " VARCHAR(255)," + NAMESPACE_PROPERTY_VALUE + " VARCHAR(1000),"

crates/catalog/sql/Cargo.toml

liurenjie1024 · 2024-03-06T01:58:30Z

crates/catalog/sql/src/catalog.rs

+        let rows = connection.transaction(|txn|{
+            let name = self.name.clone();
+            Box::pin(async move {
+            sqlx::query(&format!("select distinct table_namespace from iceberg_tables where catalog_name = '{}';",&name)).fetch_all(&mut **txn).await


Good point, it's better to use prepare statement here.

liurenjie1024 · 2024-03-06T02:41:41Z

crates/catalog/sql/src/catalog.rs

+        let rows = connection.transaction(|txn|{
+            let name = self.name.clone();
+            Box::pin(async move {
+            sqlx::query(&format!("select distinct table_namespace from iceberg_tables where catalog_name = '{}';",&name)).fetch_all(&mut **txn).await


There are several points with this implementation:

If parent is None, we should list all namespaces.

We should also count namespaces in iceberg_namespace_properties

We should list only sub namespaces.

See java implementation here.

crates/catalog/sql/src/lib.rs

liurenjie1024 · 2024-03-06T02:46:38Z

crates/catalog/sql/src/catalog.rs

+                        y.table_namespace
+                            .split('.')
+                            .map(ToString::to_string)
+                            .collect::<Vec<_>>(),


How about extract this to a common method in NamespaceIdent?

crates/catalog/sql/src/catalog.rs

liurenjie1024 · 2024-03-06T02:54:20Z

crates/catalog/sql/src/catalog.rs

+    }
+
+    async fn load_table(&self, identifier: &TableIdent) -> Result<Table> {
+        let metadata_location = {


Also the insertion should not blind, we need to check its version first. My suggestion is to remove the cache for now so that things don't get too complicated.

liurenjie1024 · 2024-03-06T03:13:54Z

crates/iceberg/src/spec/table_metadata.rs

 pub type TableMetadataRef = Arc<TableMetadata>;

-#[derive(Debug, PartialEq, Deserialize, Eq, Clone)]
+#[derive(Debug, PartialEq, Deserialize, Eq, Clone, TypedBuilder)]


We had a discussion in this pr about the table metadata builder. I have concern on this derived builder since it's error prone and not easy to review. TableMetadataBuilder will be heavily used by transaction api and we will need to do a lot of check for in it. I would suggest to maintain this struct manually, what do you think?

liurenjie1024 · 2024-04-16T14:19:22Z

cc @JanKaul Is this pr ready for review or you need to do more updates?

JanKaul · 2024-04-17T05:24:08Z

I have to add a couple of more changes. I'll notify you when I'm finished.

himadripal · 2024-04-18T04:55:13Z

@JanKaul WDYT? I think this PR is ready for review, I can add the update and delete in a separate PR.

liurenjie1024 · 2024-04-25T03:33:16Z

@JanKaul WDYT? I think this PR is ready for review, I can add the update and delete in a separate PR.

Cool, I'll take a look first.

JanKaul · 2024-04-25T08:51:21Z

Thank you all for your helpful comments. I think the PR is ready for review again.

@liurenjie1024 @sdd @odysa @ZENOTME @martin-g

liurenjie1024

Thanks @JanKaul for this pr, we moved a huge step forward! I think there are some places we can improve a little to make it more robust.

crates/catalog/sql/Cargo.toml

crates/catalog/sql/src/catalog.rs

liurenjie1024 · 2024-04-25T10:45:01Z

crates/catalog/sql/src/catalog.rs

+    name: String,
+    connection: AnyPool,
+    storage: FileIO,
+    cache: Arc<DashMap<TableIdent, (String, TableMetadata)>>,


I'm hesitating to add cache here, maybe we can add sth like CachedCatalog in java so that all catalog implementations could benefit from it?

crates/catalog/sql/src/catalog.rs

liurenjie1024 · 2024-04-25T14:33:14Z

crates/catalog/sql/src/catalog.rs

+        Ok(table)
+    }
+
+    async fn create_table(


There are some things missing here:

We should first check namespace exists

The location is optional, it should use warehouse's subdir as location

I would suggest to refer to python's implementation,

fix connection pool issue for sql catalog

create sqlconfig, fix rest of the tests and remove todo

Co-authored-by: Renjie Liu <[email protected]>

callum-ryan · 2024-06-21T20:34:01Z

the bind placeholder ? will not work with pg databases, which use $1..$N format unfortunately. sqlx docs

JanKaul · 2024-06-22T06:09:07Z

Thanks for bringing this up, I haven't thought about it. It's weird though, the test is using sqlite which it's supposed to not work with ? according to the docs.

JanKaul requested review from Fokko, Xuanwo and liurenjie1024 March 4, 2024 16:41

sdd reviewed Mar 5, 2024

View reviewed changes

crates/catalog/sql/Cargo.toml Outdated Show resolved Hide resolved

sdd reviewed Mar 5, 2024

View reviewed changes

crates/catalog/sql/src/catalog.rs Outdated Show resolved Hide resolved

sdd reviewed Mar 5, 2024

View reviewed changes

crates/catalog/sql/src/catalog.rs Show resolved Hide resolved

martin-g reviewed Mar 5, 2024

View reviewed changes

crates/catalog/sql/src/error.rs Outdated Show resolved Hide resolved

odysa reviewed Mar 5, 2024

View reviewed changes

liurenjie1024 mentioned this pull request Mar 7, 2024

Discussion: Design of TableMetadataBuilder. #232

Closed

liurenjie1024 reviewed Mar 11, 2024

View reviewed changes

liurenjie1024 linked an issue Mar 11, 2024 that may be closed by this pull request

Implement Sql Catalog. #248

Closed

JanKaul force-pushed the sql-catalog branch from b286678 to 307fe1d Compare March 13, 2024 10:25

JanKaul force-pushed the sql-catalog branch from 307fe1d to 042fd03 Compare April 8, 2024 18:23

Fokko added this to the 0.3.0 Release milestone Apr 24, 2024

Fokko mentioned this pull request Apr 24, 2024

Tracking issues of iceberg-rust v0.3.0 #348

Closed

73 tasks

JanKaul force-pushed the sql-catalog branch from b2cbc5d to 9893068 Compare April 25, 2024 07:38

liurenjie1024 reviewed Apr 25, 2024

View reviewed changes

liurenjie1024 removed this from the 0.3.0 Release milestone Jun 19, 2024

JanKaul added 5 commits June 20, 2024 21:35

initialize sql catalog crate

70d91f2

implement basic sql catalog functionality

2df82ae

fix clippy warnings

8de3373

fix format

e73fce2

fix cargo sort

caeba8f

hpal and others added 26 commits June 20, 2024 21:35

style fix and few additional logic removal and add todo check.

a9bdb81

rebase

769f8f5

create sqlconfig, fix rest of the tests and remove todo

4dc95c3

Merge pull request #1 from himadripal/sql-catalog-conn-pool-fix

989c61b

fix connection pool issue for sql catalog

Merge pull request #2 from himadripal/sql-catalog-conn-pool-fix-2

222f360

create sqlconfig, fix rest of the tests and remove todo

use varchar

0e7b2ea

use static strings for sql identifiers

01c9e43

fix namespace encoding

fead32c

fix typo

d18393d

Fix heading

0b1141c

Co-authored-by: Renjie Liu <[email protected]>

Update crates/catalog/sql/src/catalog.rs

80e22d5

Co-authored-by: Renjie Liu <[email protected]>

rename fileio

193da0b

simplify conversion from string to NamespaceIdent

0318576

use uri for database connection

ab65793

use features for sqlx database support

214274a

use statics for pool default values

23db911

move derive Default

75bf2f3

create new tempdir for every test run

841f916

use table record type

fe1ea0c

use parent for list namespaces

8851e3c

fix typo

f1d0582

fix clippy warning

4967b76

use statics for namespace sql

6975123

create namespace

bef93b2

remove cache

4bf7e2f

fix list_namespaces

75898b5

JanKaul force-pushed the sql-catalog branch from 9e64605 to 75898b5 Compare June 20, 2024 19:35

JanKaul closed this Jul 30, 2024

Comments

Conversation

JanKaul commented Mar 4, 2024

Uh oh!

JanKaul commented Mar 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 commented Apr 16, 2024

Uh oh!

JanKaul commented Apr 17, 2024

Uh oh!

himadripal commented Apr 18, 2024

Uh oh!

liurenjie1024 commented Apr 25, 2024

Uh oh!

JanKaul commented Apr 25, 2024

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

callum-ryan commented Jun 21, 2024

Uh oh!

JanKaul commented Jun 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone