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

ARROW-12240: [Python] Fix invalid-offsetof warning #10154

Closed
wants to merge 4 commits into from

Conversation

cyb70289
Copy link
Contributor

@cyb70289 cyb70289 commented Apr 25, 2021

arrow::csv::ConvertOptions has stl container data members which are
not guaranteed to be standard layout type. It causes invalid-offsetof
warning when compiled with clang-12.

This patch changes directly embedded CCSVConvertOptions to unique_ptr
to fix the issue. Parse and Read options are also updated.

@github-actions
Copy link

@cyb70289 cyb70289 force-pushed the 12240-invalid-offsetof branch from 1cd2e07 to 77495bf Compare April 25, 2021 10:16
@cyb70289 cyb70289 marked this pull request as draft April 25, 2021 13:23
@cyb70289
Copy link
Contributor Author

Looks we cannot simply using the unique_ptr trick here.
CSV options should be persistent by pickle. Change data member to pointer crashes related unit tests

convert_options=pa.csv.ConvertOptions(strings_can_be_null=True)),

All compute kernel options are not pickle-able as we've embedded unique_ptr, will it be a problem?
cc @bkietz

@pitrou
Copy link
Member

pitrou commented Apr 27, 2021

I'll take a look at the pickling issue.

@cyb70289
Copy link
Contributor Author

I'll take a look at the pickling issue.

Cool 👍 . Will update other csv options based on your change. Possibly also the computer kernel options.

cyb70289 and others added 2 commits May 2, 2021 07:46
arrow::csv::ConvertOptions has stl container data members which are
not guaranteed to be standard layout type. It causes invalid-offsetof
warning when compiled with clang-12.

This patch changes directly embedded CCSVConvertOptions to unique_ptr
to fix the issue. ReadOptions and WriteOptions are also updated.

ParseOptions is left untouched. It needs to be persistent with pickle.
Pointer data member must be serialized.
@cyb70289 cyb70289 force-pushed the 12240-invalid-offsetof branch 3 times, most recently from bc5ab0d to 7728915 Compare May 2, 2021 10:02
@cyb70289 cyb70289 force-pushed the 12240-invalid-offsetof branch from 7728915 to fe4cc2b Compare May 2, 2021 14:02
@cyb70289 cyb70289 marked this pull request as ready for review May 3, 2021 14:14
@cyb70289 cyb70289 requested a review from pitrou May 3, 2021 14:15
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, thank you @cyb70289

@pitrou pitrou closed this in 78e993b May 3, 2021
@cyb70289 cyb70289 deleted the 12240-invalid-offsetof branch May 3, 2021 14:26
DileepSrigiri pushed a commit to DileepSrigiri/arrow that referenced this pull request May 6, 2021
arrow::csv::ConvertOptions has stl container data members which are
not guaranteed to be standard layout type. It causes invalid-offsetof
warning when compiled with clang-12.

This patch changes directly embedded CCSVConvertOptions to unique_ptr
to fix the issue. Parse and Read options are also updated.

Closes apache#10154 from cyb70289/12240-invalid-offsetof

Lead-authored-by: Yibo Cai <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 10, 2021
arrow::csv::ConvertOptions has stl container data members which are
not guaranteed to be standard layout type. It causes invalid-offsetof
warning when compiled with clang-12.

This patch changes directly embedded CCSVConvertOptions to unique_ptr
to fix the issue. Parse and Read options are also updated.

Closes apache#10154 from cyb70289/12240-invalid-offsetof

Lead-authored-by: Yibo Cai <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
arrow::csv::ConvertOptions has stl container data members which are
not guaranteed to be standard layout type. It causes invalid-offsetof
warning when compiled with clang-12.

This patch changes directly embedded CCSVConvertOptions to unique_ptr
to fix the issue. Parse and Read options are also updated.

Closes apache#10154 from cyb70289/12240-invalid-offsetof

Lead-authored-by: Yibo Cai <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants