[Kernel] Refactor expressions #1997

vkorukanti · 2023-08-21T18:04:13Z

Which Delta project/connector is this regarding?

Description

Refactor the Kernel expression interfaces. Currently, the Expression base interface contains a few unnecessary methods such as dataType and eval. Keep the Expression to a minimum, so that it is just used to represent a SQL string version of the expression in Kernel Expression classes.

interface Expression {
  /**
    * @return a {@link List} of the immediate children of this node
    */
  List<Expression> children();
}

Introduce a subtype of Expression called ScalarExpression which is a base class for all scalar expressions.
Introduce a subtype of ScalarExpression called Predicate as the base expression class for scalar expression that evaluates to boolean type. The Predicate is defined such that it takes a generic expression name and any number of input expressions. It is up to the evaluator to make sure the given Predicate is evaluable. Currently Predicate only allows a subset of expressions (=, <, <=, >, >=, AND, OR, ALWAYS_TRUE, ALWAYS_FALSE) as of now. In the future, this can be extended to support more predicate expressions with minimal code changes.
Update scan-related APIs to Predicate instead of Expression.
Remove the use of Literal.FALSE and Literal.TRUE and instead use AlwaysTrue.ALWAYS_TRUE and AlwaysFalse.ALWAYS_FALSE. Literal is not a predicate.
Extract the expression evaluation from kernel-api into kernel-defaults.
- DefaultExpressionEvaluator validates the expression and adds necessary implicit casts to allow evaluation.

TODO (will be addressed after this PR is landed):

