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

[Python] Append columns #318

Merged
merged 5 commits into from
Nov 19, 2022
Merged

[Python] Append columns #318

merged 5 commits into from
Nov 19, 2022

Conversation

eddyxu
Copy link
Contributor

@eddyxu eddyxu commented Nov 18, 2022 •
edited
Loading

Provide equivalent of C++ API (#299) in Python.

@eddyxu eddyxu changed the base branch from main to lei/update_col November 18, 2022 20:47
Base automatically changed from lei/update_col to main November 18, 2022 21:32
@eddyxu eddyxu marked this pull request as ready for review November 18, 2022 22:25
@eddyxu eddyxu self-assigned this Nov 18, 2022
changhiskhan
changhiskhan approved these changes Nov 18, 2022
View reviewed changes
Copy link
Contributor

@changhiskhan changhiskhan left a comment

Choose a reason for hiding this comment

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

I think docs need to be clearer that the func is chunks. I see there's there's a separate issue for projection that works. Also let's add support for user to compute new column externally and pass it in (if they need to say normalize a column)?

python/lance/_lib.pyx
)
return FileSystemDataset.wrap(static_pointer_cast[CDataset, CLanceDataset](new_dataset))

def __iter__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need both iter and next?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean next() or __next__() here?

python/lance/_lib.pyx
@@ -220,6 +279,28 @@ cdef class FileSystemDataset(Dataset):
c_version = GetResultValue(self.lance_dataset.latest_version())
return _dataset_version_to_json(c_version)

def append_column(self, field: Field, func: Callable[[pyarrow.Table], pyarrow.Array]) -> FileSystemDataset:
Copy link
Contributor

Choose a reason for hiding this comment

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

User needs to know that func input is just a chunk of data. Eg they can't do something like normalization in this function unless they explicitly compute the mean / std globally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually a good argument of whether func should take in a RecordBatch instead of Table

@eddyxu
Copy link
Contributor Author

eddyxu commented Nov 18, 2022

Also let's add support for user to compute new column externally and pass it in (if they need to say normalize a column)?

Filed #320. As discussed offline, the main challenging is make the layout aligned with the existing data with the new column. Need some designs.

@eddyxu eddyxu merged commit de5ed73 into main Nov 19, 2022
@eddyxu eddyxu deleted the lei/py_append_cols branch November 19, 2022 00:05
@eddyxu eddyxu mentioned this pull request Nov 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@changhiskhan changhiskhan changhiskhan approved these changes

Assignees

@eddyxu eddyxu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@eddyxu @changhiskhan