ARROW-16033: [C++] Pass schema to consuming sink node #12721

westonpace · 2022-03-25T23:29:25Z

No description provided.

github-actions · 2022-03-25T23:29:43Z

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

github-actions · 2022-03-25T23:29:44Z

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

westonpace · 2022-03-25T23:31:43Z

There is an entirely different implementation we could take (pass the schema in an init method before anything runs):

class ARROW_EXPORT SinkNodeConsumer {
 public:
  virtual ~SinkNodeConsumer() = default;
  /// \brief Initialize the consumer with the schema of the batches that will arrive
  virtual Status Init(const std::shared_ptr<Schema>& schema) = 0;
  /// \brief Consume a batch of data
  virtual Status Consume(ExecBatch batch) = 0;
  /// \brief Signal to the consumer that the last batch has been delivered
  ///
  /// The returned future should only finish when all outstanding tasks have completed
  virtual Future<> Finish() = 0;
};

Pros:

More obvious that the schema will never change
Cons:
More methods that have to be implemented

Opinions? @lidavidm @vibhatha @paleolimbot

lidavidm · 2022-03-25T23:35:06Z

I would probably prefer Init/Consume/Finish, we could default-implement Init if it's a concern

paleolimbot · 2022-03-26T00:33:13Z

Taking this with the grain of salt that I first heard of a SinkNodeConsumer a few weeks ago, passing the schema in virtual Status Init(const std::shared_ptr<Schema>& schema) = 0; makes the most sense to me (it lines up with the C Stream interface's approach too...one schema, many batches).

vibhatha · 2022-03-26T00:42:03Z

This one is cleaner. I also prefer this method for consuming_sink. One question, does this take care of ‘SinkNode’ and ‘OrderBySink’ node too? We may also need to know what’s the schema of the batches we are going to get out of a given sink, am I right? I am merely considering a general case about sink nodes.

vibhatha · 2022-03-26T00:45:12Z

Another Advantage is we may be solving this issue too. https://issues.apache.org/jira/browse/ARROW-15297

@westonpace @lidavidm what do you think?

westonpace · 2022-03-26T01:43:42Z

One question, does this take care of ‘SinkNode’ and ‘OrderBySink’ node too?

It doesn't currently fix SinkNode or OrderBySinkNode. We could add an init-style schema callback to the options objects for those types. I'll try it out.

Another Advantage is we may be solving this issue too. https://issues.apache.org/jira/browse/ARROW-15297

Almost, looks like the python test is failing that was failing on that issue too. I think we can move towards WriteNodeOptions taking either an optional schema (which would replace the one retrieved from the node, we could add a check to make sure they are compatible in the init step) or just a dictionary of metadata that we append to the schema in the init step.

vibhatha · 2022-03-26T02:16:40Z

Looks like, if we can address these issues, this PR can cover these grounds.
@westonpace Thanks a lot for taking your time on this. I am +1 for this modification.
Shall we also add a few test cases to show the functionality?

westonpace · 2022-03-29T02:06:55Z

I would probably prefer Init/Consume/Finish, we could default-implement Init if it's a concern

I've updated this PR to use an Init method. I did not default-implement as I'm not sure any meaningful consumer can do much without the schema.

I could add a default implementation that actually stored the schema (in a protected variable) but I kind of prefer pure-virtual interfaces for the public API, though maybe that is just my Java/C# background. Happy to change if anyone has any strong opinion on the matter.

Another Advantage is we may be solving this issue too. https://issues.apache.org/jira/browse/ARROW-15297

This changes the schema to an (optional) custom_metadata which I think means we do indeed solve ARROW-15297.

Shall we also add a few test cases to show the functionality?

@vibhatha

I've added a test to make sure we test the failure path (i.e. Init returns a non-ok status). I'm not sure what other test cases you had in mind.

lidavidm · 2022-03-29T12:05:43Z

cpp/src/arrow/compute/exec/options.h

@@ -304,10 +310,9 @@ class ARROW_EXPORT TableSinkNodeOptions : public ExecNodeOptions {
 public:
  TableSinkNodeOptions(std::shared_ptr<Table>* output_table,
                       std::shared_ptr<Schema> output_schema)


We can remove this second argument now right?

Good catch. I've removed it.

vibhatha

Looks good to me @westonpace. This will help in exposing Substrait plan very neatly. I cannot think of any additional test cases since, this is basically provides a method to extract the schema in the user code.

westonpace · 2022-03-30T03:35:19Z

It seems an R change managed to sneak in that exposes a little dilemma. The output schema is including the augmented fields, even though the Substrait plan didn't call for them.

I'm still processing how we want to handle this but I'm thinking right now we just make sure not to include augmented fields when running against a Substrait plan.

paleolimbot · 2022-03-30T17:13:45Z

I did notice there were unexpected fields (although because there was no schema I had no idea what they were!). At least the ability to turn them off would be nice (or else we're still stuck having to calculate the column names in advance to know what columns not to pass on to the user).

…s the schema along with each batch

…g schema to consume. Added back in support for custom metadata which is needed for python tests to pass.

westonpace · 2022-04-05T18:03:31Z

I added an explicit selection step to drop those columns. I think the proper fix will have to be done in a follow-up. I added a comment to the R test to this effect. @paleolimbot mind taking a quick look for sanity's sake? Then I will merge.

aad8039

paleolimbot

This is much easier than what I was doing before...thank you!

ursabot · 2022-04-06T14:31:09Z

Benchmark runs are scheduled for baseline = 9e08c50 and contender = 45a97e1. 45a97e1 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.59% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.6% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/451| 45a97e13 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/436| 45a97e13 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/437| 45a97e13 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/446| 45a97e13 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/450| 9e08c503 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/435| 9e08c503 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/436| 9e08c503 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/445| 9e08c503 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

…ROW_R_WITH_ENGINE After ARROW-16033 (#12721) we get this compiler warning when compiling with `ARROW_R_WITH_ENGINE`: ``` compute-exec.cpp:304:17: warning: 'Init' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] arrow::Status Init(const std::shared_ptr<arrow::Schema>& schema) { ^ /Users/deweydunnington/.r-arrow-dev-build/dist/include/arrow/compute/exec/options.h:153:18: note: overridden virtual function is here virtual Status Init(const std::shared_ptr<Schema>& schema) = 0; ^ 1 warning generated. ``` This PR just adds the requisite `override`. Closes #12823 from paleolimbot/r-minor-override Authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

github-actions bot added the Component: C++ label Mar 25, 2022

westonpace mentioned this pull request Mar 25, 2022

Minor: [C++] Fix sink schema #12715

Closed

vibhatha mentioned this pull request Mar 28, 2022

ARROW-15779: [Python] Create python bindings for Substrait consumer #12672

Closed

5 tasks

lidavidm approved these changes Mar 29, 2022

View reviewed changes

vibhatha approved these changes Mar 29, 2022

View reviewed changes

vibhatha mentioned this pull request Mar 30, 2022

ARROW-15515: [C++] Update ExecPlan example code and documentation with new options #12689

Closed

westonpace force-pushed the feature/ARROW-16033--pass-schema-to-sink-consumer branch from 65d3a08 to 5f04ffd Compare March 30, 2022 03:31

github-actions bot added the Component: R label Mar 30, 2022

westonpace added 7 commits April 4, 2022 14:04

ARROW-16033: Modifies the sink node consumer interface so that we pas…

88b046e

…s the schema along with each batch

ARROW-16033: Moved to a Init/Consume/Finish model instead of supplyin…

e8c6ce0

…g schema to consume. Added back in support for custom metadata which is needed for python tests to pass.

ARROW-16033: Removed unused schema argument from TableSinkNodeOptions

7bef054

ARROW-16033: Lint

da90068

ARROW-16033: Forgot to update example

a0b1f46

ARROW-16033: Update R's consuming sink to the new model

9ad85ce

ARROW-16033: Add workaround for ARROW-15585 exposed by this change

aad8039

westonpace force-pushed the feature/ARROW-16033--pass-schema-to-sink-consumer branch from 5f04ffd to aad8039 Compare April 5, 2022 00:58

paleolimbot approved these changes Apr 5, 2022

View reviewed changes

westonpace closed this in 45a97e1 Apr 5, 2022

paleolimbot mentioned this pull request Apr 7, 2022

MINOR: [R] Fix compiler warning/CMD check NOTE when compiling with ARROW_R_WITH_ENGINE #12823

Closed

ARROW-16033: [C++] Pass schema to consuming sink node #12721

ARROW-16033: [C++] Pass schema to consuming sink node #12721

Uh oh!

Conversation

westonpace commented Mar 25, 2022

Uh oh!

github-actions bot commented Mar 25, 2022

Uh oh!

github-actions bot commented Mar 25, 2022

Uh oh!

westonpace commented Mar 25, 2022

Uh oh!

lidavidm commented Mar 25, 2022

Uh oh!

paleolimbot commented Mar 26, 2022

Uh oh!

vibhatha commented Mar 26, 2022

Uh oh!

vibhatha commented Mar 26, 2022

Uh oh!

westonpace commented Mar 26, 2022

Uh oh!

vibhatha commented Mar 26, 2022

Uh oh!

westonpace commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

westonpace Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

vibhatha left a comment

Choose a reason for hiding this comment

Uh oh!

westonpace commented Mar 30, 2022

Uh oh!

paleolimbot commented Mar 30, 2022

Uh oh!

westonpace commented Apr 5, 2022

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

westonpace commented Mar 29, 2022 •

edited

Loading