You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "lszskye (via GitHub)" <gi...@apache.org> on 2026/05/26 10:17:31 UTC
[PR] feat: introduce binary row format with reader/writer support [paimon-cpp]
lszskye opened a new pull request, #22:
URL: https://github.com/apache/paimon-cpp/pull/22
### Purpose
Introduce the binary row data format, compatible with Java Paimon's `BinaryRow` serialization format. This module provides efficient in-memory row representation with fixed-length and variable-length parts within a single `MemorySegment`.
Key components:
- **BinaryWriter**: Abstract interface defining the write protocol.
- **AbstractBinaryWriter**: Template-method base class implementing variable-length field writing (strings, bytes, decimals, timestamps, arrays, rows, maps) with auto-growing buffer management.
- **BinaryRowWriter**: Concrete writer for `BinaryRow`, supporting all Paimon data types.
- **BinaryRow**: Immutable binary row representation backed by `MemorySegment`. Implements `InternalRow` for reading all field types, supports null tracking via bit set, hashing, equality comparison, and copy operations.
- **BinaryDataReadUtils**: Static utilities for reading typed fields (Timestamp, Decimal, BinaryString, Array, Row, Map) from memory segments with proper offset/size decoding.
### Tests
- `BinaryRowTest`
- `BinaryRowWriterTest`
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce binary row format with reader/writer support [paimon-cpp]
Posted by "leaves12138 (via GitHub)" <gi...@apache.org>.
leaves12138 commented on code in PR #22:
URL: https://github.com/apache/paimon-cpp/pull/22#discussion_r3303526623
##########
src/paimon/common/data/abstract_binary_writer.cpp:
##########
@@ -0,0 +1,222 @@
+/*
+ * 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.
+ */
+
+#include "paimon/common/data/abstract_binary_writer.h"
+
+#include <cassert>
+#include <memory>
+#include <optional>
+
+#include "paimon/common/data/binary_array.h"
+#include "paimon/common/data/binary_map.h"
+#include "paimon/common/data/binary_row.h"
+#include "paimon/common/data/binary_section.h"
+#include "paimon/common/data/binary_string.h"
+#include "paimon/common/memory/memory_segment_utils.h"
+#include "paimon/data/decimal.h"
+#include "paimon/data/timestamp.h"
+#include "paimon/io/byte_order.h"
+#include "paimon/memory/bytes.h"
+#include "paimon/memory/memory_pool.h"
+
+namespace paimon {
+
+void AbstractBinaryWriter::WriteBytes(int32_t pos, const Bytes& bytes) {
+ int32_t len = bytes.size();
+ if (len <= BinarySection::MAX_FIX_PART_DATA_SIZE) {
+ WriteBytesToFixLenPart(&segment_, GetFieldOffset(pos), bytes, len);
+ } else {
+ WriteBytesToVarLenPart(pos, bytes, len);
+ }
+}
+
+void AbstractBinaryWriter::WriteString(int32_t pos, const BinaryString& input) {
+ int32_t len = input.GetSizeInBytes();
+ if (len <= BinarySection::MAX_FIX_PART_DATA_SIZE) {
+ auto bytes = Bytes::AllocateBytes(len, pool_);
+ MemorySegmentUtils::CopyToBytes({input.GetSegment()}, input.GetOffset(), bytes.get(), 0,
+ len);
+ WriteBytesToFixLenPart(&segment_, GetFieldOffset(pos), *bytes, len);
+ } else {
+ WriteSegmentToVarLenPart(pos, input.GetSegment(), input.GetOffset(), len);
+ }
+}
+
+void AbstractBinaryWriter::WriteBinary(int32_t pos, const Bytes& bytes) {
+ int32_t len = bytes.size();
+ if (len <= BinarySection::MAX_FIX_PART_DATA_SIZE) {
+ WriteBytesToFixLenPart(&segment_, GetFieldOffset(pos), bytes, len);
+ } else {
+ WriteBytesToVarLenPart(pos, bytes, len);
+ }
+}
+
+void AbstractBinaryWriter::WriteStringView(int32_t pos, const std::string_view& view) {
+ int32_t len = view.size();
+ if (len <= BinarySection::MAX_FIX_PART_DATA_SIZE) {
+ WriteBytesToFixLenPart(&segment_, GetFieldOffset(pos), view, len);
+ } else {
+ WriteBytesToVarLenPart(pos, view, len);
+ }
+}
+
+void AbstractBinaryWriter::WriteRow(int32_t pos, const BinaryRow& input) {
+ return WriteSegmentToVarLenPart(pos, input.GetSegment(), input.GetOffset(),
+ input.GetSizeInBytes());
+}
+
+void AbstractBinaryWriter::WriteArray(int32_t pos, const BinaryArray& input) {
+ return WriteSegmentToVarLenPart(pos, input.GetSegment(), input.GetOffset(),
+ input.GetSizeInBytes());
+}
+
+void AbstractBinaryWriter::WriteMap(int32_t pos, const BinaryMap& input) {
+ return WriteSegmentToVarLenPart(pos, input.GetSegment(), input.GetOffset(),
+ input.GetSizeInBytes());
+}
+void AbstractBinaryWriter::WriteDecimal(int32_t pos, const std::optional<Decimal>& value,
+ int32_t precision) {
+ assert(value == std::nullopt || precision == value.value().Precision());
+ if (Decimal::IsCompact(precision)) {
+ assert(value != std::nullopt);
+ WriteLong(pos, value.value().ToUnscaledLong());
+ } else {
+ // grow the global buffer before writing data.
+ EnsureCapacity(16);
+ // zero-out 16 bytes
+ segment_.PutValue<int64_t>(cursor_, 0ll);
+ segment_.PutValue<int64_t>(cursor_ + 8, 0ll);
+
+ // Make sure Decimal object has the same scale as DecimalType.
+ // Note that we may pass in null Decimal object to set null for it.
+ if (value == std::nullopt) {
+ SetNullBit(pos);
+ SetOffsetAndSize(pos, cursor_, 0l);
+ } else {
+ auto bytes = value.value().ToUnscaledBytes();
+ segment_.Put(cursor_, bytes, 0, bytes.size());
+ SetOffsetAndSize(pos, cursor_, bytes.size());
+ }
+ // move the cursor forward.
+ cursor_ += 16;
+ }
+}
+
+void AbstractBinaryWriter::WriteTimestamp(int32_t pos, const std::optional<Timestamp>& value,
+ int32_t precision) {
+ if (Timestamp::IsCompact(precision)) {
+ assert(value != std::nullopt);
+ WriteLong(pos, value.value().GetMillisecond());
+ } else {
+ // store the nanoOfMillisecond in fixed-length part as offset and nanoOfMillisecond
+ EnsureCapacity(8);
+
+ if (value == std::nullopt) {
+ SetNullBit(pos);
+ // zero-out the bytes
+ segment_.PutValue<int64_t>(cursor_, 0l);
+ SetOffsetAndSize(pos, cursor_, 0l);
+ } else {
+ segment_.PutValue<int64_t>(cursor_, value.value().GetMillisecond());
+ SetOffsetAndSize(pos, cursor_, value.value().GetNanoOfMillisecond());
+ }
+ cursor_ += 8;
+ }
+}
+
+void AbstractBinaryWriter::ZeroOutPaddingBytes(int32_t num_bytes) {
+ if ((num_bytes & 0x07) > 0) {
+ segment_.PutValue<int64_t>(cursor_ + ((num_bytes >> 3) << 3), 0L);
+ }
+}
+
+void AbstractBinaryWriter::EnsureCapacity(int32_t needed_size) {
+ const int32_t length = cursor_ + needed_size;
+ if (segment_.Size() < length) {
+ Grow(length);
+ }
+}
+
+void AbstractBinaryWriter::WriteSegmentToVarLenPart(int32_t pos, const MemorySegment& segment,
+ int32_t offset, int32_t size) {
+ const int32_t rounded_size = RoundNumberOfBytesToNearestWord(size);
+ // grow the global buffer before writing data.
+ EnsureCapacity(rounded_size);
+ ZeroOutPaddingBytes(size);
+
+ segment.CopyTo(offset, &segment_, cursor_, size);
+ SetOffsetAndSize(pos, cursor_, size);
+ // move the cursor forward.
+ cursor_ += rounded_size;
+}
+
+template <typename T>
+void AbstractBinaryWriter::WriteBytesToVarLenPart(int32_t pos, const T& bytes, int32_t len) {
+ const int32_t rounded_size = RoundNumberOfBytesToNearestWord(len);
+ // grow the global buffer before writing data.
+ EnsureCapacity(rounded_size);
+ ZeroOutPaddingBytes(len);
+ // Write the bytes to the variable length portion.
+ segment_.Put(cursor_, bytes, 0, len);
+ SetOffsetAndSize(pos, cursor_, len);
+ // move the cursor forward.
+ cursor_ += rounded_size;
+}
+
+void AbstractBinaryWriter::Grow(int32_t min_capacity) {
+ int32_t old_capacity = segment_.Size();
+ int32_t new_capacity = old_capacity + (old_capacity >> 1);
+ if (new_capacity - min_capacity < 0) {
+ new_capacity = min_capacity;
+ }
+ std::shared_ptr<Bytes> new_bytes =
+ Bytes::CopyOf(*(segment_.GetOrCreateHeapMemory(pool_)), new_capacity, pool_);
+ segment_ = MemorySegment::Wrap(new_bytes);
+ AfterGrow();
+}
+
+int32_t AbstractBinaryWriter::RoundNumberOfBytesToNearestWord(int32_t num_bytes) {
+ int32_t remainder = num_bytes & 0x07;
+ if (remainder == 0) {
+ return num_bytes;
+ } else {
+ return num_bytes + (8 - remainder);
+ }
+}
+template <typename T>
+void AbstractBinaryWriter::WriteBytesToFixLenPart(MemorySegment* segment, int32_t field_offset,
+ const T& bytes, int32_t len) {
+ int64_t first_byte = len | 0x80; // first bit is 1, other bits is len
+ int64_t seven_bytes = 0L; // real data
+ if ((SystemByteOrder() == ByteOrder::PAIMON_LITTLE_ENDIAN)) {
+ for (int32_t i = 0; i < len; i++) {
+ seven_bytes |= ((0x00000000000000FFL & bytes[i]) << (i * 8L));
+ }
+ } else {
+ for (int32_t i = 0; i < len; i++) {
+ seven_bytes |= ((0x00000000000000FFL & bytes[i]) << ((6 - i) * 8L));
+ }
+ }
+ const int64_t offset_and_size =
+ (first_byte << 56) | // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult)
Review Comment:
This inline-string/binary encoding still has undefined behavior. `first_byte` is an `int64_t`; for inline values `len | 0x80` sets bit 7, so shifting it left by 56 tries to produce the signed sign bit value (`0x80 << 56`), which is outside the representable range of `int64_t`. Please build this field with `uint64_t` (similar to `BinarySection::HIGHEST_FIRST_BIT`) and cast back only when storing it.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce binary row format with reader/writer support [paimon-cpp]
Posted by "leaves12138 (via GitHub)" <gi...@apache.org>.
leaves12138 commented on code in PR #22:
URL: https://github.com/apache/paimon-cpp/pull/22#discussion_r3309019615
##########
src/paimon/common/data/binary_row_writer.cpp:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.
+ */
+
+#include "paimon/common/data/binary_row_writer.h"
+
+#include <cassert>
+#include <optional>
+#include <string>
+#include <string_view>
+#include <utility>
+
+#include "arrow/type.h"
+#include "arrow/util/checked_cast.h"
+#include "fmt/format.h"
+#include "paimon/common/data/binary_string.h"
+#include "paimon/common/memory/memory_segment.h"
+#include "paimon/common/utils/date_time_utils.h"
+#include "paimon/data/decimal.h"
+#include "paimon/data/timestamp.h"
+#include "paimon/memory/bytes.h"
+#include "paimon/memory/memory_pool.h"
+#include "paimon/status.h"
+
+namespace paimon {
+
+BinaryRowWriter::BinaryRowWriter(BinaryRow* row, int32_t initial_size, MemoryPool* pool)
+ : null_bits_size_in_bytes_(BinaryRow::CalculateBitSetWidthInBytes(row->GetFieldCount())),
+ fixed_size_(row->GetFixedLengthPartSize()) {
+ cursor_ = fixed_size_;
+ row_ = row;
+ segment_ = MemorySegment::Wrap(Bytes::AllocateBytes(fixed_size_ + initial_size, pool));
+ row_->PointTo(segment_, 0, segment_.Size());
+ pool_ = pool;
+}
+
+Result<BinaryRowWriter::FieldSetterFunc> BinaryRowWriter::CreateFieldSetter(
+ int32_t field_idx, const std::shared_ptr<arrow::DataType>& field_type) {
+ arrow::Type::type type = field_type->id();
+ BinaryRowWriter::FieldSetterFunc field_setter;
+ switch (type) {
+ case arrow::Type::type::BOOL: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteBoolean(field_idx, DataDefine::GetVariantValue<bool>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT8: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteByte(field_idx, DataDefine::GetVariantValue<char>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT16: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteShort(field_idx, DataDefine::GetVariantValue<int16_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT32: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteInt(field_idx, DataDefine::GetVariantValue<int32_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT64: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteLong(field_idx, DataDefine::GetVariantValue<int64_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::FLOAT: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteFloat(field_idx, DataDefine::GetVariantValue<float>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::DOUBLE: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteDouble(field_idx, DataDefine::GetVariantValue<double>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::DATE32: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteInt(field_idx, DataDefine::GetVariantValue<int32_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::STRING: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ const auto* view = DataDefine::GetVariantPtr<std::string_view>(field);
+ if (view) {
+ return writer->WriteStringView(field_idx, *view);
+ }
+ return writer->WriteString(field_idx,
+ DataDefine::GetVariantValue<BinaryString>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::BINARY: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ const auto* view = DataDefine::GetVariantPtr<std::string_view>(field);
+ if (view) {
+ return writer->WriteStringView(field_idx, *view);
+ }
+ return writer->WriteBinary(
+ field_idx, *DataDefine::GetVariantValue<std::shared_ptr<Bytes>>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::TIMESTAMP: {
+ auto timestamp_type =
+ arrow::internal::checked_pointer_cast<arrow::TimestampType>(field_type);
+ int32_t precision = DateTimeUtils::GetPrecisionFromType(timestamp_type);
+ field_setter = [field_idx, precision](const VariantType& field,
+ BinaryRowWriter* writer) -> void {
+ if (DataDefine::IsVariantNull(field)) {
+ if (!Timestamp::IsCompact(precision)) {
+ writer->WriteTimestamp(field_idx, std::nullopt, precision);
+ } else {
+ writer->SetNullAt(field_idx);
+ }
+ return;
+ }
+ return writer->WriteTimestamp(
+ field_idx, DataDefine::GetVariantValue<Timestamp>(field), precision);
+ };
+ return field_setter;
+ }
+ case arrow::Type::type::DECIMAL: {
+ auto* decimal_type =
+ arrow::internal::checked_cast<arrow::Decimal128Type*>(field_type.get());
+ assert(decimal_type);
+ auto precision = decimal_type->precision();
+ field_setter = [field_idx, precision](const VariantType& field,
Review Comment:
For non-compact decimals, the setter captures only precision and then calls `WriteDecimal` with a `Decimal` value whose scale may differ from the Arrow field scale. `WriteDecimal` currently only asserts the precision and never checks or normalizes scale, but the reader reconstructs decimals with the schema scale. That means a value like `Decimal(5, 3, 123)` written to a `decimal128(5, 2)` field will be read back as `1.23` instead of the original `0.123` (or being rejected). Please capture the Arrow scale here and either validate `value.Scale() == scale` or rescale/reject before writing, so the stored unscaled bytes match the schema scale.
##########
src/paimon/common/data/binary_row_writer.cpp:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.
+ */
+
+#include "paimon/common/data/binary_row_writer.h"
+
+#include <cassert>
+#include <optional>
+#include <string>
+#include <string_view>
+#include <utility>
+
+#include "arrow/type.h"
+#include "arrow/util/checked_cast.h"
+#include "fmt/format.h"
+#include "paimon/common/data/binary_string.h"
+#include "paimon/common/memory/memory_segment.h"
+#include "paimon/common/utils/date_time_utils.h"
+#include "paimon/data/decimal.h"
+#include "paimon/data/timestamp.h"
+#include "paimon/memory/bytes.h"
+#include "paimon/memory/memory_pool.h"
+#include "paimon/status.h"
+
+namespace paimon {
+
+BinaryRowWriter::BinaryRowWriter(BinaryRow* row, int32_t initial_size, MemoryPool* pool)
+ : null_bits_size_in_bytes_(BinaryRow::CalculateBitSetWidthInBytes(row->GetFieldCount())),
+ fixed_size_(row->GetFixedLengthPartSize()) {
+ cursor_ = fixed_size_;
+ row_ = row;
+ segment_ = MemorySegment::Wrap(Bytes::AllocateBytes(fixed_size_ + initial_size, pool));
+ row_->PointTo(segment_, 0, segment_.Size());
+ pool_ = pool;
+}
+
+Result<BinaryRowWriter::FieldSetterFunc> BinaryRowWriter::CreateFieldSetter(
+ int32_t field_idx, const std::shared_ptr<arrow::DataType>& field_type) {
+ arrow::Type::type type = field_type->id();
+ BinaryRowWriter::FieldSetterFunc field_setter;
+ switch (type) {
+ case arrow::Type::type::BOOL: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteBoolean(field_idx, DataDefine::GetVariantValue<bool>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT8: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteByte(field_idx, DataDefine::GetVariantValue<char>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT16: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteShort(field_idx, DataDefine::GetVariantValue<int16_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT32: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteInt(field_idx, DataDefine::GetVariantValue<int32_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT64: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteLong(field_idx, DataDefine::GetVariantValue<int64_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::FLOAT: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteFloat(field_idx, DataDefine::GetVariantValue<float>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::DOUBLE: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteDouble(field_idx, DataDefine::GetVariantValue<double>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::DATE32: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteInt(field_idx, DataDefine::GetVariantValue<int32_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::STRING: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ const auto* view = DataDefine::GetVariantPtr<std::string_view>(field);
+ if (view) {
+ return writer->WriteStringView(field_idx, *view);
+ }
+ return writer->WriteString(field_idx,
+ DataDefine::GetVariantValue<BinaryString>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::BINARY: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ const auto* view = DataDefine::GetVariantPtr<std::string_view>(field);
+ if (view) {
+ return writer->WriteStringView(field_idx, *view);
+ }
+ return writer->WriteBinary(
+ field_idx, *DataDefine::GetVariantValue<std::shared_ptr<Bytes>>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::TIMESTAMP: {
+ auto timestamp_type =
+ arrow::internal::checked_pointer_cast<arrow::TimestampType>(field_type);
+ int32_t precision = DateTimeUtils::GetPrecisionFromType(timestamp_type);
+ field_setter = [field_idx, precision](const VariantType& field,
+ BinaryRowWriter* writer) -> void {
+ if (DataDefine::IsVariantNull(field)) {
+ if (!Timestamp::IsCompact(precision)) {
+ writer->WriteTimestamp(field_idx, std::nullopt, precision);
+ } else {
+ writer->SetNullAt(field_idx);
+ }
+ return;
+ }
+ return writer->WriteTimestamp(
+ field_idx, DataDefine::GetVariantValue<Timestamp>(field), precision);
+ };
+ return field_setter;
+ }
+ case arrow::Type::type::DECIMAL: {
Review Comment:
This branch still will not handle `arrow::decimal128(...)`. In Arrow C++, decimal128 reports `arrow::Type::DECIMAL128` (the rest of this codebase also switches on `DECIMAL128`, e.g. `DataDefine::VariantValueToLiteral` and `BinaryArrayWriter::GetElementSize` tests). As written, `CreateFieldSetter(arrow::decimal128(...))` falls through to the unsupported-type error, so the decimal cases added in `binary_row_writer_test.cpp` cannot work in a real build. Please switch this to `DECIMAL128` (and add DECIMAL256 handling only if intended).
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce binary row format with reader/writer support [paimon-cpp]
Posted by "leaves12138 (via GitHub)" <gi...@apache.org>.
leaves12138 merged PR #22:
URL: https://github.com/apache/paimon-cpp/pull/22
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce binary row format with reader/writer support [paimon-cpp]
Posted by "leaves12138 (via GitHub)" <gi...@apache.org>.
leaves12138 commented on code in PR #22:
URL: https://github.com/apache/paimon-cpp/pull/22#discussion_r3308203529
##########
src/paimon/common/data/binary_row.cpp:
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.
+ */
+
+#include "paimon/common/data/binary_row.h"
+
+#include <cstdint>
+
+#include "fmt/format.h"
+#include "paimon/common/data/binary_data_read_utils.h"
+#include "paimon/common/memory/memory_segment.h"
+#include "paimon/common/memory/memory_segment_utils.h"
+#include "paimon/io/byte_order.h"
+#include "paimon/memory/bytes.h"
+#include "paimon/memory/memory_pool.h"
+
+namespace paimon {
+const int64_t BinaryRow::FIRST_BYTE_ZERO =
+ (SystemByteOrder() == ByteOrder::PAIMON_LITTLE_ENDIAN) ? (~0xFFL) : (~(0xFFL << 56L));
Review Comment:
This still has undefined behavior on the big-endian branch: is signed, and cannot be represented as a signed . Please build this mask with an unsigned value first, e.g. , or keep the mask as until the final cast.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce binary row format with reader/writer support [paimon-cpp]
Posted by "leaves12138 (via GitHub)" <gi...@apache.org>.
leaves12138 commented on code in PR #22:
URL: https://github.com/apache/paimon-cpp/pull/22#discussion_r3315133379
##########
src/paimon/common/data/binary_row_writer.cpp:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.
+ */
+
+#include "paimon/common/data/binary_row_writer.h"
+
+#include <cassert>
+#include <optional>
+#include <string>
+#include <string_view>
+#include <utility>
+
+#include "arrow/type.h"
+#include "arrow/util/checked_cast.h"
+#include "fmt/format.h"
+#include "paimon/common/data/binary_string.h"
+#include "paimon/common/memory/memory_segment.h"
+#include "paimon/common/utils/date_time_utils.h"
+#include "paimon/data/decimal.h"
+#include "paimon/data/timestamp.h"
+#include "paimon/memory/bytes.h"
+#include "paimon/memory/memory_pool.h"
+#include "paimon/status.h"
+
+namespace paimon {
+
+BinaryRowWriter::BinaryRowWriter(BinaryRow* row, int32_t initial_size, MemoryPool* pool)
+ : null_bits_size_in_bytes_(BinaryRow::CalculateBitSetWidthInBytes(row->GetFieldCount())),
+ fixed_size_(row->GetFixedLengthPartSize()) {
+ cursor_ = fixed_size_;
+ row_ = row;
+ segment_ = MemorySegment::Wrap(Bytes::AllocateBytes(fixed_size_ + initial_size, pool));
+ row_->PointTo(segment_, 0, segment_.Size());
+ pool_ = pool;
+}
+
+Result<BinaryRowWriter::FieldSetterFunc> BinaryRowWriter::CreateFieldSetter(
+ int32_t field_idx, const std::shared_ptr<arrow::DataType>& field_type) {
+ arrow::Type::type type = field_type->id();
+ BinaryRowWriter::FieldSetterFunc field_setter;
+ switch (type) {
+ case arrow::Type::type::BOOL: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteBoolean(field_idx, DataDefine::GetVariantValue<bool>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT8: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteByte(field_idx, DataDefine::GetVariantValue<char>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT16: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteShort(field_idx, DataDefine::GetVariantValue<int16_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT32: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteInt(field_idx, DataDefine::GetVariantValue<int32_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::INT64: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteLong(field_idx, DataDefine::GetVariantValue<int64_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::FLOAT: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteFloat(field_idx, DataDefine::GetVariantValue<float>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::DOUBLE: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteDouble(field_idx, DataDefine::GetVariantValue<double>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::DATE32: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ return writer->WriteInt(field_idx, DataDefine::GetVariantValue<int32_t>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::STRING: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ const auto* view = DataDefine::GetVariantPtr<std::string_view>(field);
+ if (view) {
+ return writer->WriteStringView(field_idx, *view);
+ }
+ return writer->WriteString(field_idx,
+ DataDefine::GetVariantValue<BinaryString>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::BINARY: {
+ field_setter = [field_idx](const VariantType& field, BinaryRowWriter* writer) -> void {
+ const auto* view = DataDefine::GetVariantPtr<std::string_view>(field);
+ if (view) {
+ return writer->WriteStringView(field_idx, *view);
+ }
+ return writer->WriteBinary(
+ field_idx, *DataDefine::GetVariantValue<std::shared_ptr<Bytes>>(field));
+ };
+ break;
+ }
+ case arrow::Type::type::TIMESTAMP: {
+ auto timestamp_type =
+ arrow::internal::checked_pointer_cast<arrow::TimestampType>(field_type);
+ int32_t precision = DateTimeUtils::GetPrecisionFromType(timestamp_type);
+ field_setter = [field_idx, precision](const VariantType& field,
+ BinaryRowWriter* writer) -> void {
+ if (DataDefine::IsVariantNull(field)) {
+ if (!Timestamp::IsCompact(precision)) {
+ writer->WriteTimestamp(field_idx, std::nullopt, precision);
+ } else {
+ writer->SetNullAt(field_idx);
+ }
+ return;
+ }
+ return writer->WriteTimestamp(
+ field_idx, DataDefine::GetVariantValue<Timestamp>(field), precision);
+ };
+ return field_setter;
+ }
+ case arrow::Type::type::DECIMAL: {
Review Comment:
Correction: this comment was inaccurate. Arrow C++ defines `arrow::Type::DECIMAL` as a compatibility alias of `arrow::Type::DECIMAL128`, so `arrow::decimal128(...)` is handled by this `case arrow::Type::DECIMAL` branch. Please ignore this comment; no enum change is needed here.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce binary row format with reader/writer support [paimon-cpp]
Posted by "leaves12138 (via GitHub)" <gi...@apache.org>.
leaves12138 commented on PR #22:
URL: https://github.com/apache/paimon-cpp/pull/22#issuecomment-4552602854
Re-reviewed the current head (`94b3929`). The two blocking issues I found are already listed in my latest review comments: the decimal setter switches on the wrong Arrow decimal enum, and non-compact decimal writes do not validate/normalize scale. I did not find additional blockers in this pass.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org