Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-605: [C++] Refactor IPC adapter code into generic ArrayLoader class. Add Date32Type #365

Closed
wants to merge 3 commits into from
Closed

ARROW-605: [C++] Refactor IPC adapter code into generic ArrayLoader class. Add Date32Type #365

wants to merge 3 commits into from

Conversation

wesm
Copy link
Member

@wesm wesm commented Mar 8, 2017

These are various changes introduced to support the Feather merge in ARROW-452 #361

@wesm
Refactor IPC adapter code into generic ArrayLoader class. Add Date32Type
997b7a2 
Change-Id: I75eb30bdc12723e29f0c8e2c7c92c296dfbe95a7
@wesm wesm mentioned this pull request Mar 9, 2017
Closed
tebeka
tebeka reviewed Mar 9, 2017
View reviewed changes
cpp/src/arrow/type.h
explicit TimestampType(TimeUnit unit = TimeUnit::MILLI)
: FixedWidthType(Type::TIMESTAMP), unit(unit) {}

explicit TimestampType(const std::string& timezone, TimeUnit unit = TimeUnit::MILLI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't timezone default to "UTC"? Also maybe it should be the 2nd parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

We haven't resolved any questions about storing time zones in the IPC metadata (https://github.com/apache/arrow/blob/master/format/Message.fbs), so I did the bare minimum here to support the pandas and R use cases, where they have the notion of tz-naive and tz-aware timestamps. So I don't want to invest any more energy in it until we have a broader discussion about time zones

Copy link
Member

@xhochy xhochy Mar 9, 2017 •
edited

Choose a reason for hiding this comment

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

Empty string thus indicates a tz-naive timestamp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, wasn't sure what else to use for a "null" value at the moment

@wesm
Copy link
Member Author

wesm commented Mar 9, 2017

fixing compiler warnings

wesm added 2 commits March 9, 2017 08:05
@wesm
fix compiler warning
344e6b1 
Change-Id: I6dc42fd7b21d910dd9a8444a02048e4f08ec86cf
@wesm
Revert Array::type_id to type_enum since Parquet uses this API
bc22872 
Change-Id: I77db11a9c324d4b9055db7490fd3947e651a7d85
@wesm
Copy link
Member Author

wesm commented Mar 9, 2017

green build: https://travis-ci.org/wesm/arrow/builds/209347029

xhochy
xhochy approved these changes Mar 9, 2017
View reviewed changes
Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

Except the two questions, this looks good. If they both can be answered with yes, feel free to merge.

cpp/src/arrow/type.h
DATE,

// int32_t days since the UNIX epoch
DATE32,
Copy link
Member

Choose a reason for hiding this comment

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

Is the ordering of the types important yet, i.e. do we match the enums anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, and I don't think we should guarantee ABI stability of the enum values yet anyhow (at some point we will want to)

cpp/src/arrow/type.h
explicit TimestampType(TimeUnit unit = TimeUnit::MILLI)
: FixedWidthType(Type::TIMESTAMP), unit(unit) {}

explicit TimestampType(const std::string& timezone, TimeUnit unit = TimeUnit::MILLI)
Copy link
Member

@xhochy xhochy Mar 9, 2017 •
edited

Choose a reason for hiding this comment

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

Empty string thus indicates a tz-naive timestamp?

@wesm
Copy link
Member Author

wesm commented Mar 9, 2017

thanks

@asfgit asfgit closed this in 6b3ae2a Mar 9, 2017
@wesm wesm deleted the array-loader branch March 9, 2017 21:59
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 2, 2018
@xhochy @wesm
PARQUET-1040: Add missing writer methods
1beab1a 
Author: Korn, Uwe <Uwe.Korn@blue-yonder.com>

Closes apache#365 from xhochy/PARQUET-1040 and squashes the following commits:

ef359ef [Korn, Uwe] PARQUET-1040: Add missing writer methods
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 4, 2018
@xhochy @wesm
PARQUET-1040: Add missing writer methods
a84ba3f 
Author: Korn, Uwe <Uwe.Korn@blue-yonder.com>

Closes apache#365 from xhochy/PARQUET-1040 and squashes the following commits:

ef359ef [Korn, Uwe] PARQUET-1040: Add missing writer methods

Change-Id: I0d7a5b227b64e85c42106e37bb902cc3bbb15e85
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 6, 2018
@xhochy @wesm
PARQUET-1040: Add missing writer methods
c2ca4ad 
Author: Korn, Uwe <Uwe.Korn@blue-yonder.com>

Closes apache#365 from xhochy/PARQUET-1040 and squashes the following commits:

ef359ef [Korn, Uwe] PARQUET-1040: Add missing writer methods

Change-Id: I0d7a5b227b64e85c42106e37bb902cc3bbb15e85
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 7, 2018
@xhochy @wesm
PARQUET-1040: Add missing writer methods
04bc3d1 
Author: Korn, Uwe <Uwe.Korn@blue-yonder.com>

Closes apache#365 from xhochy/PARQUET-1040 and squashes the following commits:

ef359ef [Korn, Uwe] PARQUET-1040: Add missing writer methods

Change-Id: I0d7a5b227b64e85c42106e37bb902cc3bbb15e85
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 8, 2018
@xhochy @wesm
PARQUET-1040: Add missing writer methods
5374737 
Author: Korn, Uwe <Uwe.Korn@blue-yonder.com>

Closes apache#365 from xhochy/PARQUET-1040 and squashes the following commits:

ef359ef [Korn, Uwe] PARQUET-1040: Add missing writer methods

Change-Id: I0d7a5b227b64e85c42106e37bb902cc3bbb15e85
@toddfarmer toddfarmer mentioned this pull request Apr 16, 2017
Closed
@asfimport asfimport mentioned this pull request Apr 16, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tebeka tebeka tebeka left review comments

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

None yet
3 participants
@wesm @xhochy @tebeka