lxy-9602 commented on code in PR #198:
URL: https://github.com/apache/paimon-cpp/pull/198#discussion_r3788370161
##########
src/paimon/core/schema/table_schema.cpp:
##########
@@ -118,6 +118,14 @@ Result<std::shared_ptr<arrow::Field>>
TableSchema::AssignFieldIdsRecursively(
/*set_field_id=*/false, field_id));
return arrow::field(field->name(), arrow::list(new_value_field),
field->nullable(),
metadata);
+ } else if (type->id() == arrow::Type::FIXED_SIZE_LIST) {
+ auto vector_type =
std::static_pointer_cast<arrow::FixedSizeListType>(field->type());
+ PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<arrow::Field> new_value_field,
+
AssignFieldIdsRecursively(vector_type->value_field(),
+
/*set_field_id=*/false, field_id));
+ return arrow::field(field->name(),
+ arrow::fixed_size_list(new_value_field,
vector_type->list_size()),
Review Comment:
I found a blocker.
### [Blocking] Reject VECTOR fields in primary-key tables until merge-tree
support is implemented
This PR keeps VECTOR columns as Arrow `FixedSizeList` throughout the
framework and converts them to Parquet `LIST` only at the Parquet writer
boundary. Therefore, supporting VECTOR in primary-key tables requires the
merge-tree and row-based framework paths to understand `FIXED_SIZE_LIST`.
The current integration test only covers an append-only table. Primary-key
tables containing a VECTOR value column are currently accepted by schema
validation but fail in several deterministic places:
1. `InMemorySortBuffer::EstimateMemoryUse` has no `FIXED_SIZE_LIST` branch,
so a PK write fails while buffering the first batch.
2. `RowToArrowArrayConverter` does not support `FixedSizeListBuilder` in
`AppendField`, `Reserve`, or `Accumulate`. This blocks PK flush, compaction,
spill, and PK scan projection.
3. `ColumnarRow`, `ColumnarRowRef`, and `ColumnarArray` assume `GetArray`
always wraps an Arrow `ListArray`. They cannot expose values backed by a
`FixedSizeListArray`.
There are also missing branches in additional framework components:
- `InternalRow::CreateFieldGetter` does not recognize VECTOR.
- `BinarySerializerUtils::WriteBinaryArray` casts the type directly to
`arrow::ListType`.
- `RowCompactedSerializer` has no VECTOR reader or writer, affecting lookup
persistence.
- Partial-update and aggregation merge functions create field getters for
every value field and therefore cannot be initialized when the schema contains
VECTOR.
- `CastedRow` also lacks VECTOR handling. This one is not strictly PK-only
and may affect schema/stats evolution paths as well.
Most of the above work is specific to primary-key tables and can reasonably
be implemented in a follow-up PR. However, this PR must not expose a schema
configuration that is known to fail at runtime.
Please update `SchemaValidation::ValidateVectorFields` to reject any table
that both:
- contains a VECTOR field, including a nested VECTOR field; and
- has a non-empty primary-key definition.
For example:
```cpp
if (has_vector && !schema.PrimaryKeys().empty()) {
return Status::NotImplemented(
"VECTOR fields in primary-key tables are not implemented yet.");
}
```
The existing test only verifies that the VECTOR column itself cannot be used
as a primary key. Please also add a test for a schema such as:
```text
id BIGINT,
embedding VECTOR<FLOAT, 3>,
PRIMARY KEY (id)
```
and verify that schema validation rejects it.
The documentation should likewise state that the current implementation
supports VECTOR only in append-only Parquet tables. Full primary-key support,
including write buffering, row-to-column conversion, reads, compaction, spill,
lookup, and merge engines, can then be added and tested in a second PR.
##########
src/paimon/format/parquet/parquet_file_batch_reader.cpp:
##########
@@ -762,7 +783,7 @@ Status ParquetFileBatchReader::CollectLeafIndices(const
std::shared_ptr<arrow::D
void ParquetFileBatchReader::SkipLeafIndices(const
std::shared_ptr<arrow::DataType>& file_type,
int32_t* leaf_index) {
if (file_type->id() == arrow::Type::STRUCT || file_type->id() ==
arrow::Type::LIST ||
- file_type->id() == arrow::Type::MAP) {
Review Comment:
I looked into the Rust and Python implementations, and I think we could move
`ParquetVectorConverter::ConvertToReadType` — specifically the `list -> fixed
size list` conversion — up into the framework layer, and introduce a generic
reader wrapper on top of `ParquetFileBatchReader`.
My suggestion would be:
- convert the schema during `SetReadSchema`, and
- convert the data during `NextBatch`.
You could refer to `CompleteRowTrackingFieldsBatchReader` for a similar
pattern.
This would reduce the amount of change needed in the Parquet plugin layer.
Some external engines use their own Parquet plugin implementations, and ideally
we want plugin authors to make as few changes as possible when adding vector
support.
For the write path, could we just pass `fixed size list` directly to the
Parquet writer? In theory, Parquet does not distinguish between `fixed size
list` and `list`, and Arrow should be able to handle the conversion properly.
##########
src/paimon/format/parquet/parquet_file_batch_reader.cpp:
##########
@@ -65,6 +66,13 @@ class Predicate;
namespace paimon::parquet {
namespace {
+std::shared_ptr<arrow::DataType> GetListElementType(const
std::shared_ptr<arrow::DataType>& type) {
+ if (type->id() == arrow::Type::FIXED_SIZE_LIST) {
+ return static_cast<const
arrow::FixedSizeListType&>(*type).value_type();
+ }
+ return static_cast<const arrow::ListType&>(*type).value_type();
+}
Review Comment:
We recently made some conventions around casts. In the latest code, please
use `paimon::check_cast` instead of `static_cast` (for more details please
refer to `docs/code-style.md`).
##########
test/inte/write_and_read_inte_test.cpp:
##########
@@ -309,6 +309,66 @@ TEST_P(WriteAndReadInteTest, TestAppendSimple) {
ASSERT_TRUE(success);
}
+TEST_P(WriteAndReadInteTest, TestAppendVector) {
+ auto [file_format, file_system] = GetParam();
+ if (file_format != "parquet") {
+ return;
+ }
+
+ auto vector_type =
+ arrow::fixed_size_list(arrow::field("item", arrow::float32(),
/*nullable=*/false), 3);
+ arrow::FieldVector fields = {arrow::field("id", arrow::int32()),
+ arrow::field("embedding", vector_type)};
+ std::map<std::string, std::string> options = {
+ {Options::MANIFEST_FORMAT, "avro"}, {Options::FILE_FORMAT,
file_format},
+ {Options::TARGET_FILE_SIZE, "1024"}, {Options::BUCKET, "-1"},
+ {Options::FILE_SYSTEM, file_system},
+ };
+ if (file_system == "jindo") {
+ options = AddOptionsForJindo(options);
+ }
+ ASSERT_OK_AND_ASSIGN(auto helper,
+ TestHelper::Create(test_dir_, arrow::schema(fields),
/*partition_keys=*/{},
+ /*primary_keys=*/{}, options,
+ /*is_streaming_mode=*/false));
+ arrow::Int32Builder ids_builder;
+ ASSERT_TRUE(ids_builder.AppendValues({1, 2, 3}).ok());
+ std::shared_ptr<arrow::Array> ids;
Review Comment:
For the tests, could you please try to use JSON-style expressions to
construct the source or expected arrays, as they’re easier to read and
understand.
##########
src/paimon/common/types/data_type_json_parser.cpp:
##########
@@ -607,6 +615,34 @@ Result<std::shared_ptr<arrow::DataType>>
TokenParser::ParseTimestampLtzType() {
return ts_type;
}
+Result<std::shared_ptr<arrow::DataType>> TokenParser::ParseVectorType() {
+ PAIMON_RETURN_NOT_OK(NextToken(TokenType::BEGIN_SUBTYPE));
+ bool element_nullable = true;
+ AtomicTypeAttributes element_attributes;
+ PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<arrow::DataType> element_type,
+ ParseTypeWithNullability(&element_nullable,
&element_attributes));
+ if (element_attributes.is_blob || element_attributes.is_variant ||
+ !VectorType::IsValidElementType(element_type)) {
+ return Status::Invalid(
+ fmt::format("Invalid element type for vector: {}",
element_type->ToString()));
+ }
+ PAIMON_RETURN_NOT_OK(NextToken(TokenType::LIST_SEPARATOR));
+ PAIMON_RETURN_NOT_OK(NextToken(TokenType::LITERAL_INT));
+ const std::string& length_token = GetToken().value;
+ int64_t length = 0;
+ const auto [end, error] =
+ std::from_chars(length_token.data(), length_token.data() +
length_token.size(), length);
+ if (error != std::errc() || end != length_token.data() +
length_token.size() || length < 1 ||
+ length > std::numeric_limits<int32_t>::max()) {
+ return Status::Invalid(
+ fmt::format("Vector length must be between 1 and {} (both
inclusive), but was {}",
+ std::numeric_limits<int32_t>::max(), length_token));
+ }
Review Comment:
Could we use `StringUtils::StringToValue` 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
