shangxinli commented on code in PR #1068:
URL: https://github.com/apache/iceberg-cpp/pull/1068#discussion_r4434014317
##########
src/iceberg/data/file_scan_task_reader.cc:
##########
@@ -29,23 +29,48 @@
#include "iceberg/data/delete_filter.h"
#include "iceberg/file_reader.h"
#include "iceberg/manifest/manifest_entry.h"
+#include "iceberg/partition_spec.h"
#include "iceberg/schema.h"
#include "iceberg/schema_internal.h"
#include "iceberg/table_scan.h"
+#include "iceberg/transform.h"
#include "iceberg/util/macros.h"
namespace iceberg {
namespace {
-ReaderOptions MakeReaderOptions(const FileScanTask& task,
std::shared_ptr<FileIO> io,
- std::shared_ptr<Schema> projection,
- std::shared_ptr<Expression> filter, bool
case_sensitive,
- std::shared_ptr<NameMapping> name_mapping,
- ReaderProperties properties,
- std::optional<int64_t> first_row_id,
- std::optional<int64_t> data_sequence_number) {
+Result<std::unordered_map<int32_t, Literal>> ConstantsOf(const FileScanTask&
task) {
+ const auto& file = *task.data_file();
+ const auto& spec = task.spec();
+ std::unordered_map<int32_t, Literal> id_to_constant;
+ if (!spec) {
+ if (file.partition.num_fields() != 0) {
+ return InvalidArgument("A partition spec is required to read partition
values");
+ }
+ return id_to_constant;
+ }
+ for (size_t i = 0; i < spec->fields().size(); ++i) {
+ const auto& field = spec->fields()[i];
+ if (field.transform() == nullptr) {
+ return InvalidArgument("Missing transform for partition field {}",
+ field.field_id());
+ }
+ if (field.transform()->transform_type() == TransformType::kIdentity) {
+ id_to_constant.emplace(field.source_id(), file.partition.values()[i]);
Review Comment:
file.partition.values()[i] uses span operator[] with no bounds check, but
PartitionValues::ValueAt already exists for this exact case and returns an
invalid argument error if the data file's partition value count doesn't match
the spec field count. worth using ValueAt here to avoid undefined behavior on a
mismatched partition tuple.
##########
src/iceberg/manifest/manifest_group.cc:
##########
@@ -320,7 +320,8 @@ class ManifestGroup::FilePlanningStream final : public
FileScanTaskStream {
residuals->ResidualFor(value.data_file->partition));
return
std::optional<std::shared_ptr<FileScanTask>>{std::make_shared<FileScanTask>(
- std::move(value.data_file), std::move(delete_files),
std::move(residual))};
+ std::move(value.data_file), std::move(delete_files),
std::move(residual),
+ group_->specs_by_id_.at(spec_id))};
Review Comment:
specs_by_id_.at(spec_id) throws std::out_of_range on a missing id,
inconsistent with GetManifestEvaluator and GetResidualEvaluator above which
look up the same map via find() plus ICEBERG_CHECK with a clear error message.
--
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]
