[c++] Add c++ support for Array data type#501
[c++] Add c++ support for Array data type#501charlesdong1991 wants to merge 9 commits intoapache:mainfrom
Conversation
|
@fresh-borzoni @leekeiabstraction appreciate it a lot if you can take a look! 🙏 thanks! |
leekeiabstraction
left a comment
leekeiabstraction
left a comment
There was a problem hiding this comment.
TY for the PR. Left early comments.
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@charlesdong1991 Ty, looks good in general, added minor comments, PTAL
| /// itself an array) is deep-copied into a shared owning handle so that | ||
| /// copies of the outer DataType remain cheap while the element lives | ||
| /// as long as any reference exists. | ||
| static DataType Array(DataType element) { |
There was a problem hiding this comment.
Should we add element nullability?
Rust's ArrayType::with_nullable(false, e) carries it, by dropping here we silently lose ARRAY on schema round-trip. Probably the same applies to python, do you mind to check?
There was a problem hiding this comment.
Good observation, with another look, it seems nullability is actually not in place for every data type, not limited to Array. The same with python.
I suggest create a separate issue to fix nullability instead, and focus on supporting array type in this PR.
WDYT? If you agree, i can create 2 issues for python and c++ @fresh-borzoni
There was a problem hiding this comment.
Created 2 issues
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@charlesdong1991 Ty for the great work! LGTM 👍
3 participants
Purpose
Linked issue: close #468
Brief change log
Tests
All tests are passed
API and Format
Documentation