wgtmac commented on code in PR #175:
URL: https://github.com/apache/iceberg-cpp/pull/175#discussion_r2280913359
##########
src/iceberg/manifest_entry.cc:
##########
@@ -29,13 +29,16 @@ namespace iceberg {
bool ManifestEntry::operator==(const ManifestEntry& other) const {
return status == other.status && snapshot_id == other.snapshot_id &&
- sequence_number == other.sequence_number &&
- file_sequence_number == other.file_sequence_number &&
- (data_file && other.data_file && *data_file == *other.data_file)
||
- (!data_file && !other.data_file);
+ sequence_number == other.sequence_number &&
+ file_sequence_number == other.file_sequence_number &&
+ ((data_file && other.data_file && *data_file == *other.data_file) ||
+ (!data_file && !other.data_file));
}
std::shared_ptr<StructType> DataFile::Type(std::shared_ptr<StructType>
partition_type) {
+ if (!partition_type) {
+ partition_type = std::make_shared<StructType>(std::vector<SchemaField>{});
Review Comment:
```suggestion
partition_type = PartitionSpec::Unpartitioned()->schema();
```
A more elegant way to fix this is like above. However, we need to fix that
as you can see that it is initialized with a nullptr too:
https://github.com/apache/iceberg-cpp/blob/a54f116211258f4199b5b1665494e9eb4bec45b2/src/iceberg/partition_spec.cc#L46-L52
##########
test/manifest_reader_test.cc:
##########
@@ -115,7 +116,88 @@ TEST_F(ManifestReaderTest, BasicTest) {
auto read_result = manifest_reader->Entries();
ASSERT_EQ(read_result.has_value(), true) << read_result.error().message;
- auto expected_entries = prepare_manifest_entries();
+ auto expected_entries = PrepareV1ManifestEntries();
+ ASSERT_EQ(read_result.value(), expected_entries);
+}
+
+class ManifestReaderV2Test : public TempFileTestBase {
+ protected:
+ static void SetUpTestSuite() { avro::AvroReader::Register(); }
+
+ void SetUp() override {
+ TempFileTestBase::SetUp();
+ local_fs_ = std::make_shared<::arrow::fs::LocalFileSystem>();
+ file_io_ =
std::make_shared<iceberg::arrow::ArrowFileSystemFileIO>(local_fs_);
+
+ avro::RegisterLogicalTypes();
+ }
+
+ std::vector<ManifestEntry> prepareV2NonPartitionedManifestEntries() {
Review Comment:
```suggestion
std::vector<ManifestEntry> PrepareV2NonPartitionedManifestEntries() {
```
##########
test/manifest_reader_test.cc:
##########
@@ -115,7 +116,88 @@ TEST_F(ManifestReaderTest, BasicTest) {
auto read_result = manifest_reader->Entries();
ASSERT_EQ(read_result.has_value(), true) << read_result.error().message;
- auto expected_entries = prepare_manifest_entries();
+ auto expected_entries = PrepareV1ManifestEntries();
+ ASSERT_EQ(read_result.value(), expected_entries);
+}
+
+class ManifestReaderV2Test : public TempFileTestBase {
+ protected:
+ static void SetUpTestSuite() { avro::AvroReader::Register(); }
+
+ void SetUp() override {
+ TempFileTestBase::SetUp();
+ local_fs_ = std::make_shared<::arrow::fs::LocalFileSystem>();
+ file_io_ =
std::make_shared<iceberg::arrow::ArrowFileSystemFileIO>(local_fs_);
+
+ avro::RegisterLogicalTypes();
+ }
+
+ std::vector<ManifestEntry> prepareV2NonPartitionedManifestEntries() {
+ std::vector<ManifestEntry> manifest_entries;
+ std::string test_dir_prefix =
"/tmp/db/db/v2_manifest_non_partitioned/data/";
Review Comment:
The two `db`s in the prefix looks weird. Is this something that can be fixed?
--
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]
