szehon-ho commented on code in PR #7898:
URL: https://github.com/apache/iceberg/pull/7898#discussion_r1240615096
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java:
##########
@@ -152,30 +160,22 @@ public static Expression convert(Predicate predicate) {
}
case EQ: // used for both eq and null-safe-eq
- Object value;
- String columnName;
- if (isRef(leftChild(predicate)) && isLiteral(rightChild(predicate)))
{
- columnName = SparkUtil.toColumnName(leftChild(predicate));
- value = convertLiteral(rightChild(predicate));
- } else if (isRef(rightChild(predicate)) &&
isLiteral(leftChild(predicate))) {
- columnName = SparkUtil.toColumnName(rightChild(predicate));
- value = convertLiteral(leftChild(predicate));
- } else {
- return null;
- }
-
- if (predicate.name().equals(EQ)) {
- // comparison with null in normal equality is always null. this is
probably a mistake.
- Preconditions.checkNotNull(
- value, "Expression is always false (eq is not null-safe): %s",
predicate);
- return handleEqual(columnName, value);
- } else { // "<=>"
- if (value == null) {
- return isNull(columnName);
- } else {
- return handleEqual(columnName, value);
- }
- }
+ return handleEQPredicate(
+ predicate,
+ (uboundTerm, value) -> {
Review Comment:
Nit: unbound
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java:
##########
@@ -245,6 +245,47 @@ public static Expression convert(Predicate predicate) {
return null;
}
+ private static <T> UnboundPredicate<T> handleEQPredicate(
+ Predicate predicate, BiFunction<UnboundTerm<T>, T, UnboundPredicate<T>>
func) {
Review Comment:
I think this lambda makes it harder to read. My vote is just we return a
Pair (or better, a modeled class of value/columnName), and invoke the func
directly after this method?
Probably the method name can be improved as well, as handle doesnt tell
much, could be called something like columnAndValue, or something similar
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java:
##########
@@ -152,30 +160,22 @@ public static Expression convert(Predicate predicate) {
}
case EQ: // used for both eq and null-safe-eq
- Object value;
- String columnName;
- if (isRef(leftChild(predicate)) && isLiteral(rightChild(predicate)))
{
- columnName = SparkUtil.toColumnName(leftChild(predicate));
- value = convertLiteral(rightChild(predicate));
- } else if (isRef(rightChild(predicate)) &&
isLiteral(leftChild(predicate))) {
- columnName = SparkUtil.toColumnName(rightChild(predicate));
- value = convertLiteral(leftChild(predicate));
- } else {
- return null;
- }
-
- if (predicate.name().equals(EQ)) {
- // comparison with null in normal equality is always null. this is
probably a mistake.
- Preconditions.checkNotNull(
- value, "Expression is always false (eq is not null-safe): %s",
predicate);
- return handleEqual(columnName, value);
- } else { // "<=>"
- if (value == null) {
- return isNull(columnName);
- } else {
- return handleEqual(columnName, value);
- }
- }
+ return handleEQPredicate(
+ predicate,
+ (uboundTerm, value) -> {
+ if (predicate.name().equals(EQ)) {
+ // comparison with null in normal equality is always null.
this is probably a
+ // mistake.
+ Preconditions.checkNotNull(
+ value, "Expression is always false (eq is not
null-safe): %s", predicate);
+ return handleEqual(uboundTerm, value);
Review Comment:
Seems we can just pull it out of if/else?
--
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: issues-unsubscr...@iceberg.apache.org
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
