wgtmac commented on code in PR #316:
URL: https://github.com/apache/iceberg-cpp/pull/316#discussion_r2523760638
##########
src/iceberg/json_internal.cc:
##########
@@ -917,10 +926,10 @@ Status ParsePartitionSpecs(const nlohmann::json& json,
int8_t format_version,
std::move(field->transform()));
}
- // TODO(Li Feiyang):use a new PartitionSpec::Make to find the source field
of each
- // partition field from schema and then verify it
- auto spec =
- std::make_unique<PartitionSpec>(PartitionSpec::kInitialSpecId,
std::move(fields));
+ // Create partition spec with schema validation
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto spec, PartitionSpec::Make(*current_schema,
PartitionSpec::kInitialSpecId,
+ std::move(fields), true
/*allow_missing_fields*/));
Review Comment:
```suggestion
std::move(fields),
/*allow_missing_fields=*/false));
```
ditto
##########
src/iceberg/json_internal.cc:
##########
@@ -542,9 +543,17 @@ Result<std::unique_ptr<PartitionSpec>>
PartitionSpecFromJson(
ICEBERG_ASSIGN_OR_RAISE(auto partition_field,
PartitionFieldFromJson(field_json));
partition_fields.push_back(std::move(*partition_field));
}
- // TODO(Li Feiyang):use a new PartitionSpec::Make to find the source field
of each
- // partition field from schema and then verify it
- return std::make_unique<PartitionSpec>(spec_id, std::move(partition_fields));
+
+ std::unique_ptr<PartitionSpec> spec;
+ if (default_spec_id == spec_id) {
+ ICEBERG_ASSIGN_OR_RAISE(
+ spec, PartitionSpec::Make(*schema, spec_id,
std::move(partition_fields),
+ true /*allow_missing_fields*/));
+ } else {
+ ICEBERG_ASSIGN_OR_RAISE(spec,
+ PartitionSpec::Make(spec_id,
std::move(partition_fields)));
Review Comment:
```suggestion
ICEBERG_ASSIGN_OR_RAISE(spec, PartitionSpec::Make(*schema, spec_id,
std::move(partition_fields), /*allow_missing_fields=*/true));
```
The Java code does not call `checkCompatibility` in this case but
`PartitionSpec.Builder.copyToBuilder(Schema schema)` is still called as below
where non-missing field has to validate its transform:
```java
private PartitionSpec.Builder copyToBuilder(Schema schema) {
PartitionSpec.Builder builder =
PartitionSpec.builderFor(schema).withSpecId(specId);
for (UnboundPartitionField field : fields) {
Type fieldType = schema.findType(field.sourceId);
Transform<?, ?> transform;
if (fieldType != null) {
transform = Transforms.fromString(fieldType,
field.transform.toString());
} else {
transform = Transforms.fromString(field.transform.toString());
}
if (field.partitionId != null) {
builder.add(field.sourceId, field.partitionId, field.name,
transform);
} else {
builder.add(field.sourceId, field.name, transform);
}
}
return builder;
}
```
##########
src/iceberg/json_internal.cc:
##########
@@ -542,9 +543,17 @@ Result<std::unique_ptr<PartitionSpec>>
PartitionSpecFromJson(
ICEBERG_ASSIGN_OR_RAISE(auto partition_field,
PartitionFieldFromJson(field_json));
partition_fields.push_back(std::move(*partition_field));
}
- // TODO(Li Feiyang):use a new PartitionSpec::Make to find the source field
of each
- // partition field from schema and then verify it
- return std::make_unique<PartitionSpec>(spec_id, std::move(partition_fields));
+
+ std::unique_ptr<PartitionSpec> spec;
+ if (default_spec_id == spec_id) {
+ ICEBERG_ASSIGN_OR_RAISE(
+ spec, PartitionSpec::Make(*schema, spec_id,
std::move(partition_fields),
+ true /*allow_missing_fields*/));
Review Comment:
```suggestion
spec, PartitionSpec::Make(*schema, spec_id,
std::move(partition_fields),
/*allow_missing_fields=*/false));
```
It is illegal for the current spec to have a missing source field. You can
verify it by checking the Java code below that `bind(schema)` is called.
```java
public PartitionSpec bind(Schema schema) {
return copyToBuilder(schema).build();
}
public PartitionSpec bind(Schema schema, boolean ignoreMissingFields) {
return copyToBuilder(schema).build(ignoreMissingFields);
}
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
