You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/08/26 06:25:17 UTC
[GitHub] [arrow] kiszk opened a new pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
kiszk opened a new pull request #8057:
URL: https://github.com/apache/arrow/pull/8057
This PR enables `UnsafeDirectLittleEndian` class by removing to throw an exception on a big-endian platform. This is because this class originally supports primitive data types (up to 64-bit) in a native-endianness.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk edited a comment on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk edited a comment on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-723179677
I see. I will rename this class name and keep the current `UnsafeDirectLittleEndian` as `@deprecated` like [this](https://github.com/apache/arrow/blob/master/java/memory/memory-netty/src/main/java/org/apache/arrow/memory/ArrowByteBufAllocator.java#L37).
How about the new class name `UnsafeDirect`? Now, the endian does not matter in this class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-726960913
merged to master
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-723173844
I also think the classes mentioned above should be renamed to "***NativeEndian", but I think `UnsafeDirectLittleEndian` is public so we need to be careful about that. Is there a way we can deprecate the old name and not make any breaking API changes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r519156391
##########
File path: java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java
##########
@@ -60,9 +59,6 @@
private UnsafeDirectLittleEndian(AbstractByteBuf buf, boolean fake) {
super(buf);
- if (!NATIVE_ORDER || buf.order() != ByteOrder.BIG_ENDIAN) {
Review comment:
I think that this is not necessary since the overriding accessors read/write data using `PlatfoormDependant.***`` directory. The buffer order does not matter.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r522508039
##########
File path: java/memory/memory-netty/src/test/java/io/netty/buffer/TestUnsafeDirectLittleEndian.java
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.netty.buffer;
+
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+
+import org.junit.Test;
+
+public class TestUnsafeDirectLittleEndian {
+ private static final boolean LITTLE_ENDIAN = ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN;
+
+ @Test
+ public void testPrimitiveGetSet() {
+ ByteBuf byteBuf = Unpooled.directBuffer(64);
+ UnsafeDirectLittleEndian unsafeDirect = new UnsafeDirectLittleEndian(new LargeBuffer(byteBuf));
+
+ unsafeDirect.setByte(0, Byte.MAX_VALUE);
+ unsafeDirect.setByte(1, -1); // 0xFF
+ unsafeDirect.setShort(2, Short.MAX_VALUE);
+ unsafeDirect.setShort(4, -2); // 0xFFFE
+ unsafeDirect.setInt(8, Integer.MAX_VALUE);
+ unsafeDirect.setInt(12, -66052); // 0xFFFE FDFC
+ unsafeDirect.setLong(16, Long.MAX_VALUE);
+ unsafeDirect.setLong(24, -4295098372L); // 0xFFFF FFFE FFFD FFFC
+ unsafeDirect.setFloat(32, 1.23F);
+ unsafeDirect.setFloat(36, -1.23F);
+ unsafeDirect.setDouble(40, 1.234567D);
+ unsafeDirect.setDouble(48, -1.234567D);
+
+ assertEquals(Byte.MAX_VALUE, unsafeDirect.getByte(0));
+ assertEquals(-1, unsafeDirect.getByte(1));
+ assertEquals(Short.MAX_VALUE, unsafeDirect.getShort(2));
+ assertEquals(-2, unsafeDirect.getShort(4));
+ assertEquals((char) 65534, unsafeDirect.getChar(4));
+ assertEquals(Integer.MAX_VALUE, unsafeDirect.getInt(8));
+ assertEquals(-66052, unsafeDirect.getInt(12));
+ assertEquals(4294901244L, unsafeDirect.getUnsignedInt(12));
+ assertEquals(Long.MAX_VALUE, unsafeDirect.getLong(16));
+ assertEquals(-4295098372L, unsafeDirect.getLong(24));
+ assertEquals(1.23F, unsafeDirect.getFloat(32), 0.0);
+ assertEquals(-1.23F, unsafeDirect.getFloat(36), 0.0);
+ assertEquals(1.234567D, unsafeDirect.getDouble(40), 0.0);
+ assertEquals(-1.234567D, unsafeDirect.getDouble(48), 0.0);
+
+ byte[] inBytes = "1234567".getBytes(StandardCharsets.UTF_8);
+ try (ByteArrayInputStream bais = new ByteArrayInputStream(inBytes);
+ ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
+ assertEquals(5, unsafeDirect.setBytes(56, bais, 5));
+ unsafeDirect.getBytes(56, baos, 5);
+ assertEquals("12345", new String(baos.toByteArray(), StandardCharsets.UTF_8));
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
+ }
+
+ @Test
+ public void testPrimitiveWriteRead() {
Review comment:
I think the test above is sufficient.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-722573516
@BryanCutler Thank you for reviewing this PR. While I revisited the `UnsafeDirectLittleEndian` class, I cannot find any code that assumes the LE. Could you please point out the portions?
Regarding the name, it is preferable to renaming the name. However, since I was not able to understand the effect, I did not change the name of this class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-722783243
Good catch. I think that it would be good to rename `PooledByteBufAllocatorL` that is its outer class, too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-722569982
@kiszk I just quickly skimmed the `UnsafeDirectLittleEndian` class and it looks like there are some assumptions that the order is LE, is that correct? If so we should probably leave these checks in place, make another class for native endian, e.g. `UnsafeDirectNaiveEndian`, and perhaps deprecate `UnsafeDirectLittleEndian`? What are your thoughts, also cc @emkornfield @liyafan82
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler closed pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler closed pull request #8057:
URL: https://github.com/apache/arrow/pull/8057
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r518862408
##########
File path: java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java
##########
@@ -60,9 +59,6 @@
private UnsafeDirectLittleEndian(AbstractByteBuf buf, boolean fake) {
super(buf);
- if (!NATIVE_ORDER || buf.order() != ByteOrder.BIG_ENDIAN) {
Review comment:
Should keep the second check that `buf.order() != ByteOrder.BIG_ENDIAN` to make sure the wrapped buffer is BE?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r522507327
##########
File path: java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java
##########
@@ -170,6 +166,66 @@ public ByteBuf setDouble(int index, double value) {
return this;
}
+ @Override
Review comment:
I'd rather not add these if it's not necessary. I don't think the write* methods are even being used..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-722778362
Thanks. I will try to add tests to verify the behavior of methods.
A new name `UnsafeDirect` looks simple to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler edited a comment on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler edited a comment on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-723173844
I also think the classes mentioned above should be renamed to "***NativeEndian", but I think `UnsafeDirectLittleEndian` is public so we need to be careful about that. Is there a way we can deprecate the old name and not make any breaking API changes?
If not, then we don't need to rename here, but let's make a JIRA to do that before the next major release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-723209194
I think we should maybe have the any naming changes/migration to new class be a separate PR. If that isn't too much effort.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-680687276
https://issues.apache.org/jira/browse/ARROW-9862
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r523152926
##########
File path: java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java
##########
@@ -60,9 +59,6 @@
private UnsafeDirectLittleEndian(AbstractByteBuf buf, boolean fake) {
super(buf);
- if (!NATIVE_ORDER || buf.order() != ByteOrder.BIG_ENDIAN) {
Review comment:
Yeah, from what I can tell the buffer order is only taken into account when calling `PlatfoormDependant.getIntSafe()` etc., which is not used by this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r519152228
##########
File path: java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java
##########
@@ -60,9 +59,6 @@
private UnsafeDirectLittleEndian(AbstractByteBuf buf, boolean fake) {
super(buf);
- if (!NATIVE_ORDER || buf.order() != ByteOrder.BIG_ENDIAN) {
Review comment:
Sure, I will update this to check it using `buf.order !=ByteOrder.nativeOrder()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-722762839
After examining the code, I also think `UnsafeDirectLittleEndian` is assuming the native byte order. This should make sense before, as we only support little endian. After supporting big endian, it would be nice to rename the class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-723179677
I see. I will rename this class name and keep `UnsafeDirectLittleEndian` as `@deprecated` like [this](https://github.com/apache/arrow/blob/master/java/memory/memory-netty/src/main/java/org/apache/arrow/memory/ArrowByteBufAllocator.java#L37).
How about the new class name `UnsafeDirect`? Now, the endian does not matter in this class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-723455119
I added tests to verify it works well on any architecture.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r519152228
##########
File path: java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java
##########
@@ -60,9 +59,6 @@
private UnsafeDirectLittleEndian(AbstractByteBuf buf, boolean fake) {
super(buf);
- if (!NATIVE_ORDER || buf.order() != ByteOrder.BIG_ENDIAN) {
Review comment:
~Sure, I will update this.~
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#discussion_r519152228
##########
File path: java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java
##########
@@ -60,9 +59,6 @@
private UnsafeDirectLittleEndian(AbstractByteBuf buf, boolean fake) {
super(buf);
- if (!NATIVE_ORDER || buf.order() != ByteOrder.BIG_ENDIAN) {
Review comment:
Sure, I will update this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk edited a comment on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
kiszk edited a comment on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-723179677
I see. I will rename this class name and keep the current `UnsafeDirectLittleEndian`, which will extend the new class, as `@deprecated` like [this](https://github.com/apache/arrow/blob/master/java/memory/memory-netty/src/main/java/org/apache/arrow/memory/ArrowByteBufAllocator.java#L37).
How about the new class name `UnsafeDirect`? Now, the endian does not matter in this class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #8057: ARROW-9862: [Java] Enable UnsafeDirectLittleEndian on a big-endian platform
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #8057:
URL: https://github.com/apache/arrow/pull/8057#issuecomment-722779712
> Thanks. I will try to add tests to verify the behavior of methods.
>
> A new name `UnsafeDirect` looks simple to me.
BTW, I think it would be nice to rename `AccountedUnsafeDirectLittleEndian` too, although it is an inner private class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org