[#674] feat(all): add a unified Expression system #721
Conversation
Code Coverage Report
Files |
| import org.apache.iceberg.expressions.Expressions; | ||
| import org.apache.iceberg.expressions.UnboundTerm; | ||
|
|
||
| /** Implement iceberg sort order converter to gravitino sort order. */ | ||
| /** Implement gravitino sort order converter to iceberg sort order. */ |
There was a problem hiding this comment.
This comment seems incorrect, right? This is a class that converts Gravitino classes to Iceberg classes.
There was a problem hiding this comment.
It's correct after my modification
| break; | ||
| case Transforms.NAME_OF_YEAR: | ||
| expression = Expressions.year(colName); | ||
| case "year": |
There was a problem hiding this comment.
I noticed that in the Transforms class, there are relevant constant strings. Why not consider using constants here?
There was a problem hiding this comment.
Because they're in different scopes, the constant strings in Transforms mean the partitioning method name, but here is FunctionExpression's function name, they just have the same name. Therefore, I don't reuse the constant strings.
api/src/main/java/com/datastrato/gravitino/rel/expressions/Expression.java
Show resolved
Hide resolved
| /** List of fields or columns that are referenced by this expression. */ | ||
| default NamedReference[] references() { | ||
| // SPARK-40398: Replace `Arrays.stream()...distinct()` | ||
| // to this for perf gain, the result order is not important. |
There was a problem hiding this comment.
I think we can remove this comment here, since it is not valid for us.
| return arguments(); | ||
| } | ||
|
|
||
| final class FuncExpressionValue implements FunctionExpression { |
There was a problem hiding this comment.
FuncExpressionValue seems not so accurate, may be better change to FuncExpressionImpl?
| } | ||
|
|
||
| @EqualsAndHashCode | ||
| final class LiteralValue<T> implements Literal<T> { |
| return new NamedReference[] {this}; | ||
| } | ||
|
|
||
| @EqualsAndHashCode |
There was a problem hiding this comment.
I would suggest to implement this method by our own, not generate by lombok in API module.
| return of(strategy, number, expressions); | ||
| } | ||
|
|
||
| public static final class DistributionValue implements Distribution { |
There was a problem hiding this comment.
Also here, is it better change to DistributionImpl?
| return new SortValue(expression, direction, nullOrdering); | ||
| } | ||
|
|
||
| public static final class SortValue implements SortOrder { |
| return arguments(); | ||
| } | ||
|
|
||
| final class FuncExpressionValue implements FunctionExpression { |
There was a problem hiding this comment.
The name FuncExpressionValue is confusing.
| /** Represents a field or column reference in the public logical expression API. */ | ||
| public interface NamedReference extends Expression { | ||
|
|
||
| static FieldReference field(String... fieldName) { |
There was a problem hiding this comment.
I suggest adding some explanation as to why we use a string array instead of a string here.
There was a problem hiding this comment.
Why should we use a string array?
There was a problem hiding this comment.
Why should we use a string array?
@diqiu50 comment added
There was a problem hiding this comment.
Why not use "NameIdentifier" as the parameter type
| try { | ||
| return valueOf(strategy.toUpperCase()); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
It's strange here because we throw the same kind of exception here.
There was a problem hiding this comment.
Here I want to improve the error message for IllegalArgumentException, as it is currently not user-friendly.
There was a problem hiding this comment.
I suggest you using the following code to handle it.
public static Strategy getByName(String name) {
for (Strategy st : Strategy.values()) {
if (st.name().equalsIgnoreCase(name)) {
return st;
}
}
throw new IllegalArgumentException(" your error message: " + name);
}
| */ | ||
| public interface Transform extends Expression { | ||
| /** Returns the transform function name. */ | ||
| String name(); |
There was a problem hiding this comment.
What's the different between Transform and FunctionExpression?
There was a problem hiding this comment.
The usage scenarios are different. Transform is used for partitioning, while FunctionExpression is used for ordinary expressions. The difference here has already been explained in the design doc.
There was a problem hiding this comment.
I noticed that the definitions are almost the same, so I was confused by their usage.
qqqttt123
changed the title
| Transforms.field(new String[] {"field1"}), Transforms.function("now", new Transform[0]) | ||
| Expression[] distributionArg2 = | ||
| new Expression[] { | ||
| field("field1"), FunctionExpression.of("now", Expression.EMPTY_EXPRESSION) |
There was a problem hiding this comment.
We can define a FunctionExpression.of(String) function that takes no parameters
| public enum Strategy { | ||
| HASH, | ||
| RANGE, | ||
| EVEN; |
|
|
||
| public static BucketTransform bucket(int numBuckets, String[]... fieldNames) { | ||
| return new BucketTransform( | ||
| Literal.of(numBuckets, TypeCreator.REQUIRED.I32), |
There was a problem hiding this comment.
I think adding StringLiteral.of() and IntegerLiteral.of() functions would make it easier to use.
api/src/main/java/com/datastrato/gravitino/rel/expressions/Expression.java
Show resolved
Hide resolved
| Expression expression, SortDirection direction, NullOrdering nullOrdering) { | ||
| return new SortImpl(expression, direction, nullOrdering); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please give more funcitons to make easier to use.
of(Expression expression)
of(Expression expression, SortDirection direction)
There was a problem hiding this comment.
SortOrders.of(Expression expression, SortDirection direction) already exists.
|
@Clearvive @jerryshao @diqiu50 @yuqi1129 I have feedback on all the comments you left, please review the PR again, thanks! |
| /** Returns the distribution strategy name. */ | ||
| Strategy strategy(); | ||
|
|
||
| int number(); |
There was a problem hiding this comment.
Can you please add java doc for this interface.
| public static final Distribution NONE = | ||
| new DistributionImpl(Strategy.HASH, 0, Expression.EMPTY_EXPRESSION); | ||
|
|
||
| public static Distribution ofEVEN(int number, Expression... expressions) { |
There was a problem hiding this comment.
better rename to "even", "hash"
There was a problem hiding this comment.
You should add javadoc for all the public API methods.
There was a problem hiding this comment.
Javadoc for all the public API methods was added
| * nameReferenceDistribution(Strategy.EVEN, 1, new String[]{"a"}); | ||
| * </pre> | ||
| */ | ||
| public static Distribution ofFields(Strategy strategy, int number, String[] fieldNames) { |
There was a problem hiding this comment.
Should it be String[][] for fieldNames?
There was a problem hiding this comment.
Also the javadoc you mentioned here is unmatched to the method.
| return arguments(); | ||
| } | ||
|
|
||
| abstract class SingleFieldTransform implements Transform { |
There was a problem hiding this comment.
Do we currently have transform with more than one fields?
There was a problem hiding this comment.
The "SingleField" refers to the transform arguments, for example, ListTransform and BucketTransform may require referencing multiple fields.
| public static final String NAME_OF_LIST = "list"; | ||
| public static final String NAME_OF_RANGE = "range"; | ||
|
|
||
| public static IdentityTransform identity(String... fieldName) { |
There was a problem hiding this comment.
I think it would better change to identity(String[] fieldName), here String[] field represents a field with name as an array fo string. Using String... will give the user a feeling that identity supports multiple fields.
There was a problem hiding this comment.
OK, I change it to identity(String[] fieldName) and add another method identity(String columnName) for convenient to use
| Arrays.stream(fieldNames).map(NamedReference::field).toArray(NamedReference[]::new)); | ||
| } | ||
|
|
||
| public static ListTransform list(String[][] fieldNames) { |
There was a problem hiding this comment.
You should unify with either String[][] or String[]....
There was a problem hiding this comment.
Unified into String[]...
| import com.datastrato.gravitino.rel.expressions.Expression; | ||
| import java.util.Arrays; | ||
|
|
||
| public interface FunctionArg extends Expression { |
There was a problem hiding this comment.
What is the usage of this class?
There was a problem hiding this comment.
This interface is added to more clearly distinguish the definition of function argument expression and the function expression on the client side. Otherwise, LiteralDTO and FieldReferenceDTO will be mixed up with FuncExpressionDTO.
|
@Clearvive Can you help to review the Iceberg catalog part again? |
|
@yuqi1129 Can you please review the Hive catalog part again? |
| } else if (gravitinoExpr.arguments().length == 2) { | ||
| colName = | ||
| String.join( | ||
| DOT, ((NamedReference.FieldReference) gravitinoExpr.arguments()[1]).fieldName()); |
There was a problem hiding this comment.
Are you not considering the values stored in [0]? Shouldn't we log them?
There was a problem hiding this comment.
Thank you very much for pointing out my issue. My latest commit refactors this part of the code and improves readability. I welcome your further review.
| break; | ||
| case Transforms.NAME_OF_YEAR: | ||
| expression = Expressions.year(colName); | ||
| case "year": |
| * @param expression The expression to sort by | ||
| * @return The created sort order | ||
| */ | ||
| public static SortImpl of(Expression expression) { |
There was a problem hiding this comment.
Is it better rename to "ascending", also adding a new method called "descending"? I think only of cannot clearly describe the meaning of the method, and users have to read the javadoc to understand the default behavior.
|
Just left one comment, overall LGTM. |
What changes were proposed in this pull request?
Why are the changes needed?
clarify Expression, Partitioning, SortOrder, and Distribution
Fix: #674
Does this PR introduce any user-facing change?
Yep. There have been changes in the representation of Partitioning, SortOrderDTO, and DistributionDTO.
How was this patch tested?
existing tests