Skip to content

ARROW-13882: [C++] Improve min_max/hash_min_max type support - #11111

Closed
lidavidm wants to merge 5 commits into
apache:masterfrom
lidavidm:arrow-13882
Closed

ARROW-13882: [C++] Improve min_max/hash_min_max type support#11111
lidavidm wants to merge 5 commits into
apache:masterfrom
lidavidm:arrow-13882

Conversation

@lidavidm

Copy link
Copy Markdown
Member

This adds support for non-nested types to both, except for MonthDayNanoInterval (which is not really sortable). hash_min_max additionally lacks binary/string-like types as they require a different approach (I will file a followup).

@lidavidmlidavidm changed the title ARROW-13882: [C++] Improve min_max/hash_min_max type support.ARROW-13882: [C++] Improve min_max/hash_min_max type supportSep 8, 2021
@github-actions

Copy link
Copy Markdown

@lidavidm

Copy link
Copy Markdown
MemberAuthor

In light of the proposal to clarify how DayTime intervals are handled some changes here should probably be reverted.

@lidavidm

Copy link
Copy Markdown
MemberAuthor

(Removed support for DayTimeInterval since it's only partially orderable.)

@pitroupitrou 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.

Very nice, thank you. Just a couple questions and nits.

Comment threadcpp/src/arrow/array/validate.cc Outdated

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.

I'm a bit surprised. This should already be checked by the ValidateArray top-level function, no?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm. Odd, I do remember getting a crash for this reason here. But there is indeed a check:

if (type.id() != Type::EXTENSION) {
if (data.child_data.size() != static_cast<size_t>(type.num_fields())) {
returnStatus::Invalid("Expected ", type.num_fields(),
" child arrays in array "
"of type ",
type.ToString(), ", got ", data.child_data.size());
}
}

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.

It seems this will generate different template specializations for types with the same underlying c_type. Is it desirable?
(note that GetValues is called with the output type independently from the ArrowType parameter)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I changed this and the one below to use PhysicalType. I also replaced GetValues with MakeScalar (since now the static type doesn't match the runtime type).

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.

Should this be else if? Presumably value cannot be bother smaller than the min and greater than the max, and it would spare a string comparison.

Comment threadcpp/src/arrow/compute/kernels/aggregate_basic_internal.h Outdated

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.

Update message here as well?

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.

Ha :-)

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.

Same remark as for the scalar aggregations: this will generate a separate kernel per logical type?

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.

Hmm, I don't see any interval in the type definition above. Is this comment outdated?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I fixed the comments/docs.

Comment threaddocs/source/cpp/compute.rst Outdated

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.

Neither is day-time, right?

Comment threaddocs/source/cpp/compute.rst Outdated

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.

Same question.

@lidavidm
lidavidmforce-pushed the arrow-13882 branch 2 times, most recently from e9087e0 to c05cb96CompareSeptember 15, 2021 15:22

@pitroupitrou 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.

+1, thank you @lidavidm

ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
This adds support for non-nested types to both, except for MonthDayNanoInterval (which is not really sortable). hash_min_max additionally lacks binary/string-like types as they require a different approach (I will file a followup).
Closesapache#11111 from lidavidm/arrow-13882
Lead-authored-by: David Li <li.davidm96@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
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.

2 participants

@lidavidm@pitrou