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-15745: [Java] Deprecate redundant iterable of ScanTask #14168

Merged
merged 5 commits into from
Sep 21, 2022

Conversation

davisusanibar
Copy link
Contributor

Deprecate redundant iterable ScanTask since there are no more ScanTasks on the C++ side

@github-actions
Copy link

@@ -68,6 +68,15 @@ ArrowReader execute() {
}

@Override
public NativeScanTask scanTask() {
Copy link
Member

Choose a reason for hiding this comment

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

We just shouldn't have ScanTask at all, there is no such thing on the C++ side anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me delete that, this scan() method is exposed to the client and will cause break compatibilities

Copy link
Member

Choose a reason for hiding this comment

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

The deprecation makes sense to me, I think we should consider what the new interface is, though. I would think something like ArrowReader scanBatchesUnordered().

Copy link
Member

Choose a reason for hiding this comment

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

That would mirror the C++ API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

*
* @return a {@link ArrowReader}.
*/
ArrowReader scanBatchesUnordered();
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, it looks like it's actually scanBatches in the implementation:

ARROW_ASSIGN_OR_RAISE(auto batch_itr, scanner->ScanBatches());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated

@lidavidm lidavidm merged commit fc98c95 into apache:master Sep 21, 2022
@ursabot
Copy link

ursabot commented Sep 21, 2022

Benchmark runs are scheduled for baseline = d7258aa and contender = fc98c95. fc98c95 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.26% ⬆️0.44%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.6% ⬆️0.36%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] fc98c95b ec2-t3-xlarge-us-east-2
[Finished] fc98c95b test-mac-arm
[Failed] fc98c95b ursa-i9-9960x
[Finished] fc98c95b ursa-thinkcentre-m75q
[Finished] d7258aa1 ec2-t3-xlarge-us-east-2
[Failed] d7258aa1 test-mac-arm
[Failed] d7258aa1 ursa-i9-9960x
[Finished] d7258aa1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
…14168)

Deprecate redundant iterable ScanTask since there are no more ScanTasks on the C++ side

Authored-by: david dali susanibar arce <[email protected]>
Signed-off-by: David Li <[email protected]>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
…14168)

Deprecate redundant iterable ScanTask since there are no more ScanTasks on the C++ side

Authored-by: david dali susanibar arce <[email protected]>
Signed-off-by: David Li <[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.

3 participants