Skip to content

ARROW-16700: [C++][R][Datasets] aggregates on partitioning columns - #13518

Merged
westonpace merged 9 commits into
apache:masterfrom
jvanstraten:ARROW-16700-aggregates-on-partitioning-columns-2
Jul 22, 2022
Merged

ARROW-16700: [C++][R][Datasets] aggregates on partitioning columns#13518
westonpace merged 9 commits into
apache:masterfrom
jvanstraten:ARROW-16700-aggregates-on-partitioning-columns-2

Conversation

@jvanstraten

Copy link
Copy Markdown
Contributor

This updates the Scanner node such that it will use the guarantee expression to fill out columns missing from the dataset but guaranteed to be some constant with appropriate scalars, rather than just inserting a null placeholder column. In case both are available, the dataset constructor prefers using the scalar from the guarantee expression over the actual data, since the latter would probably be an array that unnecessarily repeats the constant value.

This is the other part of what was uncovered while analyzing ARROW-16700, the more direct cause being a duplicate of ARROW-16904 (see also #13509 for my fix for that).

@github-actions

Copy link
Copy Markdown

@westonpacewestonpace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good. One suggestion for the test.

Comment threadcpp/src/arrow/dataset/scanner_test.cc Outdated
@drin
drinforce-pushed the ARROW-16700-aggregates-on-partitioning-columns-2 branch from 706f409 to 6592ad2CompareJuly 14, 2022 17:10
@drin

drin commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

rebased branch

drin added 2 commits July 15, 2022 15:44
The major part of this commit is to add columns to ExecBatches from the
guarantees vector instead of taking a lambda. The lambda can be
repetitive and a source of error. This still only adds the guarantees to
ExecBatches and not the Dataset
whitespace for test data was demolished by clang-format, this just
restores the whitespace that made it human readable
@drin

drin commented Jul 15, 2022

Copy link
Copy Markdown
Contributor

I confirmed that the C++ unit tests validate the fix provided in this PR. Still working on an R test that correctly validates the fix.

auto guarantee = partial.fragment.value->partition_expression();
ARROW_ASSIGN_OR_RAISE(
util::optional<compute::ExecBatch> batch,
compute::MakeExecBatch(*scan_options->dataset_schema,
partial.record_batch.value, guarantee));

@westonpace

Copy link
Copy Markdown
Member

I'm sorry but I believe I have sent you on a wild goose chase. I had suggested an R test but, upon re-reading the JIRA, realize that I had forgotten this is not reproducible from R (because it always inserts a project node prior to an aggregate node) and so no R test is needed.

@westonpacewestonpace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor suggestions on the unit test and I think this is good to go.

Comment threadcpp/src/arrow/dataset/scanner_test.cc Outdated
Comment threadcpp/src/arrow/dataset/scanner_test.cc Outdated
drinand others added 4 commits July 18, 2022 10:33
minor style change
Co-authored-by: Weston Pace <weston.pace@gmail.com>
changing use of ARROW_WARN_NOT_OK
A more appropriate macro was suggested, EXPECT_OK_AND_ASSIGN
Co-authored-by: Weston Pace <weston.pace@gmail.com>
changed variable name and method invocation because EXPECT_OK_AND_ASSIGN
unwraps the result whereas ARROW_RETURN_NOT_OK did not

@westonpacewestonpace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks both of you for taking care of this.

Comment threadcpp/src/arrow/dataset/scanner_test.cc Outdated
@westonpace
westonpace merged commit e0ccfa1 into apache:masterJul 22, 2022
@drin
drin deleted the ARROW-16700-aggregates-on-partitioning-columns-2 branch July 22, 2022 18:20
@ursabot

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = b3ce0fa and contender = e0ccfa1. e0ccfa1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.59% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.63% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.29% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] e0ccfa11 ec2-t3-xlarge-us-east-2
[Failed] e0ccfa11 test-mac-arm
[Failed] e0ccfa11 ursa-i9-9960x
[Finished] e0ccfa11 ursa-thinkcentre-m75q
[Failed] b3ce0fa7 ec2-t3-xlarge-us-east-2
[Failed] b3ce0fa7 test-mac-arm
[Failed] b3ce0fa7 ursa-i9-9960x
[Finished] b3ce0fa7 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

kou pushed a commit that referenced this pull request Feb 20, 2023
Sign up for freeto 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.

4 participants

@jvanstraten@drin@westonpace@ursabot