Skip to content

ARROW-6393: [C++] Add EqualOptions support in SparseTensor::Equals#6443

Closed
mrkn wants to merge 10 commits intoapache:masterfrom
mrkn:ARROW-6393
Closed

ARROW-6393: [C++] Add EqualOptions support in SparseTensor::Equals#6443
mrkn wants to merge 10 commits intoapache:masterfrom
mrkn:ARROW-6393

Conversation

@mrkn
Copy link
Copy Markdown
Member

@mrkn mrkn commented Feb 18, 2020

No description provided.

@github-actions
Copy link
Copy Markdown

@mrkn mrkn force-pushed the ARROW-6393 branch 3 times, most recently from d5ea290 to 9ba7b09 Compare February 20, 2020 06:11
@mrkn mrkn marked this pull request as ready for review February 20, 2020 06:12
@mrkn mrkn force-pushed the ARROW-6393 branch 3 times, most recently from 3e7a709 to e9ddb8f Compare February 21, 2020 01:16
@mrkn
Copy link
Copy Markdown
Member Author

mrkn commented Feb 21, 2020

This failure seems not related to this pull-request.

@mrkn mrkn requested a review from pitrou February 21, 2020 06:38
@mrkn
Copy link
Copy Markdown
Member Author

mrkn commented Feb 21, 2020

@pitrou Could you review this?
@rok Could you review this, especially the part related to SparseCSFTensor?

@mrkn mrkn changed the title ARROW-6393: WIP: [C++] Add EqualOptions support in SparseTensor::Equals ARROW-6393: [C++] Add EqualOptions support in SparseTensor::Equals Feb 21, 2020
@rok
Copy link
Copy Markdown
Member

rok commented Feb 21, 2020

@mrkn will do :)

Copy link
Copy Markdown
Member

@rok rok left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm just wondering about Float to Floating name change.

Copy link
Copy Markdown
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Looks good on the principle, just some minor comments.

@mrkn mrkn requested a review from pitrou February 26, 2020 06:58
}

template <typename DataType>
bool FloatSparseTensorDataEquals(const typename DataType::c_type* left_data,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I renamed Floating to Float here and other places in this pull-request.
But I didn't rename Floating outside of this pull-request, such as BaseFloatingEquals at the top of this file.

Copy link
Copy Markdown
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1. Thank you @mrkn !

@pitrou pitrou closed this in 76db492 Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants