feat: support add sub-column to struct col#5126
feat: support add sub-column to struct col#5126jackye1995 merged 2 commits intolance-format:mainfrom
Conversation
dd91de3 to
b49887f
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
b49887f to
a633fc0
Compare
|
In this pr, adding sub-columns to struct column only supports versions 2.1 and later. Considering that the v2.0 format will gradually be phased out, I think it might be unnecessary to support adding sub-columns for v2.0. If we need to support adding sub-columns for v2.0, the main challenge is: In v2.0, a header column is stored for nested columns (struct, list), and it assumes the same header column will not exist multiple times within a single fragment. There are two possible approaches:
|
|
The failures are caused by #5206. |
|
Hi @jackye1995 , could you help review when you have time, looking forward to your suggestions! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async fn test_add_sub_column_to_packed_struct_col(version: LanceFileVersion) { | ||
| let mut metadata = HashMap::new(); | ||
| metadata.insert("lance-encoding:packed".to_string(), "true".to_string()); | ||
| let mut dataset = prepare_initial_dataset_with_struct_col(version, metadata).await; |
There was a problem hiding this comment.
Import HashMap for new struct column tests
The added tests call HashMap::new() (and pass HashMap<String, String> to helpers) but the surrounding test module never imports std::collections::HashMap. As written these tests won’t compile (cannot find type HashMap in this scope) and will block CI. Add the missing import or qualify the type before using it.
Useful? React with 👍 / 👎.
9479118 to
aaf562e
Compare
aaf562e to
79ba8bb
Compare
|
This is a prerequisite for the variant (#5238). Hi @jackye1995 , could you help review this when you have time, thanks very much~ |
|
File format v2.1 has been stabilized, and we should not add any more features to it. Instead, please target file format 2.2. |
ffb02f1 to
f51d118
Compare
@Xuanwo Thanks your reminding! Marked v2.1 as unsupported in adding sub-column. |
f51d118 to
ef48b26
Compare
|
Fixed all comments. Hi @jackye1995 , please help review when you have time, thanks very much~ |
7987045 to
c2c0778
Compare
06ad2d6 to
062d6b3
Compare
|
I rebased this pr and it is ready for review now. Hi @jackye1995 , please help when you have time, thanks very much~. |
jackye1995
left a comment
There was a problem hiding this comment.
minor error message comment, other things look good to me
| (DataType::FixedSizeList(fl, _), DataType::FixedSizeList(fr, _)) => { | ||
| check_field_conflict(fl, fr, version) | ||
| } | ||
| (_, _) => Err(Error::invalid_input( |
There was a problem hiding this comment.
sorry I guess this actually need 2 different error messages, one is this one and one is the original one, because if the type is the same then this message is not really right. could you update that?
062d6b3 to
d7681a3
Compare
Adding sub-columns to a struct data type column. --------- Co-authored-by: lijinglun <lijinglun@bytedance.com>
Adding sub-columns to a struct data type column.