feat: implement IcebergTableProviderFactory for datafusion #600

yukkit · 2024-09-03T14:34:42Z

resolve #586

TODO:

Supplement unit tests and add validation of data
Add examples of how to integrate into DataFusion

liurenjie1024

Thanks @yukkit for this pr! Generally looks good to me, I've left minor points to improve.

liurenjie1024 · 2024-09-06T03:23:20Z

crates/integrations/datafusion/src/table/table_provider_factory.rs

+    if !table_partition_cols.is_empty() {
+        return Err(Error::new(
+            ErrorKind::FeatureUnsupported,
+            "Partition columns cannot be specified",


Instead of returning different errors for each check, I think a more appropriate message would be like

Currently we only support reading existing icebergs tables in external table command. To create new table, please use catalog provider.

Ok, I roughly understand your intention. First, inform the user that external tables currently only support read operations, but this doesn't mean write operations won't be supported in the future. Secondly, provide a method for creating a new table.

liurenjie1024 · 2024-09-06T03:25:10Z

crates/integrations/datafusion/src/table/table_provider_factory.rs

+        // Field { name: "y", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {"PARQUET:field_id": "2"} },
+        // Field { name: "z", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {"PARQUET:field_id": "3"} }
+        let actual_schema = table_provider.schema();
+        let expected_schema_str = "Field { name: \"x\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {\"PARQUET:field_id\": \"1\"} }, Field { name: \"y\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {\"PARQUET:field_id\": \"2\"} }, Field { name: \"z\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {\"PARQUET:field_id\": \"3\"} }";


Pure format string comparison doesn't sound like a good idea to me. It would be better to use schema builder api to create schema and compare them.

sounds good

liurenjie1024 · 2024-09-06T03:27:34Z

crates/integrations/datafusion/src/table/table_provider_factory.rs

+
+#[derive(Default)]
+#[non_exhaustive]
+pub struct IcebergTableProviderFactory {}


It would be better to provide some doc to explain its usage.

Alright, I will add doc for it

yukkit · 2024-09-07T07:26:10Z

crates/integrations/datafusion/src/table/table_provider_factory.rs

+fn complement_namespace_if_necessary(table_name: &TableReference) -> Cow<'_, TableReference> {
+    match table_name {
+        TableReference::Bare { table } => {
+            Cow::Owned(TableReference::partial("default", table.as_ref()))
+        }
+        other => Cow::Borrowed(other),
+    }
+}


Please pay attention to this function, because when DataFusion creates external tables, it only creates bare TableReference, which does not include a schema. However, when constructing TableIdent in Iceberg, the 'schema' is required. Does anyone have a good approach for handling compatibility?

I ran into an issue with external tables only creating bare TableReference as well - do you know if thats the intended behavior or if its a bug in datafusion?

@matthewmturner I believe this is a known issue, see https://github.com/apache/datafusion/blob/main/datafusion/sql/src/statement.rs#L1242

Yeah weve got a ticket created for that now. thanks @yukkit

Does anyone have a good approach for handling compatibility?

I'm not quite familiar with datafusion, but typicall it should be registered with current namespace? For example, a user has executed use ns1 before registering this table, then ideally this table should be register under ns1?

@liurenjie1024 You’re right. When creating an external table in DataFusion without specifying a schema, it will to being created under the default schema. In this case, the table name passed to TableProviderFactory does not include schema information. However, TableIdent requires a namespace, so I use "default" to populate this value. Anyway, the IcebergTableProvider doesn't actually rely on the specific content of TableIdent, so it has no impact when used as an external table in DataFusion (at least as things stand).

yukkit · 2024-09-11T02:59:35Z

@liurenjie1024 Please help review the PR again when you get a chance

yukkit · 2024-09-27T03:07:41Z

remind @liurenjie1024

liurenjie1024

Thanks @yukkit for this pr, LGTM! Just some minor problems to fix, others are great!

liurenjie1024 · 2024-10-12T08:33:20Z

crates/integrations/datafusion/src/table/table_provider_factory.rs

+/// An error will be returned if any unsupported feature, such as partition columns,
+/// order expressions, constraints, or column defaults, is detected in the table creation command.
+#[derive(Default)]
+#[non_exhaustive]


Sorry, I don't quite understand why we need to add this annotation here?

liurenjie1024 · 2024-10-12T09:11:35Z

crates/integrations/datafusion/src/table/table_provider_factory.rs

+fn complement_namespace_if_necessary(table_name: &TableReference) -> Cow<'_, TableReference> {
+    match table_name {
+        TableReference::Bare { table } => {
+            Cow::Owned(TableReference::partial("default", table.as_ref()))
+        }
+        other => Cow::Borrowed(other),
+    }
+}


Does anyone have a good approach for handling compatibility?

I'm not quite familiar with datafusion, but typicall it should be registered with current namespace? For example, a user has executed use ns1 before registering this table, then ideally this table should be register under ns1?

liurenjie1024 · 2024-10-12T09:15:31Z

remind @liurenjie1024

Sorry for late reply, would you mind to fix comments and resolve conflicts?

liurenjie1024 · 2024-11-01T13:04:34Z

Thanks @yukkit for this pr!

* feat: implement IcebergTableProviderFactory for datafusion * fix comments * add doc&ut * remove print * fix comments

liurenjie1024 reviewed Sep 6, 2024

View reviewed changes

yukkit commented Sep 7, 2024

View reviewed changes

yukkit force-pushed the table_factory branch from c350f7e to 625cdbf Compare September 7, 2024 07:33

yukkit requested a review from liurenjie1024 September 7, 2024 07:57

yukkit force-pushed the table_factory branch from 0582384 to eb466b5 Compare September 27, 2024 03:06

liurenjie1024 reviewed Oct 12, 2024

View reviewed changes

shehabgamin mentioned this pull request Oct 12, 2024

Iceberg lakehq/sail#242

Draft

yukkit added 5 commits October 26, 2024 16:39

feat: implement IcebergTableProviderFactory for datafusion

5d2ad4c

fix comments

2eeae0e

add doc&ut

b6d95b0

remove print

111e700

fix comments

6ae219d

yukkit force-pushed the table_factory branch from eb466b5 to 6ae219d Compare October 26, 2024 08:44

yukkit requested a review from liurenjie1024 October 28, 2024 03:20

liurenjie1024 approved these changes Nov 1, 2024

View reviewed changes

liurenjie1024 merged commit b8f088e into apache:main Nov 1, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement IcebergTableProviderFactory for datafusion #600

feat: implement IcebergTableProviderFactory for datafusion #600

yukkit commented Sep 3, 2024 •

edited

Loading

liurenjie1024 left a comment

liurenjie1024 Sep 6, 2024

yukkit Sep 7, 2024

liurenjie1024 Sep 6, 2024

yukkit Sep 7, 2024

liurenjie1024 Sep 6, 2024

yukkit Sep 7, 2024

yukkit Sep 7, 2024

matthewmturner Sep 24, 2024

yukkit Sep 26, 2024

matthewmturner Sep 26, 2024

liurenjie1024 Oct 12, 2024

yukkit Oct 26, 2024

yukkit commented Sep 11, 2024

yukkit commented Sep 27, 2024

liurenjie1024 left a comment

liurenjie1024 Oct 12, 2024

liurenjie1024 Oct 12, 2024

liurenjie1024 commented Oct 12, 2024

liurenjie1024 commented Nov 1, 2024

feat: implement IcebergTableProviderFactory for datafusion #600

feat: implement IcebergTableProviderFactory for datafusion #600

Conversation

yukkit commented Sep 3, 2024 • edited Loading

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukkit commented Sep 11, 2024

yukkit commented Sep 27, 2024

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 commented Oct 12, 2024

liurenjie1024 commented Nov 1, 2024

yukkit commented Sep 3, 2024 •

edited

Loading