Skip to content
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

Remove "NOT YET FULLY SUPPORTED" comment from DataType::Utf8View/BinaryView #6380

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Sep 11, 2024

Which issue does this PR close?

Part of #6163

Rationale for this change

As I was reviewing #6368 I came across a warning about Utf8View being not completely supported which was added in #5470 by @XiangpengHao when we began the implementation

Given these types are now supported across most of this crate I think this warning is no longer relevant

What changes are included in this PR?

Remove warning in comments

Are there any user-facing changes?

Doc changes, no functional changes

@github-actions github-actions bot added the arrow Changes to the arrow crate label Sep 11, 2024
@alamb alamb force-pushed the alamb/remove_string_view_warning branch from 791fc20 to 1e029dc Compare September 11, 2024 11:06
@alamb alamb changed the title Alamb/remove string view warning Remove "NOT YET FULLY SUPPORTED" comment from DataType::Utf8View/BinaryView Sep 11, 2024
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@kylebarron
Copy link
Contributor

Do you intend to leave in the warnings about ListView and LargeListView?

/// (NOT YET FULLY SUPPORTED) A list of some logical data type with variable length.
///
/// Note this data type is not yet fully supported. Using it with arrow APIs may result in `panic`s.

/// (NOT YET FULLY SUPPORTED) A list of some logical data type with variable length and 64-bit offsets.
///
/// Note this data type is not yet fully supported. Using it with arrow APIs may result in `panic`s.

@alamb
Copy link
Contributor Author

alamb commented Sep 11, 2024

Do you intend to leave in the warnings about ListView and LargeListView?

/// (NOT YET FULLY SUPPORTED) A list of some logical data type with variable length.
///
/// Note this data type is not yet fully supported. Using it with arrow APIs may result in `panic`s.

/// (NOT YET FULLY SUPPORTED) A list of some logical data type with variable length and 64-bit offsets.
///
/// Note this data type is not yet fully supported. Using it with arrow APIs may result in `panic`s.

Unfortunately I think they are still largely unimplemented (tracked by #5375)

@Kikkon has some work here #5501 but needs someone to help review it

@alamb alamb merged commit aad55d5 into apache:master Sep 17, 2024
26 checks passed
@alamb
Copy link
Contributor Author

alamb commented Sep 17, 2024

Thanks for the reviews @Xuanwo and @kylebarron

@alamb alamb deleted the alamb/remove_string_view_warning branch September 17, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants