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

refactor(functions): converting limit to use arrow arrays #570

Merged
merged 1 commit into from
Jan 8, 2019
Merged

refactor(functions): converting limit to use arrow arrays #570

merged 1 commit into from
Jan 8, 2019

Conversation

affo
Copy link
Contributor

@affo affo commented Dec 19, 2018

Fixes #508

Done checklist

  • docs/SPEC.md updated
  • Test cases written

@affo affo requested a review from jsternberg December 19, 2018 18:15
affo
affo commented Dec 19, 2018
View reviewed changes
functions/transformations/limit.go Outdated
@@ -155,7 +155,7 @@ func (t *limitTransformation) Process(id execute.DatasetID, tbl flux.Table) erro
}
n -= count
lcr := sliceColReader{
ColReader: cr,
ColReader: arrow.ColReader(cr),
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 seems wrong to me 😅

Is there any other method that allows to reslice an arrow array and append it as column?
Should I stick with converting it manually to a slice?

Copy link
Contributor

Choose a reason for hiding this comment

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

The arrow packages has ways to create slices from arrow arrays https://godoc.org/github.com/apache/arrow/go/arrow#example-package--Float64Slice

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a similar comment here: #566 (review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should learn to browse godocs 😅 Thank you!

@affo
Copy link
Contributor Author

affo commented Dec 20, 2018 •
edited
Loading

Must be merged after #575

Is there a better way to specify dependencies between PRs?
At the moment I checked out from Jonathan's branch for PR 575 because I needed his implementation of AppendColArrow. Thank you

@affo affo changed the title feat(functions): converting limit to use arrow arrays refactor(functions): converting limit to use arrow arrays Jan 3, 2019
@affo affo mentioned this pull request Jan 7, 2019
Merged
2 tasks
@affo
Copy link
Contributor Author

affo commented Jan 8, 2019

@jsternberg this PR now uses adam's new testing functions.

jpacik
jpacik approved these changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@jpacik jpacik left a comment

Choose a reason for hiding this comment

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

#639 has been merged so it is safe to slice arrow arrays.

@affo affo merged commit 9e7b58f into master Jan 8, 2019
@affo affo deleted the la-arrow-limit branch January 8, 2019 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanielc nathanielc nathanielc left review comments

@jpacik jpacik jpacik approved these changes

@jsternberg jsternberg Awaiting requested review from jsternberg

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

Successfully merging this pull request may close these issues.
4 participants
@affo @jsternberg @nathanielc @jpacik