-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support create partition table for non REST catalog #577
base: main
Are you sure you want to change the base?
Conversation
Hi, @FANNG1 Thanks for your contribution. The reason why we use |
I agree that pub struct TableCreation {
/// The name of the table.
pub name: String,
/// The location of the table.
#[builder(default, setter(strip_option))]
pub location: Option<String>,
/// The schema of the table.
pub schema: Schema,
/// The partition spec of the table, could be None.
#[builder(default, setter(strip_option, into))]
pub partition_spec: Option<PartitionSpec>,
/// The sort order of the table.
#[builder(default, setter(strip_option))]
pub sort_order: Option<SortOrder>,
/// The properties of the table.
#[builder(default)]
pub properties: HashMap<String, String>,
} |
The reason we use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @FANNG1 for this pr. However I think there are some prepartion work before we can actually finished this pr. If we can narrow down the goal of this pr to change type for UnboundPartitonSpec
to PartitionSpec
only, we can merge this.
"Can't create table with partition spec now", | ||
)) | ||
} | ||
Some(p) => HashMap::from([(p.spec_id(), Arc::new(p))]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, we can't simply add a spec. We should use AddPartitionSpec
transaction api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I couldn't understand AddPartitionSpec transaction api
, could you provide more context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can refer to iceberg-python's implementation The whole table creation process is treated as a transaction, similar to transactions.
} | ||
|
||
#[tokio::test] | ||
async fn test_partition_table() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests
dir is integration tests for iceberg crate. I think this is create partitioned table should apply to all catalogs, rather memory catalog only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean we should test again for all catalogs, like Hive, REST, JDBC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it would be easier if we could finish #519 first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions:
- Please don't remove
ANCHOR
comments, those are for websites. - It would be better to add another create partitioned table example rather modifying current one.
@liurenjie1024 , thanks for your review, let's keep consistent with what means
What you mean is just keeping the first part? |
Yes, I think keeping only part 1 is a good start. Add
I think it's better to implement them in transaction api. |
fixes: #578