Skip to content

ARROW-786: [Format] In-memory format for 128-bit Decimals, handling of sign bit - #981

Closed
cpcloud wants to merge 22 commits into
apache:masterfrom
cpcloud:decimal-rewrite
Closed

ARROW-786: [Format] In-memory format for 128-bit Decimals, handling of sign bit#981
cpcloud wants to merge 22 commits into
apache:masterfrom
cpcloud:decimal-rewrite

Conversation

@cpcloud

@cpcloudcpcloud commented Aug 20, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor
  • Reimplement Decimal128 types to use the Int128 type as the underlying integer
    representation, adapted from the Apache ORC project's C++ in memory format.
    This enables us to write integration tests and results in an in-memory
    Decimal128 format that is compatible with the Java implementation
  • Additionaly, this PR also fixes Decimal slice comparison and adds related
    regression tests
  • Follow-ups include ARROW-695 (C++ Decimal integration tests), ARROW-696 (JSON
    read/write support for decimals) and ARROW-1238 (Java Decimal integration
    tests).

@wesm

wesm commented Aug 20, 2017

Copy link
Copy Markdown
Member

nice! I will review when I can

@wesm

wesm commented Aug 21, 2017

Copy link
Copy Markdown
Member

If you rebase on master and then fix bugs, you should be able to get to a passing build

wesm
wesm reviewed Aug 22, 2017
View reviewed changes
Comment threadcpp/src/arrow/array.cc Outdated

@wesmwesmAug 22, 2017

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.

Make the default offset 0

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It's set to 0 as the default in the constructor already.

Comment threadcpp/src/arrow/compare.cc Outdated

@wesmwesmAug 22, 2017

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.

Any way to share code with FixedSizeBinaryArray on this?

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Possibly. Let me look into cleaning up DecimalArray even more.

@cpcloudcpcloudAug 24, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was able to do a pretty significant refactoring of DecimalArray and inherit most methods from FixedSizeBinaryArray, including reuse of existing comparison methods for both FixedSizeBinaryArray and DecimalArray.

Comment threadcpp/src/arrow/python/arrow_to_pandas.cc Outdated

@wesmwesmAug 22, 2017

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.

kMaximumPrecision

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

Comment threadcpp/src/arrow/util/CMakeLists.txt Outdated

@wesmwesmAug 22, 2017

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.

does this do anything?

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

removed

Comment threadcpp/src/arrow/util/decimal-test.cc Outdated

@wesmwesmAug 22, 2017

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.

trailing underscores

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

Comment threadcpp/src/arrow/util/int128.cc Outdated

@wesmwesmAug 22, 2017

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.

Divide

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

Comment threadcpp/src/arrow/util/int128.h Outdated

@wesmwesmAug 22, 2017

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.

Negate

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

Comment threadcpp/src/arrow/util/int128.h Outdated

@wesmwesmAug 22, 2017

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.

Divide

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

Comment threadcpp/src/arrow/util/int128.h Outdated

@wesmwesmAug 22, 2017

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.

Add \brief here for doxygen

@wesmwesmAug 22, 2017

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.

Can you use /// style comments for doxygen instead of C-style?

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

Comment threadcpp/src/arrow/util/int128.h Outdated

@wesmwesmAug 22, 2017

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.

\brief, and below

@cpcloudcpcloudAug 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

@wesm

wesm commented Aug 22, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Here's my "Arrow preflight check" that helps with not checking in cpplint or flake8 fails

functionarrow_preflight {
ARROW_PREFLIGHT_DIR=$HOME/code/arrow/cpp/preflight
mkdir -p $ARROW_PREFLIGHT_DIRpushd$ARROW_PREFLIGHT_DIR
arrow_cmake
ninja format
ninja lint
popdpushd$HOME/code/arrow/python
flake8 pyarrow
flake8 --config=.flake8.cython pyarrow
popd
}

@cpcloud
ARROW-786: [Format] In-memory format for 128-bit Decimals, handling o…
349dc58
…f sign bit
@cpcloud
MSVC fix
9af97d8
@cpcloud
kMaximumPrecision
22eda4b
@cpcloud
Remove unnecessary header from CMakeLists.txt
1eab5c4
@cpcloud
Fix naming convention in decimal-test.cc
2930d7b
@cpcloud
Fix doc style in int128.h and add const where possible
38c9b50
@cpcloud
NOLINT for MSVC specific and necessary types
178d3f2
@cpcloud
Formatting
8ecb315
@cpcloud
More const
e4a1792
@cpcloud
Fix more docs
628ce85
@cpcloud
Remove unnecessary args to sto* function calls
8833904
@cpcloud
Cast 18 to matching type size_t for msvc
5716010
@cpcloud
Cast so we have enough space to contain the integer
be220c8
@cpcloud
BUG: Export symbols
b98c894
@cpcloud
BUG: Double export
162eeeb
@cpcloud
Share comparison code
b53d7cd
@cpcloud
Include the correct header for _BitScanReverse
355fb24
@cpcloud
Use CHAR_BIT
1cc43ce
@cpcloud
Use a macro for cases
535f9ff
@cpcloud
Subclass from FixedSizeBinaryArray for code reuse
86db184
@cpcloud
Remove redundant constructor
fe13ef3
@cpcloud
Formatting
53ce04b
wesm
wesm approved these changes Aug 24, 2017
View reviewed changes

@wesmwesm 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, thanks for all this! onward to the integration tests

Comment threadcpp/src/arrow/array.h
private:
void SetData(const std::shared_ptr<internal::ArrayData>& data);
const uint8_t* raw_values_;
const uint8_t* sign_bitmap_data_;

@wesmwesmAug 24, 2017

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.

Nice

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cpcloud@wesm