Skip to content

ARROW-16525: [C++] Tee node not properly marking node finished - #13117

Closed
westonpace wants to merge 4 commits into
apache:masterfrom
westonpace:feature/ARROW-16525--tee-node-not-marking-finished
Closed

ARROW-16525: [C++] Tee node not properly marking node finished#13117
westonpace wants to merge 4 commits into
apache:masterfrom
westonpace:feature/ARROW-16525--tee-node-not-marking-finished

Conversation

@westonpace

@westonpacewestonpace commented May 11, 2022

Copy link
Copy Markdown
Member

No description provided.

@westonpace
ARROW-16525: Added tests for the tee node and write node. Fix bug in …
f0f4d2f
…tee node where it was not properly marking itself finished
@github-actions

github-actionsBot commented May 11, 2022

Copy link
Copy Markdown

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

@github-actions

github-actionsBot commented May 11, 2022

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@westonpace

westonpace commented May 11, 2022

Copy link
Copy Markdown
MemberAuthor

CC @rtpsw please take a look

@rtpsw

rtpsw commented May 11, 2022

Copy link
Copy Markdown
Contributor

Thanks for doing this, @westonpace !

rtpsw
rtpsw suggested changes May 11, 2022
View reviewed changes
Comment threadcpp/src/arrow/dataset/file_base.cc Outdated
Comment threadcpp/src/arrow/dataset/file_test.cc
Comment threadcpp/src/arrow/dataset/file_test.cc Outdated
Comment threadcpp/src/arrow/dataset/file_test.cc Outdated
@rtpsw

rtpsw commented May 11, 2022

Copy link
Copy Markdown
Contributor

LGTM. Just a few small suggestions.

@westonpace
ARROW-16525: Addressed review feedback. Added some helper functions f…
0d657b3
…or working with arrow::compute::Declaration. Minor cleanup of some error messages
rtpsw
rtpsw approved these changes May 11, 2022
View reviewed changes
Comment threadcpp/src/arrow/dataset/file_test.cc Outdated
Comment threadcpp/src/arrow/dataset/file_base.cc
rtpsw
rtpsw suggested changes May 11, 2022
View reviewed changes

@rtpswrtpsw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Relevant tests are failing. Need to take a look.

@westonpace
ARROW-16525: My changes to Sequence were a bit to aggressive with err…
adc75d3
…or checking. The first node should be allowed to have multiple inputs since we don't need to recurse any further
@rtpsw

rtpsw commented May 12, 2022

Copy link
Copy Markdown
Contributor

Current failures don't look relevant to this PR.

bkietz
bkietz reviewed May 12, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/exec_plan.cc Outdated
Comment threadcpp/src/arrow/compute/exec/exec_plan.cc Outdated
@westonpace
westonpace requested a review from bkietzMay 13, 2022 21:27
@ursabot

ursabot commented May 16, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 940ab93 and contender = d040fb5. d040fb5 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
[Finished ⬇️0.43% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.28% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d040fb56 ec2-t3-xlarge-us-east-2
[Finished] d040fb56 test-mac-arm
[Finished] d040fb56 ursa-i9-9960x
[Finished] d040fb56 ursa-thinkcentre-m75q
[Finished] 940ab93a ec2-t3-xlarge-us-east-2
[Finished] 940ab93a test-mac-arm
[Finished] 940ab93a ursa-i9-9960x
[Finished] 940ab93a 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

@rtpswrtpsw mentioned this pull request Jun 2, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkietzbkietzbkietz approved these changes
+1 more reviewer
@rtpswrtpswrtpsw approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@westonpace@rtpsw@ursabot@bkietz