lxy-9602 commented on code in PR #222: URL: https://github.com/apache/paimon-cpp/pull/222#discussion_r3819753651
########## include/paimon/table/format/format_data_split.h: ########## @@ -0,0 +1,103 @@ +/* + * 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. + */ + +#pragma once + +#include <cstdint> +#include <limits> +#include <map> +#include <string> +#include <vector> + +#include "paimon/table/source/split.h" +#include "paimon/visibility.h" + +namespace paimon { + +/// A split of a format table: the data files of one partition directory, or of the table +/// directory itself when the table is not partitioned. +/// +/// Every file is held whole, since parquet and orc each record where their own row groups and +/// stripes begin and a reader handed a byte range of one would have to find that out for itself. +/// The partition is carried on the split rather than read from the files: a Hive-style layout +/// keeps partition values in the directory names. +class PAIMON_EXPORT FormatDataSplit : public Split { + public: + /// Frame of `Split::Serialize` for a format table split. This encoding is paimon-cpp's own + /// and has no cross-runtime counterpart, so these bytes travel between paimon-cpp processes + /// only. + static constexpr int64_t MAGIC = -8172530964192837451L; + static constexpr int32_t VERSION = 1; + + /// One data file of the split. + struct PAIMON_EXPORT FileMeta { + FileMeta(const std::string& _file_path, int64_t _file_size) + : file_path(_file_path), file_size(_file_size) {} + + bool operator==(const FileMeta& other) const { + return file_path == other.file_path && file_size == other.file_size; + } + + /// Bytes this entry accounts for. + int64_t ReadSize() const { + return file_size; + } Review Comment: As a `struct`, this probably doesn’t need getters for its members. ########## include/paimon/table/format/format_data_split.h: ########## @@ -0,0 +1,103 @@ +/* + * 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. + */ + +#pragma once + +#include <cstdint> +#include <limits> +#include <map> +#include <string> +#include <vector> + +#include "paimon/table/source/split.h" +#include "paimon/visibility.h" + +namespace paimon { + +/// A split of a format table: the data files of one partition directory, or of the table +/// directory itself when the table is not partitioned. +/// +/// Every file is held whole, since parquet and orc each record where their own row groups and +/// stripes begin and a reader handed a byte range of one would have to find that out for itself. +/// The partition is carried on the split rather than read from the files: a Hive-style layout +/// keeps partition values in the directory names. +class PAIMON_EXPORT FormatDataSplit : public Split { Review Comment: Do we really need to expose the include/format layer in public headers? My understanding is that components like data split, table read/write/scan/commit could all stay internal under src. ########## include/paimon/reader/batch_reader.h: ########## @@ -47,9 +47,10 @@ class PAIMON_EXPORT BatchReader { /// avoid potential issues during conversion through the Arrow C Data Interface. /// /// @return A result containing a `::ReadBatch`, which consists of a unique pointer to - /// `ArrowArray` and a unique pointer to `ArrowSchema`. Returned array contains a `_VALUE_KIND` - /// field (the first field) to indicate the row kind of each row. Deleted or index-filtered rows - /// are removed. + /// `ArrowArray` and a unique pointer to `ArrowSchema`. A reader over a table that records row + /// kinds puts a `_VALUE_KIND` field first to carry them; one over a table where every row is + /// an insert, such as a format table, has no such field. Deleted or index-filtered rows are + /// removed. virtual Result<ReadBatch> NextBatch() = 0; Review Comment: Please do not change the semantics of `NextBatch`. Many upstream engines access fields by index, so removing the `_VALUE_KIND` column could shift the field positions and lead to incorrect results. If format tables always contain inserts, we can use `CompleteRowKindBatchReader` to add `_VALUE_KIND` back before returning the result. ########## include/paimon/table/format/format_table_read.h: ########## @@ -0,0 +1,109 @@ +/* + * 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. + */ + +#pragma once + +#include <memory> +#include <optional> +#include <string> +#include <vector> + +#include "paimon/memory/memory_pool.h" +#include "paimon/reader/batch_reader.h" +#include "paimon/result.h" +#include "paimon/table/format/format_table.h" +#include "paimon/table/source/split.h" +#include "paimon/table/source/table_read.h" +#include "paimon/visibility.h" + +namespace paimon { + +class Predicate; + +/// Reads the splits a `FormatTableScan` produced. +/// +/// The batches hold the table's own columns only, with no leading `_VALUE_KIND` field: a directory +/// of plain data files records no row kind and every row in it is an insert. Partition columns are +/// rebuilt from the split's partition values, since the data files do not carry them. +/// +/// A batch borrows memory from the reader that produced it, so every batch must be released before +/// that reader is destroyed. +/// +/// Building a reader leaves the read as it was, so one may be shared between threads; the +/// `BatchReader`s it hands out may not be, as `TableRead` says. +/// +/// `CreateCountReader()` is not implemented and falls through to `TableRead`'s default, which +/// refuses: counting a format table's rows means reading them. +class PAIMON_EXPORT FormatTableRead : public TableRead { + public: + /// @param table Table the splits belong to. + /// @param projection Names of the columns to read, in the order they should appear. When + /// absent, every column of the table is read. A column named twice is rejected. + /// @param limit Upper bound on the rows to return across all splits of one reader. When + /// absent, every row is returned. + /// @param pool Memory pool the batches are allocated from. + /// @param predicate Rows the caller is interested in, as a filter over the table's columns. + /// It is pushed into the file readers so the format skips what its own statistics let + /// it skip; on its own that is a best effort and rows the predicate rejects can still + /// come back. Every field it names must be a column of the table, of the type the + /// conjunct declares, with literals of that same type and none of them null. The field + /// index a conjunct carries is not read - a field is resolved by name - so a predicate + /// built against the table stays valid under any projection. + /// @param enable_predicate_filter Whether the returned reader applies `predicate` exactly to + /// the rows it returns. Off by default, as everywhere else in paimon-cpp. With it on, + /// `predicate` may only name columns the projection keeps, since a column the reader + /// does not produce cannot be tested. + static Result<std::unique_ptr<FormatTableRead>> Create( + const std::shared_ptr<FormatTable>& table, + const std::optional<std::vector<std::string>>& projection = std::nullopt, + const std::optional<int32_t>& limit = std::nullopt, + const std::shared_ptr<MemoryPool>& pool = nullptr, + const std::shared_ptr<Predicate>& predicate = nullptr, + bool enable_predicate_filter = false); Review Comment: A small note: if this is moved into a src file later, please remove the default arguments. ########## include/paimon/table/source/split.h: ########## @@ -34,9 +34,12 @@ namespace paimon { class MemoryPool; /// An input split for reading operation. Needed by most batch computation engines. Support -/// Serialize and Deserialize, compatible with java version. -/// This split can be either a `DataSplit` (for direct data file reads) or an `IndexedSplit` -/// (for reads leveraging global indexes). +/// Serialize and Deserialize. +/// +/// This split can be a `DataSplit` (for direct data file reads), an `IndexedSplit` (for reads +/// leveraging global indexes), or a `FormatDataSplit` (for a format table's plain data files). +/// The first two use the cross-runtime encoding; `FormatDataSplit` has none, so it round-trips +/// only here. Review Comment: If the serialization protocol is not cross-language compatible, should we remove support for the serialization/deserialization interfaces for now? That would make it clear that the state is memory-only. Otherwise, in many cases we may have C++ writing while Java commits or scans, and then C++ reads again—could that cause problems? ########## src/paimon/common/reader/predicate_batch_reader.cpp: ########## @@ -81,8 +81,16 @@ Result<BatchReader::ReadBatchWithBitmap> PredicateBatchReader::NextBatchWithBitm auto& [batch, bitmap] = batch_with_bitmap; auto& [c_array, c_schema] = batch; assert(c_array); - PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Array> array, - arrow::ImportArray(c_array.get(), c_schema.get())); + // Importing takes the batch over on success only. A `unique_ptr<ArrowArray>` frees the + // struct without releasing what it points at, so a failure here has to release the batch + // itself or the reader below is left holding memory nothing will ever hand back. + arrow::Result<std::shared_ptr<arrow::Array>> imported = + arrow::ImportArray(c_array.get(), c_schema.get()); + if (!imported.ok()) { + ReaderUtils::ReleaseReadBatch(std::move(batch)); + return ToPaimonStatus(imported.status()); + } + std::shared_ptr<arrow::Array> array = std::move(imported).ValueUnsafe(); Review Comment: Was this issue found in testing? It doesn’t seem like this should be a real problem. We use this pattern in many places, and `arrow::ImportArray(array, schema)` also explicitly guarantees that schema will be released on failure. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