It is not clear now whether we need the CAST expression as a first class Expression in the kernel-api module. If needed in the future, we can add one ([Feature Request][Kernel] Investigate whether we need Cast as an expression in Kernel #2043).
Implicit cast in kernel-default may need to support more type conversions, especially around the Decimal type ([Feature Request][Kernel] Add more implicit cast conversions. #2044)
Add support for nested column reference in Column expression ([Feature Request][Kernel] Add support for referring to nested columns in Kernel Column expression #2040).
Implicit cast of DefaultExpressionEvaluator output type to expected type ([Feature Request][Kernel] Support implicit cast for DefaultExpressionEvaluator output. #2047)

How was this patch tested?

Moved the existing Java-based test to Scala and also added new tests (some of them are copied over from the standalone ExpressionSuite and updated).

kernel/kernel-api/src/main/java/io/delta/kernel/client/ExpressionHandler.java

tdas

comments

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Expression.java

tdas · 2023-08-23T17:05:07Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Expression.java

-     * @return the String representation of this expression.
-     */
-    String toString();
-
    /**
     * @return a {@link List} of the immediate children of this node
     */
    List<Expression> children();


Any thoughts on using List vs Array?

I am inclined to use the List. Lists are easy to create (like appending an element etc) and also can create an unmodifiable view on top of the list when the children are requested through getter.

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Column.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicate/AlwaysFalse.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicate/Predicate.java

tdas · 2023-08-23T17:57:04Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicate/Predicate.java

+    /**
+     * List of supported predicate expressions
+     */
+    private static final Set<String> PREDICATE_TYPES = Stream.of(


types? or names? since these are actual allowed names.

Either of them should be fine. Given this is a private variable, does it matter?

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicate/Predicate.java

allisonport-db

A few questions on the changes in kernel-api

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Column.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Expression.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Column.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Literal.java

allisonport-db · 2023-08-30T00:58:34Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/And.java

+    public Predicate getLeft() {
+        return (Predicate) children().get(0);
+    }
+
+    /**
+     * @return Right side operand.
+     */
+    public Predicate getRight() {
+        return (Predicate) children().get(1);
+    }


Do we need these APIs in the Kernel API framework?

It helps the clients of this expression. Don't need to have the code to get the childrens list, get the first element and validate it is a Predicate expression.

allisonport-db · 2023-08-30T01:14:39Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/Predicate.java

+    private static void validatePredicateExpression(String name, List<Expression> children) {
+        checkArgument(PREDICATE_TYPES.contains(name), "Unknown predicate expression: %s", name);
+        switch (name) {
+            case "AND": // fall through


What about a user provided expression like col1 AND col2? Do we expect the connector to create a different kind of expression than Predicate?

col1 AND col2 can be rewritten using col1 = true AND col2 = true. If this becomes a problem, we can relax this requirement in future.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/InternalUtils.java

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructType.java

kernel/kernel-api/src/main/java/io/delta/kernel/client/ExpressionHandler.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Literal.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/ScalarExpression.java

vkorukanti · 2023-08-30T17:03:39Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/Predicate.java

+     * @param name Predicate expression name
+     * @param children List of input expressions to the predicate expression.
+     */
+    private static void validatePredicateExpression(String name, List<Expression> children) {


@tdas I am wondering if we should get rid of this type checks? It is becoming complicated as there is a ScalarExpression in between Predicate and Expression. ScalarExpression needs to aware of these types if it wants to validate the given expression is allowed or not.

ScalarExpression has no data type or type validation right? So whats the problem if Predicate constructor does type checks before calling in the ScalarExpression constructor which does not do type checks?

So in future when we add more scalar expression that are not Predicate, where do we do the validation?

The validation also includes the given name is one of the supported expression.

It looks like the validation is not adding much value. The implementation of the ExpressionHandler already throws error if it gets an expression which is not one of the listed expression in the JavaDoc of Predicate or ScalarExpression. I am inclined to remove these validation checks.

If there is a case in the future where wrong expressions does not throw any error and silently does not produce the intended result.. then we throwing error might be better. Keep an eye out for those stuff.

tdas · 2023-08-31T14:34:07Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/Predicate.java

+        return String.format("%s(%s)", name, args);
+    }
+
+    private static final Set<String> COMPARATOR_OPERATORS =


These are the names... right? why are you calling this "operators" in one place and "types" in another place (below)? why not just "names"

This code is removed. See here for details.

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/Predicate.java

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java

tdas · 2023-08-31T14:53:02Z

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java

+    /**
+     * Implementation of {@link ExpressionVisitor} to evaluate it on a {@link ColumnarBatch}.
+     */
+    private static class ExpressionEvaluator extends ExpressionVisitor<ColumnVector> {


This is the same name as ExpressionEvaluator interface!!!! This is hella confusing!

Renamed to ExpressionEvalVisitor.

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java

allisonport-db · 2023-08-31T22:28:52Z

kernel/kernel-api/src/main/java/io/delta/kernel/client/ExpressionHandler.java

-    ExpressionEvaluator getEvaluator(StructType batchSchema, Expression expression);
+    ExpressionEvaluator getEvaluator(
+        StructType inputSchema,
+        Expression expression,


Should this be ScalarExpression if we're adding that as an an expression type?

I am ok to update this, but we need to start adding more methods to the interface if we end up adding aggregation expressions or introduce another interface between Expression and ScalarExpression

@tdas let me know what you think.

Yeah, its likely we have to for aggregate expressions... because for aggregate .. would ExpressionsEvaluator produce ColumnVector?

So it makes sense to make this a ScalaExpression

Currently Column and Literal are not derived from ScalarExpression. Should we make them extend ScalarExpression before making this change? Otherwise we cannot populate partition values.

This makes sense to me, seems like literal and column should be scalar expressions

Literal and Column are leaf expressions which don't take any input expressions. There isn't a one input and output concept here. Also Spark DS v2 doesn't extend Literal and Column from ScalarExpression.

I am inclined to keep it this way. If there is a strong reason, I will followup the change in a separate PR.

Okay good to merge. Let's follow up on it with @tdas next week?

allisonport-db · 2023-08-31T22:33:51Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/ScalarExpression.java

+ * output value. A subclass of these expressions are of type {@link Predicate} whose result is
+ * `boolean`. See {@link Predicate} for predicate type scalar expressions. Supported
+ * non-predicate type scalar expressions are listed.
+ *  TODO: Currently there aren't any. Will be added in future. An example one looks like this:


Do we intend to allow engines to create/pass through any scalar expressions they support? I was under the impression we wouldn't be enforcing a supported list for these.

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/Predicate.java

allisonport-db · 2023-08-31T22:41:10Z

kernel/kernel-api/src/main/java/io/delta/kernel/utils/Utils.java

we already have these in InternalUtils? let's not duplicate them in both places?

I am overall confused. It this Utils supposed to be public or internal? things like checkArgument does not seem like things that kernel should expose as public apis

It involves copying, but I am inclined to keep them internal and duplicate them in both kernel-api and kernel-default. This is just a few lines of code.

You could put them in an internal util in Kernel API and reuse that in Kernel Default. Its okay for Kernel Defaults to use some small internal stuff from Kernel API.

I will followup with a separate PR (there is a TODO already).

tdas · 2023-09-05T20:30:46Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Literal.java

+     * of the following types based on the literal data type:
+     *
+     * <u>
+     * <li>BOOLEAN -> {@link Boolean}</li>>


what do these link to?

They link to the specific class referred with @link

tdas · 2023-09-05T20:33:21Z

...el-defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/ExpressionUtils.java

+
+    /**
+     * Utility method to compare the left and right according to the natural ordering
+     * and return the comparison result (-1, 0, 1) for each row as an integer array.


what types are allowed?

added a comment.

tdas · 2023-09-05T20:42:46Z

kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/DefaultKernelUtils.java

@@ -156,16 +156,6 @@ private static Type prunedType(Type type, DataType deltaType) {
        }
    }

-    /**


why remove this from internal class to public class?

See the previous comment response.

kernel/kernel-api/src/main/java/io/delta/kernel/client/FileHandler.java

tdas · 2023-09-08T14:38:09Z

kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/DefaultKernelUtils.java

@@ -74,6 +72,8 @@ public static Type findSubFieldType(GroupType groupType, StructField field) {
        return null;
    }

+    // TODO: Move these precondition checks into a separate utility class.


please keep track of the TODOs and make sure all these are done before the 3.0 release.

tdas · 2023-09-08T14:56:16Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/ScalarExpression.java

+ * output value. A subclass of these expressions are of type {@link Predicate} whose result type is
+ * `boolean`. See {@link Predicate} for predicate type scalar expressions. Supported
+ * non-predicate type scalar expressions are listed below.
+ * TODO: Currently there aren't any. Will be added in future. An example one looks like this:


@vkorukanti @allisonport-db remind me what was decided... whether we are going to validation for allow expressions or not.

I thought we weren't since we were going to let engines use whatever expressions they support

kernel/kernel-api/src/main/java/io/delta/kernel/utils/Utils.java

...st/scala/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluatorSuite.scala

tdas · 2023-09-08T15:24:30Z

...st/scala/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluatorSuite.scala

+    }
+  }
+
+  test("expression: and") {


why are these empty??

...st/scala/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluatorSuite.scala

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/TestUtils.scala

...st/scala/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluatorSuite.scala

scottsand-db

I just wonder if a BinaryPredicate could simplify the code a bit and remove duplication?

scottsand-db · 2023-09-11T20:13:53Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/And.java

+    /**
+     * @return Left side operand.
+     */
+    public Predicate getLeft() {


wouldn't this fit best inside of a BinaryPredicate class?

We are trying to minimize the number of interfaces. The goal is to have just one Predicate class that captures all the predicates. And and Or are just two special expressions.

scottsand-db · 2023-09-11T20:14:35Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Column.java

- * <p>
- * Only supports primitive data types, see
- * <a href="https://github.com/delta-io/delta/blob/master/PROTOCOL.md#primitive-types">Delta Transaction Log Protocol: Primitive Types</a>.
+ * An expression type that refers to a column by name (case-sensitive) in the input.


how does this handle nested columns?

scottsand-db · 2023-09-11T20:15:54Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Literal.java

     * @return a {@link Literal} with data type {@link TimestampType}
     */
-    public static Literal of(Timestamp value) {
-        return new Literal(value, TimestampType.INSTANCE);
+    public static Literal ofTimestamp(long microsSinceEpochUTC) {


nit: ofTimestampUTC ? wouldn't that make this API super, super clear

We are using the datatype as a suffix to differentiate. Adding the UTC makes it look like another datatype.

scottsand-db · 2023-09-11T20:19:47Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Predicate.java

-    default DataType dataType() {
-        return BooleanType.INSTANCE;
+    public String toString() {
+        if (Arrays.asList("<", "<=", ">", ">=", "=").contains(name)) {


where are these expression classes (less than, greater than, etc.) defined?

To avoid the class bloat whenever a new expression is added, we defined a minimal set of generic expressions. The supported expressions are listed in the javadoc. This type of generic interface also allows the connector to pass through any special expression it wants through Kernel-API module to its own implementation of the ExpressionHandler.

allisonport-db

Mostly minor comments

allisonport-db · 2023-08-31T23:59:59Z

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java

+     * <ul>
+     *     <li>given input column is part of the input data schema</li>
+     *     <li>expression inputs are of supported types. Insert cast according to the rules in
+     *     {@link ImplicitCastExpression} to make the types compatible for evaluation by


What about implicit casting to the output type? Should we do that somewhere?

Bump on this?

Good qn. We want to do this at some point. For now all our expressions either return boolean or specify the exact type (partition col population). Created #2047 to track this.

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Literal.java

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Predicate.java

allisonport-db · 2023-09-11T21:55:28Z

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/ScalarExpression.java

+ * output value. A subclass of these expressions are of type {@link Predicate} whose result type is
+ * `boolean`. See {@link Predicate} for predicate type scalar expressions. Supported
+ * non-predicate type scalar expressions are listed below.
+ * TODO: Currently there aren't any. Will be added in future. An example one looks like this:


I thought we weren't since we were going to let engines use whatever expressions they support

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/TestUtils.scala

...c/test/scala/io/delta/kernel/defaults/internal/expressions/ImplicitCastExpressionSuite.scala

allisonport-db · 2023-09-12T01:57:20Z

...ults/src/main/java/io/delta/kernel/defaults/internal/expressions/ImplicitCastExpression.java

+ * the same type, but the evaluator expects same type inputs. There could be more use cases, but
+ * for now this is the only use case.
+ */
+final class ImplicitCastExpression implements Expression {


Should we do anything for decimal types?

We may need to add more types. That's why I added a comment above to indicate the list is not exhastive.

Want to create a github issue to follow up on this? Seems pretty weird that a comparison between decimal types with different scales (which just determines how it's written in parquet and not the actual value) would fail

#2044 (also mentioned in the PR description)

allisonport-db · 2023-09-12T02:38:52Z

kernel/kernel-api/src/main/java/io/delta/kernel/client/ExpressionHandler.java

-    ExpressionEvaluator getEvaluator(StructType batchSchema, Expression expression);
+    ExpressionEvaluator getEvaluator(
+        StructType inputSchema,
+        Expression expression,


This makes sense to me, seems like literal and column should be scalar expressions

allisonport-db

Pending questions about casting to output type and making literal/column scalar expressions

allisonport-db · 2023-09-12T19:47:45Z

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java

+     * <ul>
+     *     <li>given input column is part of the input data schema</li>
+     *     <li>expression inputs are of supported types. Insert cast according to the rules in
+     *     {@link ImplicitCastExpression} to make the types compatible for evaluation by


Bump on this?

vkorukanti · 2023-09-12T20:07:24Z

Pending questions about casting to output type and making literal/column scalar expressions

Created a tracking issue to handle the cast expression output to evaluator output.

For making the literal/column scalar expression: I responded here. Let me know if there is any strong reason to deviate from Spark DSv2 expression model.

allisonport-db

LGTM. To follow up about literal/column as scalar expressions next week.

vkorukanti force-pushed the expression_eval branch 5 times, most recently from 4736839 to 472d32a Compare August 22, 2023 21:53

vkorukanti commented Aug 23, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/client/ExpressionHandler.java Outdated Show resolved Hide resolved

vkorukanti mentioned this pull request Aug 24, 2023

[Umbrella Feature Request] Delta Kernel APIs to simplify building connectors for reading Delta tables #1783

Open

3 tasks

tdas requested changes Aug 24, 2023

View reviewed changes

vkorukanti force-pushed the expression_eval branch from a667ad9 to c9b65ec Compare August 29, 2023 16:39

vkorukanti changed the title ~~[WIP][Kernel] Refactor expressions~~ [Kernel] Refactor expressions Aug 29, 2023

vkorukanti requested review from tdas and allisonport-db August 29, 2023 18:08

allisonport-db reviewed Aug 30, 2023

View reviewed changes

tdas reviewed Aug 30, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/client/ExpressionHandler.java Show resolved Hide resolved

tdas reviewed Aug 30, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/Literal.java Show resolved Hide resolved

tdas reviewed Aug 30, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/ScalarExpression.java Outdated Show resolved Hide resolved

tdas reviewed Aug 30, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/ScalarExpression.java Outdated Show resolved Hide resolved

vkorukanti commented Aug 30, 2023

View reviewed changes

tdas reviewed Aug 31, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/Predicate.java Outdated Show resolved Hide resolved

tdas reviewed Aug 31, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/expressions/predicates/Predicate.java Outdated Show resolved Hide resolved

tdas reviewed Aug 31, 2023

View reviewed changes

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java Show resolved Hide resolved

tdas reviewed Aug 31, 2023

View reviewed changes

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java Show resolved Hide resolved

tdas reviewed Aug 31, 2023

View reviewed changes

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java Outdated Show resolved Hide resolved

tdas reviewed Aug 31, 2023

View reviewed changes

.../src/main/java/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluator.java Outdated Show resolved Hide resolved

vkorukanti force-pushed the expression_eval branch from 1ec889f to 0f29734 Compare August 31, 2023 17:21

allisonport-db reviewed Aug 31, 2023

View reviewed changes

tdas reviewed Sep 5, 2023

View reviewed changes

vkorukanti force-pushed the expression_eval branch from 0f29734 to f190318 Compare September 6, 2023 18:30