wgtmac commented on code in PR #423:
URL: https://github.com/apache/iceberg-cpp/pull/423#discussion_r2633967298
##########
src/iceberg/expression/expression.h:
##########
@@ -340,14 +340,14 @@ class ICEBERG_EXPORT Unbound {
public:
/// \brief Bind this expression to a concrete schema.
///
- /// \param schema The schema to bind against
+ /// \param struct_ The struct type (or schema) to bind against
Review Comment:
```suggestion
/// \param struct_type The struct type to bind against
```
##########
src/iceberg/schema.h:
##########
@@ -59,25 +59,22 @@ class ICEBERG_EXPORT Schema : public StructType {
std::string ToString() const override;
- /// \brief Find the SchemaField by field name.
+ /// \brief Recursively Find the SchemaField by field name.
Review Comment:
```suggestion
/// \brief Recursively find the SchemaField by field name.
```
##########
src/iceberg/expression/predicate.h:
##########
@@ -28,6 +28,7 @@
#include "iceberg/expression/literal.h"
#include "iceberg/expression/term.h"
#include "iceberg/iceberg_export.h"
+#include "iceberg/type.h"
Review Comment:
```suggestion
```
Use forward declaration.
##########
src/iceberg/expression/binder.h:
##########
@@ -54,7 +54,7 @@ class ICEBERG_EXPORT Binder : public
ExpressionVisitor<std::shared_ptr<Expressio
const std::shared_ptr<UnboundAggregate>& aggregate) override;
private:
- const Schema& schema_;
+ const StructType& struct_;
Review Comment:
```suggestion
const StructType& struct_type_;
```
##########
src/iceberg/schema.h:
##########
@@ -59,25 +59,22 @@ class ICEBERG_EXPORT Schema : public StructType {
std::string ToString() const override;
- /// \brief Find the SchemaField by field name.
+ /// \brief Recursively Find the SchemaField by field name.
///
/// Short names for maps and lists are included for any name that does not
conflict with
/// a canonical name. For example, a list, 'l', of structs with field 'x'
will produce
- /// short name 'l.x' in addition to canonical name 'l.element.x'. a map 'm',
if its
Review Comment:
Please revert this.
##########
src/iceberg/expression/aggregate.cc:
##########
@@ -467,13 +467,13 @@ bool MinAggregate::HasValue(const DataFile& file) const {
template <typename B>
Result<std::shared_ptr<Expression>> UnboundAggregateImpl<B>::Bind(
- const Schema& schema, bool case_sensitive) const {
+ const StructType& struct_, bool case_sensitive) const {
Review Comment:
Please rename all `struct_` to `struct_type`.
##########
src/iceberg/schema.h:
##########
@@ -59,25 +59,22 @@ class ICEBERG_EXPORT Schema : public StructType {
std::string ToString() const override;
- /// \brief Find the SchemaField by field name.
+ /// \brief Recursively Find the SchemaField by field name.
///
/// Short names for maps and lists are included for any name that does not
conflict with
/// a canonical name. For example, a list, 'l', of structs with field 'x'
will produce
- /// short name 'l.x' in addition to canonical name 'l.element.x'. a map 'm',
if its
- /// value include a structs with field 'x' wil produce short name 'm.x' in
addition to
- /// canonical name 'm.value.x'
+ /// short name 'l.x' in addition to canonical name 'l.element.x'.
/// FIXME: Currently only handles ASCII lowercase conversion; extend to
support
/// non-ASCII characters (e.g., using std::towlower or ICU)
Result<std::optional<std::reference_wrapper<const SchemaField>>>
FindFieldByName(
std::string_view name, bool case_sensitive = true) const;
- /// \brief Find the SchemaField by field id.
+ /// \brief Recursively Find the SchemaField by field id.
Result<std::optional<std::reference_wrapper<const SchemaField>>>
FindFieldById(
int32_t field_id) const;
/// \brief Get the accessor to access the field by field id.
///
- /// \param field_id The id of the field to get the accessor for.
Review Comment:
Revert this.
##########
src/iceberg/schema.h:
##########
@@ -59,25 +59,22 @@ class ICEBERG_EXPORT Schema : public StructType {
std::string ToString() const override;
- /// \brief Find the SchemaField by field name.
+ /// \brief Recursively Find the SchemaField by field name.
///
/// Short names for maps and lists are included for any name that does not
conflict with
/// a canonical name. For example, a list, 'l', of structs with field 'x'
will produce
- /// short name 'l.x' in addition to canonical name 'l.element.x'. a map 'm',
if its
- /// value include a structs with field 'x' wil produce short name 'm.x' in
addition to
- /// canonical name 'm.value.x'
+ /// short name 'l.x' in addition to canonical name 'l.element.x'.
/// FIXME: Currently only handles ASCII lowercase conversion; extend to
support
/// non-ASCII characters (e.g., using std::towlower or ICU)
Result<std::optional<std::reference_wrapper<const SchemaField>>>
FindFieldByName(
std::string_view name, bool case_sensitive = true) const;
- /// \brief Find the SchemaField by field id.
+ /// \brief Recursively Find the SchemaField by field id.
Review Comment:
ditto
##########
src/iceberg/expression/residual_evaluator.cc:
##########
@@ -270,7 +270,7 @@ Result<std::shared_ptr<Expression>>
ResidualVisitor::Predicate(
if (inclusive_projection != nullptr) {
ICEBERG_ASSIGN_OR_RAISE(
auto bound_inclusive,
- inclusive_projection->Bind(*partition_schema_, case_sensitive_));
+ inclusive_projection->Bind(*partition_type_->ToSchema(),
case_sensitive_));
Review Comment:
ditto
##########
src/iceberg/expression/residual_evaluator.cc:
##########
@@ -245,7 +245,7 @@ Result<std::shared_ptr<Expression>>
ResidualVisitor::Predicate(
if (strict_projection != nullptr) {
ICEBERG_ASSIGN_OR_RAISE(
auto bound_strict,
- strict_projection->Bind(*partition_schema_, case_sensitive_));
+ strict_projection->Bind(*partition_type_->ToSchema(),
case_sensitive_));
Review Comment:
Please create once outside and reuse it.
--
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]
