Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and setting io.netty.tryReflectionSetAccessible to true for java builds

[Siddharth Teotia]

Hi Bryan,

AFAIK, there is not other impact. So we should be good.

The last few integration issues that I had been chasing are now fixed (got
a clean build with my previous commit pushed over the weekend). I just
pushed a new commit with some cleanup and the changes are now ready. We
should plan to merge this asap this week.

Thanks,
Siddharth

On Fri, May 3, 2019 at 10:21 AM Bryan Cutler  wrote:

> Hi Sidd,
>
> Does setting the system property io.netty.tryReflectionSetAccessible to
> true have any other adverse effect other than those warnings during build?
>
> Bryan
>
> On Thu, May 2, 2019 at 8:43 PM Jacques Nadeau  wrote:
>
> > I'm onboard with this change.
> >
> > On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia 
> > wrote:
> >
> > > As part of working on this patch <
> > > https://github.com/apache/arrow/pull/4151>,
> > > I ran into a problem with jdk 9 and 11 builds.  Since memory underlying
> > > ArrowBuf may not necessarily be a ByteBuf (or any of its extensions),
> > > methods like nioBuffer() can no longer be delegated as
> > > UnsafeDirectLittleEndian.nioBuffer() to Netty implementation.
> > >
> > > So I used PlatformDependent.directBuffer(memory address, size) to
> create
> > a
> > > direct Byte Buffer  to closely mimic what Netty was originally doing
> > > underneath for nioBuffer(). It turns out that PlatformDependent code in
> > > netty first checks for the existence of constructor
> DirectByteBuffer(long
> > > address, int size) as seen here
> > > <
> > >
> >
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L223
> > > >.
> > > The constructor (long address, int size) is very well available in jdk
> > 8, 9
> > > and 11 but on the next line it tries to set it accessible. The
> reflection
> > > based access is disabled by default in netty code for jdk >= 9 as seen
> > here
> > > <
> > >
> >
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L829
> > > >.
> > > Thus calls to PlatformDependent.directBuffer(address, size) were
> failing
> > in
> > > travis CI builds for JDK 9 and 11 with UnsupportedOperationException as
> > > seen here
> > > <
> > >
> >
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L415
> > > >
> > > and
> > > this was because of the decision that was taken by netty at startup
> w.r.t
> > > whether to provide access to constructor or not.
> > >
> > > We should set io.netty.tryReflectionSetAccessible system property to
> true
> > > in java root pom
> > >
> > > I want to make sure people are aware and agree/disagree with this
> change.
> > >
> > > The tests now give the following warning:
> > >
> > > WARNING: An illegal reflective access operation has occurred
> > > WARNING: Illegal reflective access by
> > io.netty.util.internal.ReflectionUtil
> > >
> > >
> >
> (file:/Users/siddharthteotia/.m2/repository/io/netty/netty-common/4.1.22.Final/netty-common-4.1.22.Final.jar)
> > > to constructor java.nio.DirectByteBuffer(long,int)
> > > WARNING: Please consider reporting this to the maintainers of
> > > io.netty.util.internal.ReflectionUtil
> > > WARNING: Use --illegal-access=warn to enable warnings of further
> illegal
> > > reflective access operations
> > > WARNING: All illegal access operations will be denied in a future
> release
> > >
> > > Thanks.
> > > On Thu, Apr 18, 2019 at 3:39 PM Siddharth Teotia  >
> > > wrote:
> > >
> > > > I  have made all the necessary changes in java code to work with new
> > > > ArrowBuf, ReferenceManager interfaces. More importantly, there is a
> > > wrapper
> > > > buffer NettyArrowBuf interface to comply with usage in RPC and Netty
> > > > related code. It will be good to get feedback on this one (and of
> > course
> > > > all other changes).  As of now, the java modules build fine but I
> have
> > to
> > > > fix test failures. That is in progress.
> > > >
> > > > On Wed, Apr 17, 2019 at 6:41 AM Jacques Nadeau 
> > > wrote:
> > > >
> > > >> Are there any other general comments here? If not, let's get this
> done
> > > and
> > > >> merged.
> > > >>
> > > >> On Mon, Apr 15, 2019, 4:19 PM Siddharth Teotia <
> siddha...@dremio.com>
> > > >> wrote:
> > > >>
> > > >> > I believe reader/writer indexes are typically used when we send
> > > buffers
> > > >> > over the wire -- so may not be necessary for all users of
> > ArrowBuf.  I
> > > >> am
> > > >> > okay with the idea of providing a simple wrapper to ArrowBuf to
> > manage
> > > >> the
> > > >> > reader/writer indexes with a couple of APIs. Note that some APIs
> > like
> > > >> > writeInt, writeLong() bump the writer index unlike setInt/setLong
> > > >> > counterparts. JsonFileReader uses some of these APIs.
> > > >> >
> > > >> >
> > > >> >
> > > >> > On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau <
> jacq...@apache.org>
> > > >> wrote:
> > > >> >
> > > >> > > Hey Sidd,
> > > >> > >
> > > >> > > Thanks for pulling this 

Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and setting io.netty.tryReflectionSetAccessible to true for java builds

