Repository navigation
Conversation
|
The shared-library builds failed to link because the Parquet reader in (AI-assisted reply.) |
When ReaderOptions::projection is not set, the Avro and Parquet readers now derive the read schema from the file schema instead of failing. Also export FromArrowSchema for the bundle library so shared builds can link it. Closes apache#514 Signed-off-by: Vincent <1729065730@qq.com>
fd253e1 to
bb8cd9f
Compare
|
Rebased onto current (AI-assisted reply.) |
|
Is there any human involved in this PR, or did your bot just randomly scan the issues and try to fix one? 😂 |
| reader_, ::parquet::arrow::FileReader::Make(pool_, std::move(file_reader), | ||
| arrow_reader_properties)); | ||
|
|
||
| ::parquet::arrow::SchemaManifest schema_manifest; |
There was a problem hiding this comment.
SchemaManifest is built twice, once here, once in BuildProjection, I think we can reduce one.
| return InvalidSchema( | ||
| "Array-backed map type must have a record node with two fields"); | ||
| } | ||
| SchemaField key(entry_fields[0].field_id(), MapType::kKeyName, |
There was a problem hiding this comment.
Do we need to validate the key is not optional? We had the following in ToAvroNodeVisitor::Visit(const MapType& type, ::avro::NodePtr* node), it would be better to mirror that here.
if (key_field.optional()) [[unlikely]] {
return InvalidArgument("Map key `{}` must be required", key_field.name());
}
| ::arrow::FieldVector fields; | ||
| fields.reserve(parquet_schema.schema_fields.size()); | ||
| for (const auto& parquet_field : parquet_schema.schema_fields) { | ||
| if (auto field = ToArrowFieldWithIds(parquet_field); field != nullptr) { |
There was a problem hiding this comment.
Fields without an ID are skipped here, but Avro fails on missing IDs, should we made them consistent?
|
I created this issue when I was implementing the avro/parquet readers and thought it would be nice feature to not explicitly provide the read schema and make benchmark much easiler. However, as the development goes on, a lot of complexity has been added to these readers and it is not a trivial work as we can see from this PR. I'm also concerned if the new functions added by this PR (which are error-prone) will be used by elsewhere in the future. So I'm inclined to close the issue as an invalid feature request. |
Closes #514.
What
ReaderOptions::projectionis no longer required for the Avro and Parquet readers. When it is not set, the reader derives the read schema from the file schema and reads all fields.FromAvroSchema(), which converts an Avro schema written by Iceberg (withfield-id,element-id,key-id/value-idand themaplogical type for non-string keys) back into an IcebergSchema. Sanitized field names are restored fromiceberg-field-name.FromParquetSchema(), which builds the schema from theSchemaManifest. Since it runs afterApplyNameMapping, files without field IDs work when a name mapping is provided. Fields that still have no ID (or lists/maps whose element/key/value has no ID) are skipped, because they can't be read by ID anyway. A file with neither field IDs nor a name mapping still fails as before.ParquetReadernow builds theSchemaManifestonce inOpen()and uses it for both the schema derivation and the projection.Tests
FromAvroSchemaTest: primitive types, nested struct/list/map (string and non-string keys), sanitized names, missing field ID and unsupported types.ParquetSchemaConversionTest: round trip throughToParquetSchemafor list and large_list, and conversion with a partial name mapping.avro_test,parquet_test,schema_test,data_test,scan_test,manifest_test,table_testandarrow_testpass locally.This PR was written with the help of an AI coding assistant. I reviewed the changes and tests myself.