ARROW-194: C++: Allow read-only memory mapped source #72

jihoonson · 2016-05-07T02:06:45Z

A simple patch to allow read-only mode. A test is also included.

emkornfield · 2016-05-07T16:36:16Z

cpp/src/arrow/ipc/ipc-memory-test.cc

+  rwmmap->Close();
+
+  std::shared_ptr<MemoryMappedSource> rommap;
+  ASSERT_OK(MemoryMappedSource::Open(path, MemorySource::READ_WRITE, &rommap));


did you mean to open this read only?

Yes. Sorry for my mistake.

- Added a routine to check the protection flag before writing - Added a unit test to check the error status for protection mode - Improved failure check for mmap()

jihoonson · 2016-05-08T02:18:45Z

@emkornfield, thank you for your review. I've addressed your comments.

jihoonson · 2016-05-09T01:06:51Z

Broken ci is fixed.

emkornfield · 2016-05-09T01:14:29Z

cpp/src/arrow/ipc/memory.cc

@@ -53,10 +53,12 @@ class MemoryMappedSource::Impl {
  Status Open(const std::string& path, MemorySource::AccessMode mode) {
    if (is_open_) { return Status::IOError("A file is already open"); }

-    path_ = path;


why are you deleting this?

Because this variable is currently not used anywhere. In addition, current implementation expects a file exists in a given path, which in turn means the file is already created in some other parts of the program. So, I think MemoryMappedSource is not responsible for deleting files used for mmap.
In addition, if path is required later, we can add it again easily.

emkornfield · 2016-05-09T01:18:29Z

A couple of more small comments but otherwise LGTM.

jihoonson · 2016-05-09T01:28:32Z

Thanks for your review. I've changed the type of protection flag and left a comment.

wesm · 2016-05-09T02:14:37Z

This also looks good to me -- if you can add a check that trying to write to a read-only file results in error status, that is the last major item.

jihoonson · 2016-05-09T03:25:11Z

@wesm, thank you for your comment.

I've already added a check as seen at Line 142 in memory.cc (https://github.com/apache/arrow/pull/72/files#diff-a061f9b82da1c0b5b7ff8905bd134e4fR142), but do you mean that it is not enough? If so, please let me know how I can improve this.

wesm · 2016-05-09T04:42:39Z

Please add a test case

jihoonson · 2016-05-09T04:54:42Z

Yes, here is the test https://github.com/apache/arrow/pull/72/files#diff-00cfe1c5fd8e391fc6e3440086d0b603R103.

wesm · 2016-05-09T05:15:09Z

@jihoonson oops, sorry about that, I see it now =) I have a difficult time following code reviews in GitHub, hoping we move to Gerrit soon

wesm · 2016-05-09T05:15:19Z

+1, thank you

wesm · 2016-05-09T05:16:52Z

@jihoonson I've added you as a contributor on the Arrow JIRA. welcome to the project and thank you!

jihoonson · 2016-05-09T05:19:40Z

No problem. @emkornfield and @wesm, thank you for your review!

Added support for literals and null for time types.

Added `MemoryAllocator` interface and a default implementation, ensured `MemPool` can use a custom allocator, added `Vector<T>` to replace `std::vector<T>`. Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com> Closes apache#72 from asandryh/PARQUET-542 and squashes the following commits: a740edb [Aliaksei Sandryhaila] Incorporated PR feedback. 6422e0d [Aliaksei Sandryhaila] Added MemoryAllocator interface and default implementation, ensured MemPool can use a custom allocator, added Vector<T> to replace std::vector<T>.

Added support for literals and null for time types.

Added `MemoryAllocator` interface and a default implementation, ensured `MemPool` can use a custom allocator, added `Vector<T>` to replace `std::vector<T>`. Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com> Closes apache#72 from asandryh/PARQUET-542 and squashes the following commits: a740edb [Aliaksei Sandryhaila] Incorporated PR feedback. 6422e0d [Aliaksei Sandryhaila] Added MemoryAllocator interface and default implementation, ensured MemPool can use a custom allocator, added Vector<T> to replace std::vector<T>. Change-Id: I1556f3988516e878893744c2c73688c923892b5e

Added support for literals and null for time types.

* Inital commit * Add ceil function

* Add translate expression support (apache#68) * Initial commit * Introduce TranslateHolder * Remove unused header * Return 1 if empty string is given as substring (apache#69) * Add two math operations: floor & ceil (apache#72) * Inital commit * Add ceil function Co-authored-by: PHILO-HE <feilong.he@intel.com>

* Inital commit * Add ceil function

Allow read-only memory mapped source.

d8939fa

emkornfield reviewed May 7, 2016
View reviewed changes

- Fixed a wrong protection flag in a test

5559b8d

- Added a routine to check the protection flag before writing - Added a unit test to check the error status for protection mode - Improved failure check for mmap()

jihoonson added 3 commits May 8, 2016 11:24

Simplify error check

22e6128

Remove unintended whitespace

63b99c5

Add missing initialization

b928031

emkornfield reviewed May 9, 2016
View reviewed changes

Change the type of protection flag from int8_t to int

f55dd22

asfgit closed this in c9ffe54 May 9, 2016

praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018

GDV-94:[C++][Java]Fixed literals and nulls for time types. (apache#72)

edb0fc5

Added support for literals and null for time types.

praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018

GDV-94:[C++][Java]Fixed literals and nulls for time types. (apache#72)

603ef48

Added support for literals and null for time types.

praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018

GDV-94:[C++][Java]Fixed literals and nulls for time types. (apache#72)

cd8986a

Added support for literals and null for time types.

praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018

GDV-94:[C++][Java]Fixed literals and nulls for time types. (apache#72)

97d7dd9

Added support for literals and null for time types.

praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Sep 4, 2018

GDV-94:[C++][Java]Fixed literals and nulls for time types. (apache#72)

1e23542

Added support for literals and null for time types.

praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Sep 10, 2018

GDV-94:[C++][Java]Fixed literals and nulls for time types. (apache#72)

cbe259a

Added support for literals and null for time types.

praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Sep 10, 2018

GDV-94:[C++][Java]Fixed literals and nulls for time types. (apache#72)

0a1edd5

Added support for literals and null for time types.

zhouyuan pushed a commit to zhouyuan/arrow that referenced this pull request Jan 7, 2022

Add two math operations: floor & ceil (apache#72)

5b50ffe

* Inital commit * Add ceil function

zhztheplayer pushed a commit to zhztheplayer/arrow-1 that referenced this pull request Jan 7, 2022

Add two math operations: floor & ceil (apache#72)

c2f31f5

* Inital commit * Add ceil function

zhztheplayer pushed a commit to zhztheplayer/arrow-1 that referenced this pull request Feb 8, 2022

Add two math operations: floor & ceil (apache#72)

360c5ef

* Inital commit * Add ceil function

zhztheplayer pushed a commit to zhztheplayer/arrow-1 that referenced this pull request Mar 3, 2022

Add two math operations: floor & ceil (apache#72)

9f876a6

* Inital commit * Add ceil function

rui-mo pushed a commit to rui-mo/arrow-1 that referenced this pull request Mar 23, 2022

Add two math operations: floor & ceil (apache#72)

a8fb555

* Inital commit * Add ceil function

paleolimbot mentioned this pull request Jan 28, 2023

[R] Crash on MacOS (x86) when running tests with homebrew apache-arrow also installed #33903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-194: C++: Allow read-only memory mapped source #72

ARROW-194: C++: Allow read-only memory mapped source #72

jihoonson commented May 7, 2016

emkornfield May 7, 2016

jihoonson May 8, 2016

jihoonson commented May 8, 2016

jihoonson commented May 9, 2016

emkornfield May 9, 2016

jihoonson May 9, 2016

emkornfield commented May 9, 2016

jihoonson commented May 9, 2016

wesm commented May 9, 2016

jihoonson commented May 9, 2016

wesm commented May 9, 2016

jihoonson commented May 9, 2016

wesm commented May 9, 2016

wesm commented May 9, 2016

wesm commented May 9, 2016

jihoonson commented May 9, 2016

ARROW-194: C++: Allow read-only memory mapped source #72

ARROW-194: C++: Allow read-only memory mapped source #72

Conversation

jihoonson commented May 7, 2016

emkornfield May 7, 2016

Choose a reason for hiding this comment

jihoonson May 8, 2016

Choose a reason for hiding this comment

jihoonson commented May 8, 2016

jihoonson commented May 9, 2016

emkornfield May 9, 2016

Choose a reason for hiding this comment

jihoonson May 9, 2016

Choose a reason for hiding this comment

emkornfield commented May 9, 2016

jihoonson commented May 9, 2016

wesm commented May 9, 2016

jihoonson commented May 9, 2016

wesm commented May 9, 2016

jihoonson commented May 9, 2016

wesm commented May 9, 2016

wesm commented May 9, 2016

wesm commented May 9, 2016

jihoonson commented May 9, 2016