[Bryan Cutler]

Hi Sidd,

Does setting the system property io.netty.tryReflectionSetAccessible to
true have any other adverse effect other than those warnings during build?

Bryan

On Thu, May 2, 2019 at 8:43 PM Jacques Nadeau  wrote:

> I'm onboard with this change.
>
> On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia 
> wrote:
>
> > As part of working on this patch <
> > https://github.com/apache/arrow/pull/4151>,
> > I ran into a problem with jdk 9 and 11 builds.  Since memory underlying
> > ArrowBuf may not necessarily be a ByteBuf (or any of its extensions),
> > methods like nioBuffer() can no longer be delegated as
> > UnsafeDirectLittleEndian.nioBuffer() to Netty implementation.
> >
> > So I used PlatformDependent.directBuffer(memory address, size) to create
> a
> > direct Byte Buffer  to closely mimic what Netty was originally doing
> > underneath for nioBuffer(). It turns out that PlatformDependent code in
> > netty first checks for the existence of constructor DirectByteBuffer(long
> > address, int size) as seen here
> > <
> >
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L223
> > >.
> > The constructor (long address, int size) is very well available in jdk
> 8, 9
> > and 11 but on the next line it tries to set it accessible. The reflection
> > based access is disabled by default in netty code for jdk >= 9 as seen
> here
> > <
> >
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L829
> > >.
> > Thus calls to PlatformDependent.directBuffer(address, size) were failing
> in
> > travis CI builds for JDK 9 and 11 with UnsupportedOperationException as
> > seen here
> > <
> >
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L415
> > >
> > and
> > this was because of the decision that was taken by netty at startup w.r.t
> > whether to provide access to constructor or not.
> >
> > We should set io.netty.tryReflectionSetAccessible system property to true
> > in java root pom
> >
> > I want to make sure people are aware and agree/disagree with this change.
> >
> > The tests now give the following warning:
> >
> > WARNING: An illegal reflective access operation has occurred
> > WARNING: Illegal reflective access by
> io.netty.util.internal.ReflectionUtil
> >
> >
> (file:/Users/siddharthteotia/.m2/repository/io/netty/netty-common/4.1.22.Final/netty-common-4.1.22.Final.jar)
> > to constructor java.nio.DirectByteBuffer(long,int)
> > WARNING: Please consider reporting this to the maintainers of
> > io.netty.util.internal.ReflectionUtil
> > WARNING: Use --illegal-access=warn to enable warnings of further illegal
> > reflective access operations
> > WARNING: All illegal access operations will be denied in a future release
> >
> > Thanks.
> > On Thu, Apr 18, 2019 at 3:39 PM Siddharth Teotia 
> > wrote:
> >
> > > I  have made all the necessary changes in java code to work with new
> > > ArrowBuf, ReferenceManager interfaces. More importantly, there is a
> > wrapper
> > > buffer NettyArrowBuf interface to comply with usage in RPC and Netty
> > > related code. It will be good to get feedback on this one (and of
> course
> > > all other changes).  As of now, the java modules build fine but I have
> to
> > > fix test failures. That is in progress.
> > >
> > > On Wed, Apr 17, 2019 at 6:41 AM Jacques Nadeau 
> > wrote:
> > >
> > >> Are there any other general comments here? If not, let's get this done
> > and
> > >> merged.
> > >>
> > >> On Mon, Apr 15, 2019, 4:19 PM Siddharth Teotia 
> > >> wrote:
> > >>
> > >> > I believe reader/writer indexes are typically used when we send
> > buffers
> > >> > over the wire -- so may not be necessary for all users of
> ArrowBuf.  I
> > >> am
> > >> > okay with the idea of providing a simple wrapper to ArrowBuf to
> manage
> > >> the
> > >> > reader/writer indexes with a couple of APIs. Note that some APIs
> like
> > >> > writeInt, writeLong() bump the writer index unlike setInt/setLong
> > >> > counterparts. JsonFileReader uses some of these APIs.
> > >> >
> > >> >
> > >> >
> > >> > On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau 
> > >> wrote:
> > >> >
> > >> > > Hey Sidd,
> > >> > >
> > >> > > Thanks for pulling this together. This looks very promising. One
> > quick
> > >> > > thought: do we think the concept of the reader and writer index
> need
> > >> to
> > >> > be
> > >> > > on ArrowBuf? It seems like something that could be added as an
> > >> additional
> > >> > > decoration/wrapper when needed instead of being part of the core
> > >> > structure.
> > >> > >
> > >> > > On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia <
> > >> siddha...@dremio.com>
> > >> > > wrote:
> > >> > >
> > >> > > > Hi All,
> > >> > > >
> > >> > > > I have put a PR with WIP changes. All the major set of changes
> > have
> > >> > been
> > >> > > > done to decouple the usage of ArrowBuf and reference management.
> > The
> > >> > > > 

Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and setting io.netty.tryReflectionSetAccessible to true for java builds

