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.

Sign up for GitHub

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

Jump to bottom

ARROW-7886: [C++][Dataset][Python][R] Consolidate Source and Dataset classes #6470

Closed
wants to merge 12 commits into from
Closed

ARROW-7886: [C++][Dataset][Python][R] Consolidate Source and Dataset classes #6470

wants to merge 12 commits into from

Conversation

bkietz
Copy link
Member

@bkietz bkietz commented Feb 20, 2020

No description provided.

@github-actions
Copy link

https://issues.apache.org/jira/browse/ARROW-7886

jorisvandenbossche
jorisvandenbossche reviewed Feb 21, 2020
View reviewed changes
python/pyarrow/_dataset.pyx Outdated Show resolved Hide resolved
python/pyarrow/dataset.py Outdated Show resolved Hide resolved
@bkietz bkietz marked this pull request as ready for review February 22, 2020 18:54
@bkietz bkietz force-pushed the 7886-Dataset-Consolidate-Sourc branch from 283f85d to c3da817 Compare February 24, 2020 21:14
@kszucs kszucs self-requested a review February 24, 2020 21:52
nealrichardson
nealrichardson reviewed Feb 25, 2020
View reviewed changes
Copy link
Contributor

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

I pushed some edits to the R bindings. I'm still not 100% satisfied but think this is probably a step in the right direction if others are on board. I can revisit my concerns later when I have time--feel free to merge when y'all are satisfied.

kszucs
kszucs reviewed Feb 26, 2020
View reviewed changes
python/pyarrow/dataset.py Outdated Show resolved Hide resolved
kszucs
kszucs requested changes Feb 26, 2020
View reviewed changes
Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

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

Reviewed C++ and Python.

C++: please fix the FileSource renames
Python: there are couple of minor issues but I have them addressed in https://github.com/bkietz/arrow/pull/1/files

@bkietz bkietz force-pushed the 7886-Dataset-Consolidate-Sourc branch from 881ee85 to 11bf0c9 Compare February 26, 2020 14:54
kszucs
kszucs approved these changes Feb 27, 2020
View reviewed changes
Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

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

LGTM

@bkietz bkietz closed this in 3d81e4e Feb 27, 2020
@nealrichardson nealrichardson mentioned this pull request Feb 28, 2020
Closed
nealrichardson added a commit that referenced this pull request Feb 28, 2020
@nealrichardson
ARROW-7962: [R][Dataset] Followup to "Consolidate Source and Dataset …
5096803 
…classes"

This commit was pushed to #6470 along with my "approval" review, but a force push overwrote it.

cc @bkietz

Closes #6502 from nealrichardson/dataset-source-followup and squashes the following commits:

c1c5e75 <Neal Richardson> Some R edits

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
@bkietz bkietz deleted the 7886-Dataset-Consolidate-Sourc branch February 25, 2021 16:35
@asfimport asfimport mentioned this pull request Apr 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

@nealrichardson nealrichardson nealrichardson left review comments

@kszucs kszucs kszucs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bkietz @kszucs @jorisvandenbossche @nealrichardson