shardulm94 commented on a change in pull request #933:
URL: https://github.com/apache/iceberg/pull/933#discussion_r439585809
##########
File path:
mr/src/main/java/org/apache/iceberg/mr/mapred/IcebergFilterFactory.java
##########
@@ -131,7 +90,7 @@ private static Expression translate(ExpressionTree tree,
List<PredicateLeaf> lea
*/
private static Expression translateLeaf(PredicateLeaf leaf) {
String column = leaf.getColumnName();
- if (column.equals("snapshot__id")) {
+ if (column.equals(SystemTableUtil.DEFAULT_SNAPSHOT_ID_COLUMN_NAME)) {
Review comment:
Actually, this may not be enough since the column name is configurable?
##########
File path:
mr/src/main/java/org/apache/iceberg/mr/mapred/IcebergFilterFactory.java
##########
@@ -96,29 +54,30 @@ public static Expression
generateFilterExpression(SearchArgument sarg) {
* @param leaves List of all leaf nodes within the tree.
* @return Expression that is translated from the Hive SearchArgument.
*/
- private static Expression translate(ExpressionTree tree, List<PredicateLeaf>
leaves) {
+ private static Expression translate(ExpressionTree tree, List<PredicateLeaf>
leaves,
Review comment:
Nit: childNodes doesn't need to be parameter, can be a local variable
instead.
##########
File path:
mr/src/main/java/org/apache/iceberg/mr/mapred/IcebergFilterFactory.java
##########
@@ -96,29 +54,30 @@ public static Expression
generateFilterExpression(SearchArgument sarg) {
* @param leaves List of all leaf nodes within the tree.
* @return Expression that is translated from the Hive SearchArgument.
*/
- private static Expression translate(ExpressionTree tree, List<PredicateLeaf>
leaves) {
+ private static Expression translate(ExpressionTree tree, List<PredicateLeaf>
leaves,
+ List<ExpressionTree> childNodes) {
switch (tree.getOperator()) {
case OR:
- return or(translate(tree.getChildren().get(0), leaves),
- translate(tree.getChildren().get(1), leaves));
+ Expression orResult = Expressions.alwaysFalse();
+ for (ExpressionTree child : childNodes) {
+ orResult = or(orResult, translate(child, leaves, childNodes));
Review comment:
I think passing `childNodes` here is incorrect. It should be
`child.getChildren()` else we just keep on passing the root child nodes over
and over. However, I think we should just make `childNodes` a local variable
instead so that we don't make this mistake. It would also be good to add some
tests for nested expressions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