[Jacques Nadeau]

I'm onboard with this change.

On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia 
wrote:

> As part of working on this patch <
> https://github.com/apache/arrow/pull/4151>,
> I ran into a problem with jdk 9 and 11 builds.  Since memory underlying
> ArrowBuf may not necessarily be a ByteBuf (or any of its extensions),
> methods like nioBuffer() can no longer be delegated as
> UnsafeDirectLittleEndian.nioBuffer() to Netty implementation.
>
> So I used PlatformDependent.directBuffer(memory address, size) to create a
> direct Byte Buffer  to closely mimic what Netty was originally doing
> underneath for nioBuffer(). It turns out that PlatformDependent code in
> netty first checks for the existence of constructor DirectByteBuffer(long
> address, int size) as seen here
> <
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L223
> >.
> The constructor (long address, int size) is very well available in jdk 8, 9
> and 11 but on the next line it tries to set it accessible. The reflection
> based access is disabled by default in netty code for jdk >= 9 as seen here
> <
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L829
> >.
> Thus calls to PlatformDependent.directBuffer(address, size) were failing in
> travis CI builds for JDK 9 and 11 with UnsupportedOperationException as
> seen here
> <
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L415
> >
> and
> this was because of the decision that was taken by netty at startup w.r.t
> whether to provide access to constructor or not.
>
> We should set io.netty.tryReflectionSetAccessible system property to true
> in java root pom
>
> I want to make sure people are aware and agree/disagree with this change.
>
> The tests now give the following warning:
>
> WARNING: An illegal reflective access operation has occurred
> WARNING: Illegal reflective access by io.netty.util.internal.ReflectionUtil
>
> (file:/Users/siddharthteotia/.m2/repository/io/netty/netty-common/4.1.22.Final/netty-common-4.1.22.Final.jar)
> to constructor java.nio.DirectByteBuffer(long,int)
> WARNING: Please consider reporting this to the maintainers of
> io.netty.util.internal.ReflectionUtil
> WARNING: Use --illegal-access=warn to enable warnings of further illegal
> reflective access operations
> WARNING: All illegal access operations will be denied in a future release
>
> Thanks.
> On Thu, Apr 18, 2019 at 3:39 PM Siddharth Teotia 
> wrote:
>
> > I  have made all the necessary changes in java code to work with new
> > ArrowBuf, ReferenceManager interfaces. More importantly, there is a
> wrapper
> > buffer NettyArrowBuf interface to comply with usage in RPC and Netty
> > related code. It will be good to get feedback on this one (and of course
> > all other changes).  As of now, the java modules build fine but I have to
> > fix test failures. That is in progress.
> >
> > On Wed, Apr 17, 2019 at 6:41 AM Jacques Nadeau 
> wrote:
> >
> >> Are there any other general comments here? If not, let's get this done
> and
> >> merged.
> >>
> >> On Mon, Apr 15, 2019, 4:19 PM Siddharth Teotia 
> >> wrote:
> >>
> >> > I believe reader/writer indexes are typically used when we send
> buffers
> >> > over the wire -- so may not be necessary for all users of ArrowBuf.  I
> >> am
> >> > okay with the idea of providing a simple wrapper to ArrowBuf to manage
> >> the
> >> > reader/writer indexes with a couple of APIs. Note that some APIs like
> >> > writeInt, writeLong() bump the writer index unlike setInt/setLong
> >> > counterparts. JsonFileReader uses some of these APIs.
> >> >
> >> >
> >> >
> >> > On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau 
> >> wrote:
> >> >
> >> > > Hey Sidd,
> >> > >
> >> > > Thanks for pulling this together. This looks very promising. One
> quick
> >> > > thought: do we think the concept of the reader and writer index need
> >> to
> >> > be
> >> > > on ArrowBuf? It seems like something that could be added as an
> >> additional
> >> > > decoration/wrapper when needed instead of being part of the core
> >> > structure.
> >> > >
> >> > > On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia <
> >> siddha...@dremio.com>
> >> > > wrote:
> >> > >
> >> > > > Hi All,
> >> > > >
> >> > > > I have put a PR with WIP changes. All the major set of changes
> have
> >> > been
> >> > > > done to decouple the usage of ArrowBuf and reference management.
> The
> >> > > > ArrowBuf interface is much simpler and clean now.
> >> > > >
> >> > > > I believe there would be several folks in the community interested
> >> in
> >> > > these
> >> > > > changes so please feel free to take a look at the PR and provide
> >> your
> >> > > > feedback -- https://github.com/apache/arrow/pull/4151
> >> > > >
> >> > > > There is some cleanup needed (code doesn't compile yet) due to
> >> moving
> >> > the
> >> > > > APIs but I have raised the PR to get an early 

Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and setting io.netty.tryReflectionSetAccessible to true for java builds

[Siddharth Teotia]

As part of working on this patch ,
I ran into a problem with jdk 9 and 11 builds.  Since memory underlying
ArrowBuf may not necessarily be a ByteBuf (or any of its extensions),
methods like nioBuffer() can no longer be delegated as
UnsafeDirectLittleEndian.nioBuffer() to Netty implementation.

So I used PlatformDependent.directBuffer(memory address, size) to create a
direct Byte Buffer  to closely mimic what Netty was originally doing
underneath for nioBuffer(). It turns out that PlatformDependent code in
netty first checks for the existence of constructor DirectByteBuffer(long
address, int size) as seen here
.
The constructor (long address, int size) is very well available in jdk 8, 9
and 11 but on the next line it tries to set it accessible. The reflection
based access is disabled by default in netty code for jdk >= 9 as seen here
.
Thus calls to PlatformDependent.directBuffer(address, size) were failing in
travis CI builds for JDK 9 and 11 with UnsupportedOperationException as
seen here

and
this was because of the decision that was taken by netty at startup w.r.t
whether to provide access to constructor or not.

We should set io.netty.tryReflectionSetAccessible system property to true
in java root pom

I want to make sure people are aware and agree/disagree with this change.

The tests now give the following warning:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by io.netty.util.internal.ReflectionUtil
(file:/Users/siddharthteotia/.m2/repository/io/netty/netty-common/4.1.22.Final/netty-common-4.1.22.Final.jar)
to constructor java.nio.DirectByteBuffer(long,int)
WARNING: Please consider reporting this to the maintainers of
io.netty.util.internal.ReflectionUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal
reflective access operations
WARNING: All illegal access operations will be denied in a future release

Thanks.
On Thu, Apr 18, 2019 at 3:39 PM Siddharth Teotia 
wrote:

> I  have made all the necessary changes in java code to work with new
> ArrowBuf, ReferenceManager interfaces. More importantly, there is a wrapper
> buffer NettyArrowBuf interface to comply with usage in RPC and Netty
> related code. It will be good to get feedback on this one (and of course
> all other changes).  As of now, the java modules build fine but I have to
> fix test failures. That is in progress.
>
> On Wed, Apr 17, 2019 at 6:41 AM Jacques Nadeau  wrote:
>
>> Are there any other general comments here? If not, let's get this done and
>> merged.
>>
>> On Mon, Apr 15, 2019, 4:19 PM Siddharth Teotia 
>> wrote:
>>
>> > I believe reader/writer indexes are typically used when we send buffers
>> > over the wire -- so may not be necessary for all users of ArrowBuf.  I
>> am
>> > okay with the idea of providing a simple wrapper to ArrowBuf to manage
>> the
>> > reader/writer indexes with a couple of APIs. Note that some APIs like
>> > writeInt, writeLong() bump the writer index unlike setInt/setLong
>> > counterparts. JsonFileReader uses some of these APIs.
>> >
>> >
>> >
>> > On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau 
>> wrote:
>> >
>> > > Hey Sidd,
>> > >
>> > > Thanks for pulling this together. This looks very promising. One quick
>> > > thought: do we think the concept of the reader and writer index need
>> to
>> > be
>> > > on ArrowBuf? It seems like something that could be added as an
>> additional
>> > > decoration/wrapper when needed instead of being part of the core
>> > structure.
>> > >
>> > > On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia <
>> siddha...@dremio.com>
>> > > wrote:
>> > >
>> > > > Hi All,
>> > > >
>> > > > I have put a PR with WIP changes. All the major set of changes have
>> > been
>> > > > done to decouple the usage of ArrowBuf and reference management. The
>> > > > ArrowBuf interface is much simpler and clean now.
>> > > >
>> > > > I believe there would be several folks in the community interested
>> in
>> > > these
>> > > > changes so please feel free to take a look at the PR and provide
>> your
>> > > > feedback -- https://github.com/apache/arrow/pull/4151
>> > > >
>> > > > There is some cleanup needed (code doesn't compile yet) due to
>> moving
>> > the
>> > > > APIs but I have raised the PR to get an early feedback from the
>> > community
>> > > > on the critical changes.
>> > > >
>> > > > Thanks,
>> > > > Siddharth
>> > > >
>> > >
>> >
>>
>

Re: ARROW-3191: Making ArrowBuf work with arbitrary memory

[Siddharth Teotia]

I  have made all the necessary changes in java code to work with new
ArrowBuf, ReferenceManager interfaces. More importantly, there is a wrapper
buffer NettyArrowBuf interface to comply with usage in RPC and Netty
related code. It will be good to get feedback on this one (and of course
all other changes).  As of now, the java modules build fine but I have to
fix test failures. That is in progress.

On Wed, Apr 17, 2019 at 6:41 AM Jacques Nadeau  wrote:

> Are there any other general comments here? If not, let's get this done and
> merged.
>
> On Mon, Apr 15, 2019, 4:19 PM Siddharth Teotia 
> wrote:
>
> > I believe reader/writer indexes are typically used when we send buffers
> > over the wire -- so may not be necessary for all users of ArrowBuf.  I am
> > okay with the idea of providing a simple wrapper to ArrowBuf to manage
> the
> > reader/writer indexes with a couple of APIs. Note that some APIs like
> > writeInt, writeLong() bump the writer index unlike setInt/setLong
> > counterparts. JsonFileReader uses some of these APIs.
> >
> >
> >
> > On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau 
> wrote:
> >
> > > Hey Sidd,
> > >
> > > Thanks for pulling this together. This looks very promising. One quick
> > > thought: do we think the concept of the reader and writer index need to
> > be
> > > on ArrowBuf? It seems like something that could be added as an
> additional
> > > decoration/wrapper when needed instead of being part of the core
> > structure.
> > >
> > > On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia <
> siddha...@dremio.com>
> > > wrote:
> > >
> > > > Hi All,
> > > >
> > > > I have put a PR with WIP changes. All the major set of changes have
> > been
> > > > done to decouple the usage of ArrowBuf and reference management. The
> > > > ArrowBuf interface is much simpler and clean now.
> > > >
> > > > I believe there would be several folks in the community interested in
> > > these
> > > > changes so please feel free to take a look at the PR and provide your
> > > > feedback -- https://github.com/apache/arrow/pull/4151
> > > >
> > > > There is some cleanup needed (code doesn't compile yet) due to moving
> > the
> > > > APIs but I have raised the PR to get an early feedback from the
> > community
> > > > on the critical changes.
> > > >
> > > > Thanks,
> > > > Siddharth
> > > >
> > >
> >
>

Re: ARROW-3191: Making ArrowBuf work with arbitrary memory

[Jacques Nadeau]

Are there any other general comments here? If not, let's get this done and
merged.

On Mon, Apr 15, 2019, 4:19 PM Siddharth Teotia  wrote:

> I believe reader/writer indexes are typically used when we send buffers
> over the wire -- so may not be necessary for all users of ArrowBuf.  I am
> okay with the idea of providing a simple wrapper to ArrowBuf to manage the
> reader/writer indexes with a couple of APIs. Note that some APIs like
> writeInt, writeLong() bump the writer index unlike setInt/setLong
> counterparts. JsonFileReader uses some of these APIs.
>
>
>
> On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau  wrote:
>
> > Hey Sidd,
> >
> > Thanks for pulling this together. This looks very promising. One quick
> > thought: do we think the concept of the reader and writer index need to
> be
> > on ArrowBuf? It seems like something that could be added as an additional
> > decoration/wrapper when needed instead of being part of the core
> structure.
> >
> > On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia 
> > wrote:
> >
> > > Hi All,
> > >
> > > I have put a PR with WIP changes. All the major set of changes have
> been
> > > done to decouple the usage of ArrowBuf and reference management. The
> > > ArrowBuf interface is much simpler and clean now.
> > >
> > > I believe there would be several folks in the community interested in
> > these
> > > changes so please feel free to take a look at the PR and provide your
> > > feedback -- https://github.com/apache/arrow/pull/4151
> > >
> > > There is some cleanup needed (code doesn't compile yet) due to moving
> the
> > > APIs but I have raised the PR to get an early feedback from the
> community
> > > on the critical changes.
> > >
> > > Thanks,
> > > Siddharth
> > >
> >
>

Re: ARROW-3191: Making ArrowBuf work with arbitrary memory

[Siddharth Teotia]

I believe reader/writer indexes are typically used when we send buffers
over the wire -- so may not be necessary for all users of ArrowBuf.  I am
okay with the idea of providing a simple wrapper to ArrowBuf to manage the
reader/writer indexes with a couple of APIs. Note that some APIs like
writeInt, writeLong() bump the writer index unlike setInt/setLong
counterparts. JsonFileReader uses some of these APIs.



On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau  wrote:

> Hey Sidd,
>
> Thanks for pulling this together. This looks very promising. One quick
> thought: do we think the concept of the reader and writer index need to be
> on ArrowBuf? It seems like something that could be added as an additional
> decoration/wrapper when needed instead of being part of the core structure.
>
> On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia 
> wrote:
>
> > Hi All,
> >
> > I have put a PR with WIP changes. All the major set of changes have been
> > done to decouple the usage of ArrowBuf and reference management. The
> > ArrowBuf interface is much simpler and clean now.
> >
> > I believe there would be several folks in the community interested in
> these
> > changes so please feel free to take a look at the PR and provide your
> > feedback -- https://github.com/apache/arrow/pull/4151
> >
> > There is some cleanup needed (code doesn't compile yet) due to moving the
> > APIs but I have raised the PR to get an early feedback from the community
> > on the critical changes.
> >
> > Thanks,
> > Siddharth
> >
>

Re: ARROW-3191: Making ArrowBuf work with arbitrary memory

[Jacques Nadeau]

Hey Sidd,

Thanks for pulling this together. This looks very promising. One quick
thought: do we think the concept of the reader and writer index need to be
on ArrowBuf? It seems like something that could be added as an additional
decoration/wrapper when needed instead of being part of the core structure.

On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia 
wrote:

> Hi All,
>
> I have put a PR with WIP changes. All the major set of changes have been
> done to decouple the usage of ArrowBuf and reference management. The
> ArrowBuf interface is much simpler and clean now.
>
> I believe there would be several folks in the community interested in these
> changes so please feel free to take a look at the PR and provide your
> feedback -- https://github.com/apache/arrow/pull/4151
>
> There is some cleanup needed (code doesn't compile yet) due to moving the
> APIs but I have raised the PR to get an early feedback from the community
> on the critical changes.
>
> Thanks,
> Siddharth
>