You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fory.apache.org by "stevenschlansker (via GitHub)" <gi...@apache.org> on 2025/07/15 00:18:48 UTC
[PR] Draft: introduce Compact Row Codec [fory]
stevenschlansker opened a new pull request, #2414:
URL: https://github.com/apache/fory/pull/2414
## What does this PR do?
Introduce alternate "compact" row encoding that better uses knowledge of fixed-size types and sacrifices alignment to save space.
Introduce new Builder pattern to avoid explosion of `Encoders` static methods as more features are added to row format.
Optimizations include:
* struct stores fixed-size fields (e.g. Int128. FixedSizeBinary) inline in fixed-data area without offset + size
* struct of all fixed-sized fields is itself considered fixed-size to store in other struct or array
* struct skips null bitmap if all fields are non-nullable
* struct sorts fields by fixed-size for best-effort (but not guaranteed) alignment
* struct can use less than 8 bytes for small data (int, short, etc)
* struct null bitmap stored at end of struct to borrow alignment padding if possible
* array stores fixed-size fields inline in fixed-data area without offset+size
* array header uses 4 bytes for size (since Collection and array are only int-sized) and leaves remaining 4 bytes for start of null bitmap
Fixups include:
* toString better handles varbinary / fixed-binary (hex dump of first 256 bytes)
* start making Javadoc for row format
TODO:
[ ] figure out how top-level array and map encoder fit into new builder class
Not compatible with existing row format.
## Related issues
Fixes #2337
## Does this PR introduce any user-facing change?
New API for new Compact codec. Existing codec unchanged.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang merged PR #2414:
URL: https://github.com/apache/fory/pull/2414
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "stevenschlansker (via GitHub)" <gi...@apache.org>.
stevenschlansker commented on PR #2414:
URL: https://github.com/apache/fory/pull/2414#issuecomment-3299572267
Hi @chaokunyang , I believe this PR is ready for review now. No rush, I know it is a lot to go through. Happy to explain or change anything that does not make sense.
We run this now in our testing environment successfully with about a 30% - 50% space savings on our datasets. Will be taking compact codec to production over the coming weeks.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #2414:
URL: https://github.com/apache/fory/pull/2414#issuecomment-3399909793
Great work!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378768866
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/CodecFormat.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import java.util.Collection;
+import java.util.Map;
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.types.pojo.Schema;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.BinaryRow;
+import org.apache.fory.format.row.binary.writer.BaseBinaryRowWriter;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.reflect.TypeRef;
+
+interface CodecFormat {
Review Comment:
Will the name `Encoding` better align with `Encoder`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "stevenschlansker (via GitHub)" <gi...@apache.org>.
stevenschlansker commented on PR #2414:
URL: https://github.com/apache/fory/pull/2414#issuecomment-3399851783
ok @chaokunyang I think I've addressed all the feedback. I like the idea of inline strings but I think it makes sense to defer that to a future PR at this point since all these changes are ready now
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #2414:
URL: https://github.com/apache/fory/pull/2414#issuecomment-3303369219
@stevenschlansker Great! 30% - 50% space savings is a huge gain! I will review this PR ASAP, and release it in `0.13.0`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378826811
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BufferResettingMapEncoder.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BufferResettingMapEncoder<T> implements MapEncoder<T> {
Review Comment:
Could we add some documents to clarify the situations when we need to use this and the difference between this and BaseMapEncoder
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2427685304
##########
java/fory-format/README.md:
##########
@@ -8,12 +8,52 @@ Fory row format is heavily inspired by spark tungsten row format, but with chang
- Decimal use arrow decimal format.
- Variable-size field can be inline in fixed-size region if small enough.
- Allow skip padding by generate Row using aot to put offsets in generated code.
-- Support adding fields without breaking compatibility.
-The initial fory java row data structure implementation is modified from spark unsafe row/writer.
+The initial Fory java row data structure implementation is modified from spark unsafe row/writer.
See `Encoders.bean` Javadoc for a list built-in supported types.
+## Row Format Java
+
+To begin using the row format from Java, start with the `Encoders` class:
+
+```
+// Many built-in types and collections are supported
+public record MyRecord(int key, String value) {}
+
+// The encoder supplier is relatively expensive to create
+// It is thread-safe and should be re-used
+Supplier<RowEncoder<MyRecord>> encoderFactory =
+ Encoders.buildBeanCodec(MyRecord.class)
+ .build();
+
+// Each individual encoder is relatively cheap to create
+// It is not thread-safe, but may be reused by the same thread
+var encoder = encoderFactory.get();
+byte[] encoded = encoder.encode(new MyRecord(42, "Test"));
+
+MyRecord deserialized = encoder.decode(encoded);
+```
+
+## Compact Format
+
+The default row format is cross-language compatible and alignment-padded for maximum performance.
+When data size is a greater concern, the compact format provides an alternate encoding that uses
+significantly less space.
+
+Optimizations include:
+
+- struct stores fixed-size fields (e.g. Int128. FixedSizeBinary) inline in fixed-data area without offset + size
+- struct of all fixed-sized fields is itself considered fixed-size to store in other struct or array
+- struct skips null bitmap if all fields are non-nullable
+- struct sorts fields by fixed-size for best-effort (but not guaranteed) alignment
+- struct can use less than 8 bytes for small data (int, short, etc)
+- struct null bitmap stored at end of struct to borrow alignment padding if possible
+- array stores fixed-size fields inline in fixed-data area without offset+size
+- array header uses 4 bytes for size (since Collection and array are only int-sized) and leaves remaining 4 bytes for start of null bitmap
+
Review Comment:
could we add a minmal example about how to use compact mode?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378812160
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/Encoder.java:
##########
@@ -30,11 +30,36 @@
*/
public interface Encoder<T> {
+ /** Decode a buffer with an embedded size. Variants without embedded size are not compatible. */
T decode(MemoryBuffer buffer);
+ /** Decode a buffer without an embedded size. Variants with embedded size are not compatible. */
T decode(byte[] bytes);
+ /** Decode a buffer without an embedded size. Variants with embedded size are not compatible. */
+ T decodeRemaining(MemoryBuffer buffer);
+
+ /** Encode to a buffer without embedded size. Variants with embedded size are not compatible. */
byte[] encode(T obj);
+ /**
+ * Encode to a buffer with an embedded size. Variants without embedded size are not compatible.
+ */
void encode(MemoryBuffer buffer, T obj);
+
+ /**
+ * Encode to a buffer without an embedded size. Variants with embedded size are not compatible.
+ * Returns number of bytes written to the buffer.
+ */
+ int bareEncode(MemoryBuffer buffer, T obj);
Review Comment:
this is only called on map/struct/array, merge into one method and use a branch don't introduce lots of runtime cost, but we can make the interface more simple
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378835112
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BufferResettingMapEncoder.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BufferResettingMapEncoder<T> implements MapEncoder<T> {
Review Comment:
similiar naming for other encoders
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "stevenschlansker (via GitHub)" <gi...@apache.org>.
stevenschlansker commented on PR #2414:
URL: https://github.com/apache/fory/pull/2414#issuecomment-3347969678
Thank you for your review, I will make changes in the coming days.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378761104
##########
java/fory-format/src/main/java/org/apache/fory/format/row/binary/CompactBinaryRow.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 org.apache.fory.format.row.binary;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.types.pojo.Schema;
+import org.apache.fory.format.row.binary.writer.CompactBinaryRowWriter;
+import org.apache.fory.format.type.DataTypes;
+import org.apache.fory.memory.MemoryBuffer;
+
+/**
+ * A compact version of {@link BinaryRow}. The compact encoding includes additional optimizations:
+ *
+ * <ul>
+ * <li>fixed size binary objects are stored in the fixed size section with no pointer needed
+ * <li>small values can take up fewer than 8 bytes
+ * <li>null bitmap is skipped if all fields are primitive / not-nullable
+ * <li>the header is packed better, with the null-bitmap allowed to borrow alignment padding at
+ * end of fixed section
+ * <li>data alignment is relaxed, which could lead to less performance in very intensive memory
+ * operations
+ * </ul>
+ *
+ * <b>The compact format is still under development and may not be stable yet.</b>
+ */
+public class CompactBinaryRow extends BinaryRow {
+ private final boolean allFieldsNotNullable;
+ private final int[] fixedOffsets;
+ private final int bitmapOffset;
+
+ public CompactBinaryRow(final Schema schema) {
+ this(schema, CompactBinaryRowWriter.fixedOffsets(schema));
+ }
+
+ public CompactBinaryRow(final Schema schema, final int[] fixedOffsets) {
+ super(schema);
+ this.fixedOffsets = fixedOffsets;
+ bitmapOffset = fixedOffsets[fixedOffsets.length - 1];
+ allFieldsNotNullable = CompactBinaryRowWriter.allNotNullable(schema.getFields());
+ }
+
+ @Override
+ protected int computeBitmapWidthInBytes() {
+ // cannot use field due to initialization order
+ if (CompactBinaryRowWriter.allNotNullable(schema.getFields())) {
+ return 0;
+ }
+ return CompactBinaryRowWriter.headerBytes(schema);
+ }
+
+ @Override
+ public boolean isNullAt(final int ordinal) {
+ if (allFieldsNotNullable) {
+ return false;
+ }
+ return super.isNullAt(ordinal);
+ }
+
+ // TODO: this should use StableValue once it's available
+ @Override
+ public int getOffset(final int ordinal) {
+ return baseOffset + fixedOffsets[ordinal];
+ }
+
+ @Override
+ public MemoryBuffer getBuffer(final int ordinal) {
+ final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
+ if (fixedWidthBinary >= 0) {
+ if (isNullAt(ordinal)) {
+ return null;
+ }
+ return getBuffer().slice(getOffset(ordinal), fixedWidthBinary);
+ } else {
+ return super.getBuffer(ordinal);
+ }
+ }
+
+ @Override
+ public byte[] getBinary(final int ordinal) {
+ final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
+ if (fixedWidthBinary >= 0) {
+ if (isNullAt(ordinal)) {
+ return null;
+ }
+ final byte[] bytes = new byte[fixedWidthBinary];
+ getBuffer().get(getOffset(ordinal), bytes, 0, fixedWidthBinary);
+ return bytes;
+ } else {
+ return super.getBinary(ordinal);
+ }
+ }
+
+ @Override
+ protected BinaryRow getStruct(final int ordinal, final Field field, final int extDataSlot) {
+ if (isNullAt(ordinal)) {
+ return null;
+ }
+ final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
+ if (fixedWidthBinary == -1) {
+ return super.getStruct(ordinal, field, extDataSlot);
+ }
+ if (extData[extDataSlot] == null) {
+ extData[extDataSlot] = DataTypes.createSchema(field);
+ }
+ final BinaryRow row = newRow((Schema) extData[extDataSlot]);
+ row.pointTo(getBuffer().slice(getOffset(ordinal), fixedWidthBinary), 0, fixedWidthBinary);
+ return row;
+ }
+
+ @Override
+ protected Schema newSchema(final Field field) {
+ return CompactBinaryRowWriter.sortSchema(super.newSchema(field));
+ }
+
+ @Override
+ protected BinaryRow newRow(final Schema schema) {
+ // TODO: avoid re-computing these offsets
+ return new CompactBinaryRow(schema, CompactBinaryRowWriter.fixedOffsets(schema));
+ }
+
+ @Override
+ protected BinaryArray newArray(final Field field) {
+ return new CompactBinaryArray(field);
+ }
+
+ @Override
+ protected BinaryMap newMap(final Field field) {
+ return new CompactBinaryMap(field);
+ }
+
+ @Override
+ protected int isNullBitmapOffset() {
Review Comment:
this looks like a method return `boolean`, how abouting naming it as `nullabilityBitmapOffset`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378835112
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BufferResettingMapEncoder.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BufferResettingMapEncoder<T> implements MapEncoder<T> {
Review Comment:
similiar naming for other encoders
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BaseMapEncoder.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BaseMapEncoder<M> implements MapEncoder<M> {
Review Comment:
How about `BinaryMapEncoder` to relfect the relationship with BinaryRow, BinaryArray naming in codebase
similiar naming for other encoders
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378834441
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BufferResettingMapEncoder.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BufferResettingMapEncoder<T> implements MapEncoder<T> {
Review Comment:
How about `BinaryMapEncoder` to relfect the relationship with BinaryRow, BinaryArray naming in codebase
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378761104
##########
java/fory-format/src/main/java/org/apache/fory/format/row/binary/CompactBinaryRow.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 org.apache.fory.format.row.binary;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.types.pojo.Schema;
+import org.apache.fory.format.row.binary.writer.CompactBinaryRowWriter;
+import org.apache.fory.format.type.DataTypes;
+import org.apache.fory.memory.MemoryBuffer;
+
+/**
+ * A compact version of {@link BinaryRow}. The compact encoding includes additional optimizations:
+ *
+ * <ul>
+ * <li>fixed size binary objects are stored in the fixed size section with no pointer needed
+ * <li>small values can take up fewer than 8 bytes
+ * <li>null bitmap is skipped if all fields are primitive / not-nullable
+ * <li>the header is packed better, with the null-bitmap allowed to borrow alignment padding at
+ * end of fixed section
+ * <li>data alignment is relaxed, which could lead to less performance in very intensive memory
+ * operations
+ * </ul>
+ *
+ * <b>The compact format is still under development and may not be stable yet.</b>
+ */
+public class CompactBinaryRow extends BinaryRow {
+ private final boolean allFieldsNotNullable;
+ private final int[] fixedOffsets;
+ private final int bitmapOffset;
+
+ public CompactBinaryRow(final Schema schema) {
+ this(schema, CompactBinaryRowWriter.fixedOffsets(schema));
+ }
+
+ public CompactBinaryRow(final Schema schema, final int[] fixedOffsets) {
+ super(schema);
+ this.fixedOffsets = fixedOffsets;
+ bitmapOffset = fixedOffsets[fixedOffsets.length - 1];
+ allFieldsNotNullable = CompactBinaryRowWriter.allNotNullable(schema.getFields());
+ }
+
+ @Override
+ protected int computeBitmapWidthInBytes() {
+ // cannot use field due to initialization order
+ if (CompactBinaryRowWriter.allNotNullable(schema.getFields())) {
+ return 0;
+ }
+ return CompactBinaryRowWriter.headerBytes(schema);
+ }
+
+ @Override
+ public boolean isNullAt(final int ordinal) {
+ if (allFieldsNotNullable) {
+ return false;
+ }
+ return super.isNullAt(ordinal);
+ }
+
+ // TODO: this should use StableValue once it's available
+ @Override
+ public int getOffset(final int ordinal) {
+ return baseOffset + fixedOffsets[ordinal];
+ }
+
+ @Override
+ public MemoryBuffer getBuffer(final int ordinal) {
+ final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
+ if (fixedWidthBinary >= 0) {
+ if (isNullAt(ordinal)) {
+ return null;
+ }
+ return getBuffer().slice(getOffset(ordinal), fixedWidthBinary);
+ } else {
+ return super.getBuffer(ordinal);
+ }
+ }
+
+ @Override
+ public byte[] getBinary(final int ordinal) {
+ final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
+ if (fixedWidthBinary >= 0) {
+ if (isNullAt(ordinal)) {
+ return null;
+ }
+ final byte[] bytes = new byte[fixedWidthBinary];
+ getBuffer().get(getOffset(ordinal), bytes, 0, fixedWidthBinary);
+ return bytes;
+ } else {
+ return super.getBinary(ordinal);
+ }
+ }
+
+ @Override
+ protected BinaryRow getStruct(final int ordinal, final Field field, final int extDataSlot) {
+ if (isNullAt(ordinal)) {
+ return null;
+ }
+ final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
+ if (fixedWidthBinary == -1) {
+ return super.getStruct(ordinal, field, extDataSlot);
+ }
+ if (extData[extDataSlot] == null) {
+ extData[extDataSlot] = DataTypes.createSchema(field);
+ }
+ final BinaryRow row = newRow((Schema) extData[extDataSlot]);
+ row.pointTo(getBuffer().slice(getOffset(ordinal), fixedWidthBinary), 0, fixedWidthBinary);
+ return row;
+ }
+
+ @Override
+ protected Schema newSchema(final Field field) {
+ return CompactBinaryRowWriter.sortSchema(super.newSchema(field));
+ }
+
+ @Override
+ protected BinaryRow newRow(final Schema schema) {
+ // TODO: avoid re-computing these offsets
+ return new CompactBinaryRow(schema, CompactBinaryRowWriter.fixedOffsets(schema));
+ }
+
+ @Override
+ protected BinaryArray newArray(final Field field) {
+ return new CompactBinaryArray(field);
+ }
+
+ @Override
+ protected BinaryMap newMap(final Field field) {
+ return new CompactBinaryMap(field);
+ }
+
+ @Override
+ protected int isNullBitmapOffset() {
Review Comment:
this looks like a method return `boolean`, how abouting naming it as `nullabilityBitmapOffset/nullBitmapOffset`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378802315
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/Encoder.java:
##########
@@ -30,11 +30,36 @@
*/
public interface Encoder<T> {
+ /** Decode a buffer with an embedded size. Variants without embedded size are not compatible. */
T decode(MemoryBuffer buffer);
+ /** Decode a buffer without an embedded size. Variants with embedded size are not compatible. */
T decode(byte[] bytes);
+ /** Decode a buffer without an embedded size. Variants with embedded size are not compatible. */
+ T decodeRemaining(MemoryBuffer buffer);
+
+ /** Encode to a buffer without embedded size. Variants with embedded size are not compatible. */
byte[] encode(T obj);
+ /**
+ * Encode to a buffer with an embedded size. Variants without embedded size are not compatible.
+ */
void encode(MemoryBuffer buffer, T obj);
+
+ /**
+ * Encode to a buffer without an embedded size. Variants with embedded size are not compatible.
+ * Returns number of bytes written to the buffer.
+ */
+ int bareEncode(MemoryBuffer buffer, T obj);
Review Comment:
Is it possible to merge `bareEncode` and `encode` into one method?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378841957
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BaseMapEncoder.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BaseMapEncoder<M> implements MapEncoder<M> {
Review Comment:
And since we have different kinds of encoder for same type, we'd better to add some javadoc to document the distinctions
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378834441
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BufferResettingMapEncoder.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BufferResettingMapEncoder<T> implements MapEncoder<T> {
Review Comment:
How about `BinaryMapEncoder` to relfect the relationship with BinaryRow, BinaryArray naming in codebase
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(java): introduce Compact Row Codec [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2414:
URL: https://github.com/apache/fory/pull/2414#discussion_r2378828258
##########
java/fory-format/src/main/java/org/apache/fory/format/encoder/BaseMapEncoder.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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 org.apache.fory.format.encoder;
+
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.fory.format.row.binary.BinaryArray;
+import org.apache.fory.format.row.binary.BinaryMap;
+import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
+import org.apache.fory.memory.MemoryBuffer;
+import org.apache.fory.memory.MemoryUtils;
+
+class BaseMapEncoder<M> implements MapEncoder<M> {
Review Comment:
`BaseXXX` looks like an abstract parent class, could we use a different name?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org