joosthooz commented on code in PR #33738:
URL: https://github.com/apache/arrow/pull/33738#discussion_r1073684661
##########
cpp/src/arrow/compute/exec/filter_node.cc:
##########
@@ -103,17 +103,9 @@ class FilterNode : public MapNode {
}
void InputReceived(ExecNode* input, ExecBatch batch) override {
- EVENT(span_, "InputReceived", {{"batch.length", batch.length}});
Review Comment:
Does this need a `TraceInputReceived` or `NoteInputReceived`?
##########
cpp/src/arrow/dataset/file_base.cc:
##########
@@ -556,19 +558,8 @@ class TeeNode : public compute::MapNode {
}
void InputReceived(compute::ExecNode* input, compute::ExecBatch batch)
override {
- EVENT(span_, "InputReceived", {{"batch.length", batch.length}});
Review Comment:
Does this need a `TraceInputReceived`?
##########
cpp/src/arrow/compute/exec/sink_node.cc:
##########
@@ -335,19 +327,13 @@ class ConsumingSinkNode : public ExecNode, public
BackpressureControl {
void Resume() override { inputs_[0]->ResumeProducing(this,
++backpressure_counter_); }
void StopProducing() override {
- EVENT(span_, "StopProducing");
if (input_counter_.Cancel()) {
Finish(Status::OK());
}
}
void InputReceived(ExecNode* input, ExecBatch batch) override {
- EVENT(span_, "InputReceived", {{"batch.length", batch.length}});
- util::tracing::Span span;
- START_COMPUTE_SPAN_WITH_PARENT(
- span, span_, "InputReceived",
- {{"node.label", label()}, {"batch.length", batch.length}});
-
+ auto scope = TraceInputReceived(batch);
Review Comment:
If the sink applies backpressure (e.g. the datasetwriter), it does not
result in an event being created on the span as is the case for the normal
SinkNode here
https://github.com/apache/arrow/pull/33738/files#diff-967cff6ef1964402635ac0769dece741ca0ed58bcefb8d669ecfe3ed8371998eR175
Is there a way to do this? For example, we could compare the value of
`backpressure_counter_` before and after calling `Consume()`, but then we
wouldn't be able to know if backpressure was applied or released.
##########
cpp/src/arrow/dataset/file_base.cc:
##########
@@ -533,6 +534,7 @@ class TeeNode : public compute::MapNode {
MapNode::Finish(std::move(finish_st));
return;
}
+ auto scope = TraceFinish();
Review Comment:
Why do we do a `TraceFinish` here and not inside `DatasetWriter::Finish`?
That's the only thing inside this trace and it seems weird to only trace it if
it is inside a TeeNode (e.g. it is missing here
https://github.com/apache/arrow/pull/33738/files#diff-2caf4e9bd3f139e05e55dca80725d8a9c436f5ccf65c76a37cebfa6ee9b36a6aL418)
##########
cpp/src/arrow/compute/exec/project_node.cc:
##########
@@ -96,17 +96,9 @@ class ProjectNode : public MapNode {
}
void InputReceived(ExecNode* input, ExecBatch batch) override {
- EVENT(span_, "InputReceived", {{"batch.length", batch.length}});
Review Comment:
Does this need a `TraceInputReceived`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
