Skip to content

Commit e8db0ad

Browse files
zhjwpkuzhjwpku
committed
fix: review comments
1 parent d303440 commit e8db0ad

3 files changed

Lines changed: 114 additions & 169 deletions

File tree

‎src/iceberg/expression/projections.cc‎

Lines changed: 21 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424

2525
#include "iceberg/expression/expression.h"
2626
#include "iceberg/expression/expression_visitor.h"
27-
#include "iceberg/expression/expressions.h"
2827
#include "iceberg/expression/predicate.h"
2928
#include "iceberg/expression/rewrite_not.h"
3029
#include "iceberg/expression/term.h"
@@ -36,13 +35,11 @@
3635

3736
namespace iceberg {
3837

39-
// Implementation detail - not exported
4038
class ProjectionVisitor : public ExpressionVisitor<std::shared_ptr<Expression>> {
4139
public:
4240
~ProjectionVisitor() override = default;
4341

44-
ProjectionVisitor(const std::shared_ptr<PartitionSpec>& spec,
45-
const std::shared_ptr<Schema>& schema, bool case_sensitive)
42+
ProjectionVisitor(const PartitionSpec& spec, const Schema& schema, bool case_sensitive)
4643
: spec_(spec), schema_(schema), case_sensitive_(case_sensitive) {}
4744

4845
Result<std::shared_ptr<Expression>> AlwaysTrue() override { return True::Instance(); }
@@ -57,18 +54,18 @@ class ProjectionVisitor : public ExpressionVisitor<std::shared_ptr<Expression>>
5754
Result<std::shared_ptr<Expression>> And(
5855
const std::shared_ptr<Expression>& left_result,
5956
const std::shared_ptr<Expression>& right_result) override {
60-
return Expressions::And(left_result, right_result);
57+
return And::MakeFolded(left_result, right_result);
6158
}
6259

6360
Result<std::shared_ptr<Expression>> Or(
6461
const std::shared_ptr<Expression>& left_result,
6562
const std::shared_ptr<Expression>& right_result) override {
66-
return Expressions::Or(left_result, right_result);
63+
return Or::MakeFolded(left_result, right_result);
6764
}
6865

6966
Result<std::shared_ptr<Expression>> Predicate(
7067
const std::shared_ptr<UnboundPredicate>& pred) override {
71-
ICEBERG_ASSIGN_OR_RAISE(auto bound_pred, pred->Bind(*schema_, case_sensitive_));
68+
ICEBERG_ASSIGN_OR_RAISE(auto bound_pred, pred->Bind(schema_, case_sensitive_));
7269
if (bound_pred->is_bound_predicate()) {
7370
auto bound_predicate = std::dynamic_pointer_cast<BoundPredicate>(bound_pred);
7471
ICEBERG_DCHECK(
@@ -85,8 +82,8 @@ class ProjectionVisitor : public ExpressionVisitor<std::shared_ptr<Expression>>
8582
}
8683

8784
protected:
88-
const std::shared_ptr<PartitionSpec>& spec_;
89-
const std::shared_ptr<Schema>& schema_;
85+
const PartitionSpec& spec_;
86+
const Schema& schema_;
9087
bool case_sensitive_;
9188

9289
/// \brief Get partition fields that match the predicate's term.
@@ -110,7 +107,7 @@ class ProjectionVisitor : public ExpressionVisitor<std::shared_ptr<Expression>>
110107
}
111108

112109
std::vector<const PartitionField*> result;
113-
for (const auto& field : spec_->fields()) {
110+
for (const auto& field : spec_.fields()) {
114111
if (field.source_id() == source_id) {
115112
result.push_back(&field);
116113
}
@@ -131,8 +128,8 @@ class InclusiveProjectionVisitor : public ProjectionVisitor {
131128
public:
132129
~InclusiveProjectionVisitor() override = default;
133130

134-
InclusiveProjectionVisitor(const std::shared_ptr<PartitionSpec>& spec,
135-
const std::shared_ptr<Schema>& schema, bool case_sensitive)
131+
InclusiveProjectionVisitor(const PartitionSpec& spec, const Schema& schema,
132+
bool case_sensitive)
136133
: ProjectionVisitor(spec, schema, case_sensitive) {}
137134

138135
Result<std::shared_ptr<Expression>> Predicate(
@@ -159,15 +156,13 @@ class InclusiveProjectionVisitor : public ProjectionVisitor {
159156
ICEBERG_ASSIGN_OR_RAISE(auto projected,
160157
part_field->transform()->Project(part_field->name(), pred));
161158
if (projected != nullptr) {
162-
result =
163-
Expressions::And(result, std::shared_ptr<Expression>(projected.release()));
159+
ICEBERG_ASSIGN_OR_RAISE(result,
160+
And::MakeFolded(std::move(result), std::move(projected)));
164161
}
165162
}
166163

167164
return result;
168165
}
169-
170-
protected:
171166
};
172167

173168
/// \brief Strict projection evaluator.
@@ -177,8 +172,8 @@ class StrictProjectionVisitor : public ProjectionVisitor {
177172
public:
178173
~StrictProjectionVisitor() override = default;
179174

180-
StrictProjectionVisitor(const std::shared_ptr<PartitionSpec>& spec,
181-
const std::shared_ptr<Schema>& schema, bool case_sensitive)
175+
StrictProjectionVisitor(const PartitionSpec& spec, const Schema& schema,
176+
bool case_sensitive)
182177
: ProjectionVisitor(spec, schema, case_sensitive) {}
183178

184179
Result<std::shared_ptr<Expression>> Predicate(
@@ -203,8 +198,8 @@ class StrictProjectionVisitor : public ProjectionVisitor {
203198
ICEBERG_ASSIGN_OR_RAISE(auto projected, part_field->transform()->ProjectStrict(
204199
part_field->name(), pred));
205200
if (projected != nullptr) {
206-
result =
207-
Expressions::Or(result, std::shared_ptr<Expression>(projected.release()));
201+
ICEBERG_ASSIGN_OR_RAISE(result,
202+
Or::MakeFolded(std::move(result), std::move(projected)));
208203
}
209204
}
210205

@@ -224,18 +219,18 @@ Result<std::shared_ptr<Expression>> ProjectionEvaluator::Project(
224219
return Visit<std::shared_ptr<Expression>, ProjectionVisitor>(rewritten, *visitor_);
225220
}
226221

227-
std::unique_ptr<ProjectionEvaluator> Projections::Inclusive(
228-
const std::shared_ptr<PartitionSpec>& spec, const std::shared_ptr<Schema>& schema,
229-
bool case_sensitive) {
222+
std::unique_ptr<ProjectionEvaluator> Projections::Inclusive(const PartitionSpec& spec,
223+
const Schema& schema,
224+
bool case_sensitive) {
230225
auto visitor =
231226
std::make_unique<InclusiveProjectionVisitor>(spec, schema, case_sensitive);
232227
return std::unique_ptr<ProjectionEvaluator>(
233228
new ProjectionEvaluator(std::move(visitor)));
234229
}
235230

236-
std::unique_ptr<ProjectionEvaluator> Projections::Strict(
237-
const std::shared_ptr<PartitionSpec>& spec, const std::shared_ptr<Schema>& schema,
238-
bool case_sensitive) {
231+
std::unique_ptr<ProjectionEvaluator> Projections::Strict(const PartitionSpec& spec,
232+
const Schema& schema,
233+
bool case_sensitive) {
239234
auto visitor = std::make_unique<StrictProjectionVisitor>(spec, schema, case_sensitive);
240235
return std::unique_ptr<ProjectionEvaluator>(
241236
new ProjectionEvaluator(std::move(visitor)));

‎src/iceberg/expression/projections.h‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ class ICEBERG_EXPORT ProjectionEvaluator {
5050
/// \param visitor The projection visitor to use
5151
explicit ProjectionEvaluator(std::unique_ptr<class ProjectionVisitor> visitor);
5252

53-
std::unique_ptr<class ProjectionVisitor> visitor_;
53+
std::unique_ptr<ProjectionVisitor> visitor_;
5454
};
5555

5656
/// \brief Utils to project expressions on rows to expressions on partitions.
@@ -62,8 +62,7 @@ class ICEBERG_EXPORT ProjectionEvaluator {
6262
///
6363
/// A strict projection guarantees that if a partition matches a projected expression,
6464
/// then all rows in that partition will match the original expression.
65-
class ICEBERG_EXPORT Projections {
66-
public:
65+
struct ICEBERG_EXPORT Projections {
6766
/// \brief Creates an inclusive ProjectionEvaluator for the partition spec.
6867
///
6968
/// An evaluator is used to project expressions for a table's data rows into expressions
@@ -75,12 +74,13 @@ class ICEBERG_EXPORT Projections {
7574
/// Each predicate in the expression is projected using Transform::Project.
7675
///
7776
/// \param spec a partition spec
77+
/// \param schema a schema
7878
/// \param case_sensitive whether the Projection should consider case sensitivity on
7979
/// column names or not. Defaults to true (case sensitive).
8080
/// \return an inclusive projection evaluator for the partition spec
81-
static std::unique_ptr<ProjectionEvaluator> Inclusive(
82-
const std::shared_ptr<PartitionSpec>& spec, const std::shared_ptr<Schema>& schema,
83-
bool case_sensitive = true);
81+
static std::unique_ptr<ProjectionEvaluator> Inclusive(const PartitionSpec& spec,
82+
const Schema& schema,
83+
bool case_sensitive = true);
8484

8585
/// \brief Creates a strict ProjectionEvaluator for the partition spec.
8686
///
@@ -93,15 +93,13 @@ class ICEBERG_EXPORT Projections {
9393
/// Each predicate in the expression is projected using Transform::ProjectStrict.
9494
///
9595
/// \param spec a partition spec
96+
/// \param schema a schema
9697
/// \param case_sensitive whether the Projection should consider case sensitivity on
9798
/// column names or not. Defaults to true (case sensitive).
9899
/// \return a strict projection evaluator for the partition spec
99-
static std::unique_ptr<ProjectionEvaluator> Strict(
100-
const std::shared_ptr<PartitionSpec>& spec, const std::shared_ptr<Schema>& schema,
101-
bool case_sensitive = true);
102-
103-
private:
104-
Projections() = default;
100+
static std::unique_ptr<ProjectionEvaluator> Strict(const PartitionSpec& spec,
101+
const Schema& schema,
102+
bool case_sensitive = true);
105103
};
106104

107105
} // namespace iceberg

0 commit comments

Comments
 (0)