Skip to content

ARROW-8296: [C++][Dataset] Add IpcFileWriteOptions#8389

Closed
bkietz wants to merge 14 commits into
apache:masterfrom
bkietz:8296-IpcFileWriteOptions
Closed

ARROW-8296: [C++][Dataset] Add IpcFileWriteOptions#8389
bkietz wants to merge 14 commits into
apache:masterfrom
bkietz:8296-IpcFileWriteOptions

Conversation

@bkietz

@bkietz bkietz commented Oct 8, 2020

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actions Bot commented Oct 8, 2020

Copy link
Copy Markdown

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM on the C++ part

@pitrou pitrou requested a review from nealrichardson October 8, 2020 14:32
@bkietz bkietz force-pushed the 8296-IpcFileWriteOptions branch from 638ad10 to a2b587c Compare October 8, 2020 15:51
@bkietz

bkietz commented Oct 9, 2020

Copy link
Copy Markdown
Member Author

@kou not sure how to idiomatically propagate this change to GLib; it seems there's no way to raise an exception when setting a property?

@kou

kou commented Oct 9, 2020

Copy link
Copy Markdown
Member

I'll take a look it and push a fix.

@kou

kou commented Oct 9, 2020

Copy link
Copy Markdown
Member

Done.

+1 on the GLib part.

@bkietz

bkietz commented Oct 9, 2020

Copy link
Copy Markdown
Member Author

Thanks @kou!

@jorisvandenbossche jorisvandenbossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this also be exposed in python's dataset IpcFileWriteOptions ?
(can also look into pushing that)

Comment thread cpp/src/arrow/dataset/file_ipc.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this in theory only be done if we have multiple files / fragments? (like the issue we had for parquet)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That sounds acceptable but I'd like to defer it to a follow up

Comment thread r/configure Outdated
Comment thread r/src/arrow_cpp11.h Outdated
Comment thread r/src/arrow_metadata.h Outdated
Comment thread r/src/recordbatchreader.cpp Outdated
Comment thread r/tests/testthat/test-dataset.R Outdated
Comment thread r/tests/testthat/test-dataset.R Outdated
Comment thread r/R/dataset-write.R Outdated
Comment thread r/R/dataset-format.R Outdated
@nealrichardson

Copy link
Copy Markdown
Member

I'm fixing the R and then will merge

@nealrichardson nealrichardson force-pushed the 8296-IpcFileWriteOptions branch from 0c4d1e9 to 6467dc1 Compare October 9, 2020 22:46

@nealrichardson nealrichardson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1; macOS Python failure is unrelated (failing on master)

@bkietz bkietz deleted the 8296-IpcFileWriteOptions branch February 25, 2021 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants