[Kernel] Change comparator expression to lazy evaluation #2853

zzl-7 · 2024-04-05T06:26:51Z

Which Delta project/connector is this regarding?

Description

Resolves #2541

How was this patch tested?

Unit test in delta/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluatorSuite.scala

Does this PR introduce any user-facing changes?

zzl-7 · 2024-04-05T06:38:15Z

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

+            public int getSize() { return left.getSize(); }
+            @Override
+            public boolean isNullAt(int rowId) {
+                return left.isNullAt(rowId) || right.isNullAt(rowId);


In ticket #2541 , it is mentioned that we want to check against the following truth table

Left Right Result

null null True

not null (A) null False

null not null (B) False

not null (A) not null (B) False

not null (A) not null (A) True

But the current behavior is

Left Right Result

null null False

not null (A) null False

null not null (B) False

not null (A) not null (B) False

not null (A) not null (A) True

Do we want to change the behavior? I am guessing we want to follow the truth table here https://spark.apache.org/docs/latest/sql-ref-null-semantics.html#:~:text=In%20order%20to%20compare%20the,both%20the%20operands%20are%20NULL%20. ?

Follow the existing method (i.e the second table). The example I gave in #2541 is for the null safe equals.

Thanks for the clarification, the existing code should follow the second table, once this PR is merged, I will add support for null safe equals.

zzl-7 · 2024-04-05T06:43:32Z

If code reviewer is ok with the approach, I can patch the PR with cleanup of old code and add more documentation
Thank you

vkorukanti

@zzl-7 This looks great. Thank you for working on this. Few comments.

vkorukanti · 2024-04-08T13:38:54Z

kernel/kernel-api/src/main/java/io/delta/kernel/data/VectorComparator.java

+ * Comparator interface for lazy evaluating comparator
+ */
+@Evolving
+public interface VectorComparator {


This is applicable only for the default table client module. Please move this to kernel-defaults module in package io.delta.kernel.defaults.internal.expressions.

thanks for pointing that out, fixed.

vkorukanti · 2024-04-08T13:47:29Z

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

+                    v = new VectorComparator() {
+                        @Override
+                        public boolean compare(int compareResult) {
+                            return compareResult == 0;
+                        }
+                    };


can this be defined as a lambda (compareResult) -> (compareResult == 0)?

comparatorVector(leftResult, rightResult, (compareResult) -> (compareResult == 0))

good call, fixed.

vkorukanti · 2024-04-08T13:48:36Z

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

+            public int getSize() { return left.getSize(); }
+            @Override
+            public boolean isNullAt(int rowId) {
+                return left.isNullAt(rowId) || right.isNullAt(rowId);


Follow the existing method (i.e the second table). The example I gave in #2541 is for the null safe equals.

zzl-7 · 2024-04-09T06:12:46Z

@vkorukanti thanks for code reviewing this PR, I have addressed everything, can you take a look at it again?
Thank you

zzl-7 · 2024-04-23T04:15:32Z

Hi @vkorukanti , is there any additional change that you would like me to do? If not can we merge the code?
Thanks

vkorukanti · 2024-04-23T17:16:41Z

@zzl-7, sorry this is getting delayed. Will take a look at this week for sure.

zzl-7 · 2024-04-24T01:09:03Z

@zzl-7, sorry this is getting delayed. Will take a look at this week for sure.

@vkorukanti , No worries, please let me know if there is anything I can do on my side to help :)
Thanks

vkorukanti

@zzl-7 This is very close, thanks for working on this. Let me know if you have any qns.

vkorukanti · 2024-04-29T15:46:56Z

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

+                if (isNullAt(rowId)) {
+                    return false;
+                }
+                DataType dataType = left.getDataType();


The VectorComparator interface looks good.

Only this is that we now evaluate the this long if statement for every row. Instead of we create a comparator operator for each type (e.g. boolean or short) we can avoid the if statement cost.

class ValueComparator { ValueComparator(ColumnVector v1, ColumnVector v2); boolean getCompareResult(int rowId); } class ByteValueComparator { ColumnVector v1; ColumnVector v2; ByteValueComparator(ColumnVector v1, ColumnVector v2) { this.v1 = v1; this.v2 = v2; } boolean getCompareResult(int rowId) { // null check // and then compare value for non-nulls Byte.compare(left.getByte(rowId), right.getByte(rowId))) } }

Hi @vkorukanti good catch, I didn't thought about that and should move the long if statement outside of anonymous class. I have fixed the suggestion, please let me know if this is what you are looking for
thanks

zzl-7 · 2024-05-16T06:08:41Z

Hi @vkorukanti , just want to follow up on if there any additional change you would like me to do
Thanks :)

vkorukanti

@zzl-7 Apologies for the delay. This PR is close to merging. Just a few comments to use the JDK provided interfaces/classes, which will simplify and remove many of the defined classes in the PR. Once those are addressed, then this PR is ready to merge.

vkorukanti · 2024-05-23T20:52:32Z

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

+        ValueComparator valueComparator;
        if (dataType instanceof BooleanType) {
-            compareBoolean(left, right, result);
+            valueComparator = new BooleanValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof ByteType) {
-            compareByte(left, right, result);
+            valueComparator = new ByteValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof ShortType) {
-            compareShort(left, right, result);
+            valueComparator = new ShortValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof IntegerType || dataType instanceof DateType) {
-            compareInt(left, right, result);
+            valueComparator = new IntegerValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof LongType ||
                dataType instanceof TimestampType ||
                dataType instanceof TimestampNTZType) {
-            compareLong(left, right, result);
+            valueComparator = new LongValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof FloatType) {
-            compareFloat(left, right, result);
+            valueComparator = new FloatValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof DoubleType) {
-            compareDouble(left, right, result);
+            valueComparator = new DoubleValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof DecimalType) {
-            compareDecimal(left, right, result);
+            valueComparator = new DecimalValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof StringType) {
-            compareString(left, right, result);
+            valueComparator = new StringValueComparator(left, right, vectorComparator);
        } else if (dataType instanceof BinaryType) {
-            compareBinary(left, right, result);
+            valueComparator = new BinaryValueComparator(left, right, vectorComparator);
        } else {
            throw new UnsupportedOperationException(dataType + " can not be compared.");
        }


Hi, we can actually define the classes as lambdas. That way we don't need to create a new file for each type.

Here is an example:

if (dataType instanceof DoubleType) { valueComparator = rowId -> vectorComparator.compare( Double.compare(left.getDouble(rowId), right.getDouble(rowId))); } else if (dataType instanceof DecimalType) { valueComparator = rowId -> vectorComparator.compare( BIGDECIMAL_COMPARATOR.compare(left.getDecimal(rowId), right.getDecimal(rowId))); }

You can delete all the ValueComparator classes and the interface. Use IntPredicate in place of the ValueComparator interface.

good call, thanks for pointing IntPredicate out, it made the code much cleaner

vkorukanti · 2024-05-23T20:54:27Z

...l-defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/VectorComparator.java

+/**
+ * Comparator interface for lazy evaluating comparator
+ */
+@Evolving


We can actually remove this and use the Java JDK provided IntPredicate

good call, fixed

I think this is no longer used. Please remove it.

vkorukanti · 2024-05-23T21:03:33Z

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

+                }
+                return valueComparator.getCompareResult(rowId);
+            }
+        };
    }

    static void compareBoolean(ColumnVector left, ColumnVector right, int[] result) {


we can remove all these methods right as they are no longer used?

vkorukanti · 2024-05-23T21:04:11Z

...aults/src/main/java/io/delta/kernel/defaults/internal/data/vector/StringValueComparator.java

+import io.delta.kernel.defaults.internal.expressions.VectorComparator;
+
+public class StringValueComparator implements ValueComparator {
+    static final Comparator<String> STRING_COMPARATOR = Comparator.naturalOrder();


Move constants like this into DefaultExpressionUtils.

vkorukanti · 2024-05-23T21:04:48Z

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

+    static ColumnVector comparatorVector(
+            ColumnVector left,
+            ColumnVector right,
+            VectorComparator vectorComparator) {


Suggested change

static ColumnVector comparatorVector(

ColumnVector left,

ColumnVector right,

VectorComparator vectorComparator) {

static ColumnVector comparatorVector(

ColumnVector left,

ColumnVector right,

IntPredicate vectorComparator) {

zzl-7 · 2024-05-28T09:17:25Z

@vkorukanti
Thanks for the code review, I have fixed all suggestions, can you take a look at it again when you are free?
Thank you

vkorukanti

@zzl-7 Just one last comment. The VectorComparator is no longer used. Can you remove it? Let me know, I can also do it before merging it.

vkorukanti · 2024-05-29T18:50:33Z

...l-defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/VectorComparator.java

+/**
+ * Comparator interface for lazy evaluating comparator
+ */
+@Evolving


I think this is no longer used. Please remove it.

zzl-7 · 2024-05-30T12:43:00Z

@zzl-7 Just one last comment. The VectorComparator is no longer used. Can you remove it? Let me know, I can also do it before merging it.

(forgot to comment while waiting for github action)
good catch, just removed it , thanks :)

zzl-7 added 2 commits April 4, 2024 23:26

Change comparator expression to lazy evaluation

b150f2f

fix style

4b7ecba

zzl-7 commented Apr 5, 2024

View reviewed changes

fix style

851fc75

zzl-7 mentioned this pull request Apr 5, 2024

[Kernel][Expressions] Make the default expression handler evaluation lazy #2541

Closed

8 tasks

fix style

4641e81

vkorukanti requested changes Apr 8, 2024

View reviewed changes

vkorukanti added the kernel label Apr 8, 2024

vkorukanti added this to the 3.2.0 milestone Apr 8, 2024

zzl-7 added 6 commits April 8, 2024 21:32

Fix code review

844e277

fix code review

daffb80

fix cr

70e829c

Merge remote-tracking branch 'origin' into support_lazy_comparator

ec4ddf4

fix ntz test

833b04b

code review

1b9e5dd

vkorukanti changed the title ~~Change comparator expression to lazy evaluation~~ [Kernel] Change comparator expression to lazy evaluation Apr 10, 2024

vkorukanti reviewed Apr 29, 2024

View reviewed changes

zzl-7 added 8 commits May 1, 2024 00:56

Fix CR

ee078b7

fix cr

840f586

fix cr

4b0818e

fix cr

be97175

fix cr

8afbc83

fix cr

d01c35d

fix cr

63dc5df

fix cr

eb30961

zzl-7 added 2 commits May 15, 2024 22:16

Merge remote-tracking branch 'origin' into support_lazy_comparator

15256bd

cr

a086174

vkorukanti requested changes May 23, 2024

View reviewed changes

vkorukanti modified the milestones: 3.2.0, 3.3.0 May 23, 2024

zzl-7 added 3 commits May 28, 2024 01:00

Merge remote-tracking branch 'origin' into support_lazy_comparator

1ecc62b

fix cr

843ad18

fix cr

a73803b

vkorukanti approved these changes May 29, 2024

View reviewed changes

fix cr

8d237d5

vkorukanti approved these changes May 30, 2024

View reviewed changes

vkorukanti merged commit 085f117 into delta-io:master May 30, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Change comparator expression to lazy evaluation #2853

[Kernel] Change comparator expression to lazy evaluation #2853

zzl-7 commented Apr 5, 2024 •

edited

Loading

zzl-7 Apr 5, 2024

vkorukanti Apr 8, 2024

zzl-7 Apr 9, 2024

zzl-7 commented Apr 5, 2024

vkorukanti left a comment

vkorukanti Apr 8, 2024

zzl-7 Apr 9, 2024

vkorukanti Apr 8, 2024

zzl-7 Apr 9, 2024

vkorukanti Apr 8, 2024

zzl-7 commented Apr 9, 2024

zzl-7 commented Apr 23, 2024

vkorukanti commented Apr 23, 2024

zzl-7 commented Apr 24, 2024 •

edited

Loading

vkorukanti left a comment

vkorukanti Apr 29, 2024

zzl-7 May 1, 2024

zzl-7 commented May 16, 2024

vkorukanti left a comment

vkorukanti May 23, 2024

zzl-7 May 28, 2024

vkorukanti May 23, 2024

zzl-7 May 28, 2024

vkorukanti May 29, 2024

vkorukanti May 23, 2024

zzl-7 May 28, 2024

vkorukanti May 23, 2024

zzl-7 May 28, 2024

vkorukanti May 23, 2024

zzl-7 May 28, 2024

zzl-7 commented May 28, 2024

vkorukanti left a comment

vkorukanti May 29, 2024

zzl-7 commented May 30, 2024 •

edited

Loading

Left	Right	Result
null	null	True
not null (A)	null	False
null	not null (B)	False
not null (A)	not null (B)	False
not null (A)	not null (A)	True

[Kernel] Change comparator expression to lazy evaluation #2853

[Kernel] Change comparator expression to lazy evaluation #2853

Conversation

zzl-7 commented Apr 5, 2024 • edited Loading

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzl-7 commented Apr 5, 2024

vkorukanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzl-7 commented Apr 9, 2024

zzl-7 commented Apr 23, 2024

vkorukanti commented Apr 23, 2024

zzl-7 commented Apr 24, 2024 • edited Loading

vkorukanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzl-7 commented May 16, 2024

vkorukanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzl-7 commented May 28, 2024

vkorukanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzl-7 commented May 30, 2024 • edited Loading

zzl-7 commented Apr 5, 2024 •

edited

Loading

zzl-7 commented Apr 24, 2024 •

edited

Loading

zzl-7 commented May 30, 2024 •

edited

Loading