[verifier] Add option to validate array(float) via error margin. by spershin · Pull Request #22143 · prestodb/presto

spershin · 2024-03-08T23:26:41Z

Description

Basically allow verifier to validate array(float) and array(double) columns in the same way as we validate float and double columns. Later we can extend this to the maps as well.

Motivation and Context

We are planning to use this when comparing Presto Native with Presto Java to reduce the noise from array(double) columns. This will also strengthen the Presto verification as non-deterministic array(double) column could hide a valid correctness issue in another column by marking query SKIPPED in the verifier.

Test Plan

Added new unit test and updated existing one.
Ran verifier on a query producing non-deterministic array(double) (artificial query).
Result is successful: the query passes correctness in the new verifier, while getting skipped in the old one.
Successfully run real use case verifier suite on the new verifier.

== NO RELEASE NOTE ==

spershin · 2024-03-08T23:27:40Z

Submitted PR a bit too early - the validation part has not been implemented yet. But the PR is good to see where it is all going.

elharo · 2024-03-10T00:59:00Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

-    public ArrayColumnChecksum(@Nullable Object checksum, @Nullable Object cardinalityChecksum, long cardinalitySum)
+    public ArrayColumnChecksum(
+            @Nullable Object checksum,
+            @Nullable Object sum,


Why is this an Object instead of a more specific type? For that matter why is checksum an Object? I assume there's a reason, but this is surprising me.

@elharo
I'm not sue, I was following the existing code from FloatingPointColumnValidator.
As I can see classes have long for counts and Object for everything else, why - not sure. :)

Let's watch out for Chesterton's Fence. Before proceeding we should figure out why checksum is an Object. Presumably there's a reason but maybe that reason applies to the new cardinality checksum and maybe it doesn't. It's also possible checksum shouldn't have been an Object in the first place, in which case we don't have to fix this now, but let's not repeat the mistake. Any idea who could shed some light on this?

I can't see a good reason why sum is an Object in FloatingPointColumnChecksum. the only places it's used, it's blindly cast to a double https://github.com/prestodb/presto/blob/master/presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/FloatingPointColumnValidator.java#L145-L146. For arrays, the sum is not necessarily a double, and I think it would be better not to assume it's a double here. (i mean you've only implemented it for double type, but that's more incidental. You should be able to do sum validation on arrays of other numeric types)

Regarding why Objects are used for checksums throughout, I'm not sure. I assume it was most convenient, but I also suspect the code could be improved to use more specific types. In case it's helpful for gleaning more, the object usage seems to have been introduced here 04d45ea and here: 7c48103 if that helps at all.

Some additional thoughts - since we could imagine wanting to add special validation for other types. It might be better to make this argument a ColumnChecksum object and pass in a FloatingPointColumnChecksum instead of adding all the fields from FloatingPointColumnChecksum directly here. That way later we can add validation for other kinds of arrays, and just pass in the appropriate columnchecksum object and do the corresponding validation.

It might be interesting even to flip the logic a bit so ColumnChecksum would have a validate method that implementations override to call the validation appropriate for the type (something like columnChecksum.validate(Checksum other, Column column). That way you don't even need to worry about what type it might be here. you would just call validate on whatever ColumnChecksum you end up with

It's actually weird that currently validation is done by object equality, especially considering that for float checksums that's definitely not what we want. (From a quick read through, I'm pretty sure that FloatingPointColumnChecksum at least is not used in object equality checks, but it's not at all clear and definitely not enforced) I would be very in favor of moving away from this essentially untyped everything is just an Object land.

Finally, I think we have the same problem you're solving here for map types, where it does checksums on the keys and values.

@elharo

Let's watch out for Chesterton's Fence.

If we follow that rule you mentioned - we should not deviate of what already is in there. I suggest approach of 'as is' and then follow up if we decide to change this. I would like this PR to stay focused on its task, if possible.

@rschlussel

Some additional thoughts - since we could imagine wanting to add special validation for other types.

Yes, we would likely go that way. Maps of doubles is the next candidate, especially for values part. I was thinking about it, however wanted to stay focused on just arrays now to unblock NGA use case quickly. We can adjust few things to fit Maps more as we go and introduce more unconventional support.

Finally, I think we have the same problem you're solving here for map types, where it does checksums on the keys and values.

Not only that, but the structural types. like rows, but not sure if we want to really go there. Let's fo one step at a time? Harder to break existing stuff that way.

@elharo @rschlussel
I believe objects are being used, because the aggregation can return NULL when there are no rows.
I'm currently debugging exactly such case.
To handle these nulls we use Object rather than a direct type.

Interesting. but then why not use a Double type (which can be null since it's an object) rather than a double primitive type?

Good question.
We didn't think about in the beginning?

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

rschlussel · 2024-03-11T14:57:59Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

-    public ArrayColumnChecksum(@Nullable Object checksum, @Nullable Object cardinalityChecksum, long cardinalitySum)
+    public ArrayColumnChecksum(
+            @Nullable Object checksum,
+            @Nullable Object sum,


I can't see a good reason why sum is an Object in FloatingPointColumnChecksum. the only places it's used, it's blindly cast to a double https://github.com/prestodb/presto/blob/master/presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/FloatingPointColumnValidator.java#L145-L146. For arrays, the sum is not necessarily a double, and I think it would be better not to assume it's a double here. (i mean you've only implemented it for double type, but that's more incidental. You should be able to do sum validation on arrays of other numeric types)

Regarding why Objects are used for checksums throughout, I'm not sure. I assume it was most convenient, but I also suspect the code could be improved to use more specific types. In case it's helpful for gleaning more, the object usage seems to have been introduced here 04d45ea and here: 7c48103 if that helps at all.

spershin · 2024-03-11T22:18:26Z

The latest update includes:

Fixed failing tests.
Refactored validate() from FloatingPointColumnValidator class to FloatingPointColumnChecksum.
Fixed query generation for array(double) validator (infinity issue).
Added TestChecksumValidator.testFloatingPointArray() unit test.
Added support of array(double) and array(real) to TestChecksumValidator.testChecksumQuery() unit test.
Improved TestChecksumValidator.testChecksumQuery() to tell which line does not match.

spershin · 2024-03-12T02:47:35Z

Fixed Checkstyle errors.

spershin · 2024-03-12T03:02:07Z

More Checkstyle errors...

spershin · 2024-03-12T03:11:24Z

Aaaand Checkstyle again...

spershin · 2024-03-12T06:51:11Z

Fixed the checksum query.

kewang1024

Thanks @spershin for helping improve the verification of array(double)

presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

kewang1024 · 2024-03-13T23:16:36Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

+        boolean useFloatingPointPath = useFloatingPointPath(column);
+
+        ArrayColumnChecksum controlChecksum = toColumnChecksum(column, controlResult, useFloatingPointPath);
+        ArrayColumnChecksum testChecksum = toColumnChecksum(column, testResult, useFloatingPointPath);
+
+        // Not floating point case (we have '$checksum' column).
+        if (!useFloatingPointPath) {


We don't need useFloatingPointPath here, we can infer this information from ArrayColumnChecksum structure

if ArrayColumnChecksum 's checksum is not null, then it's regular ArrayColumnChecksum
If it is null, then it's the float point check

if ArrayColumnChecksum 's checksum is not null, then it's regular ArrayColumnChecksum If it is null, then it's the float point check

@kewang1024

That was my 1st choice, but it all breaks on the corner case when we get checksums on empty tables (no rows) and then a bunch of checksum columns have nulls.

I realized after that if we are triggering floating point path checking the array element type - we should be consitent and always check using that approach. :)

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ChecksumResult.java

kewang1024 · 2024-03-13T23:50:04Z

...erifier/src/main/java/com/facebook/presto/verifier/checksum/FloatingPointColumnChecksum.java

+        }
+    }
+
+    public static ValidateResult validate(


Although changed to static function, I think it might make more sense to still put this function in FloatingPointColumnValidator.java

FloatingPointColumnChecksum is only the data structure, FloatingPointColumnValidator will handle the validation logic

@kewang1024

Initially I kept it in the FloatingPointColumnValidator, but then I thought that this logic is better to be incapsulated in the structure that keeps the data. It already implements equals(), toString().
Make sense to put validate() here too, imho.

Let me know if you really prefer it back to the validator class.

I totally get why we move it to the data structure, but might still prefer it in the validator class

I think either all validation in should be in the respective checksum classes or none of them(also, might fit better in the validate class if it were a non-static method like equals e.g. floatingPointChecksum.validate(FloatingPointChecksum other)). As things stand it seems a bit out of place, and I agree with @kewang1024 that it would be better in the validate class.

spershin · 2024-03-14T21:31:34Z

The last update:

Move ArrayColumnValidator.validate to be above private methods as per suggestion.
Remove unused ChecksumResult.hasChecksum()
Refactor ArrayColumnChecksum to contain FloatingPointColumnChecksum. Later we can use the same trick for Map keys and values.

spershin · 2024-03-14T21:49:19Z

@kewang1024 , @rschlussel
This change is close to the final change.
Code review is appreciated.
The previous change works successfully on the 10 NGA queries batch.
Will test on the real workload the final change (after comments are addressed) before merging.

kewang1024

Thanks again! Overall looks good, some NIT

kewang1024 · 2024-03-15T18:34:27Z

...erifier/src/main/java/com/facebook/presto/verifier/checksum/FloatingPointColumnChecksum.java

+        }
+    }
+
+    public static ValidateResult validate(


I totally get why we move it to the data structure, but might still prefer it in the validator class

kewang1024 · 2024-03-15T18:38:38Z

...erifier/src/main/java/com/facebook/presto/verifier/checksum/FloatingPointColumnChecksum.java

    }
+
+    // Class being used to return a result from the validate() method.
+    public static class ValidateResult


Can we move this class to the validator as well?

kewang1024 · 2024-03-15T18:40:32Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

+            return new ArrayColumnChecksum();
+        }
+
+        long rowCount = checksumResult.getRowCount();


we can remove this line

kewang1024 · 2024-03-15T18:56:30Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

+    private final FloatingPointColumnChecksum floatingPointChecksum;

-    public ArrayColumnChecksum(@Nullable Object checksum, @Nullable Object cardinalityChecksum, long cardinalitySum)
+    // Constructor for array(non- floating point) types.


NIT: non-floating

kewang1024 · 2024-03-15T18:57:39Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

    private final Object cardinalityChecksum;
    private final long cardinalitySum;
+    // For array(floating point) we have extra aggregations collected.
+    private final FloatingPointColumnChecksum floatingPointChecksum;


Let's make it Optional

kewang1024 · 2024-03-15T18:58:06Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

        this.checksum = checksum;
        this.cardinalityChecksum = cardinalityChecksum;
        this.cardinalitySum = cardinalitySum;
+        this.floatingPointChecksum = new FloatingPointColumnChecksum();


this.floatingPointChecksum = Optional.empty()

kewang1024 · 2024-03-15T18:59:23Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

+        this.checksum = null;
+        this.cardinalityChecksum = null;
+        this.cardinalitySum = 0;
+        this.floatingPointChecksum = new FloatingPointColumnChecksum();


this.floatingPointChecksum = Optional.empty()

kewang1024 · 2024-03-15T18:59:44Z

...erifier/src/main/java/com/facebook/presto/verifier/checksum/FloatingPointColumnChecksum.java

        this.rowCount = rowCount;
    }

+    // Constructor to handle cases when the result table is empty (sum aggregations returns null).


We can remove this one

Actually, I would prefer to leave it.
We use it, check the updated changes.

kewang1024 · 2024-03-15T19:58:16Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

+        ArrayColumnChecksum testChecksum = toColumnChecksum(column, testResult, useFloatingPointPath);

-        return ImmutableList.of(new ColumnMatchResult<>(Objects.equals(controlChecksum, testChecksum), column, controlChecksum, testChecksum));
+        // Not floating point case (we have '$checksum' column).


Non-floating point to be consistent?

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

rschlussel · 2024-03-15T20:16:05Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

+        }
+    }
+
+    public static FloatingPointColumnChecksum.ValidateResult validateFloatingPoint(


it's weird to me that this logic is over here, but the rest of the validation logic is in ArrayColumnValidator. I would do one or the other.

Moved validate() logic to Validate classes.

rschlussel · 2024-03-15T20:18:51Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

+        // Check the non floating point members first.
+        if (!Objects.equals(controlChecksum.getCardinalityChecksum(), testChecksum.getCardinalityChecksum()) ||
+                !Objects.equals(controlChecksum.getCardinalitySum(), testChecksum.getCardinalitySum())) {
+            return new FloatingPointColumnChecksum.ValidateResult(false, Optional.of("cardinality mismatch"));


Do we need this new class? Can we use ColumnMatchResult instead?

ColumnMatchResult is a templated class and we must return different instantiations of it from ArrayColumnChecksum and FloatingPointColumnChecksum.

I decide to have that small class to carry actual validation result, except all the columns info, which will be provided by concrete validators.

The ValidateResult still feel not needed, I don't see particular bad effect of using ColumnMatchResult

Ok, I guess, we can get rid of it.

rschlussel · 2024-03-15T20:20:19Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

 {
+    private final double relativeErrorMargin;
+    private final double absoluteErrorMargin;
+    private final boolean arrayFloatingPointErrorMargin;


I find this name unclear. Maybe something like "useErrorMarginForFloatArrays"?

Replaced by useErrorMarginForFloatingPointArrays and "use-error-margin-for-floating-point-arrays".

rschlussel · 2024-03-15T20:39:57Z

...erifier/src/main/java/com/facebook/presto/verifier/checksum/FloatingPointColumnChecksum.java

+        }
+    }
+
+    public static ValidateResult validate(


I think either all validation in should be in the respective checksum classes or none of them(also, might fit better in the validate class if it were a non-static method like equals e.g. floatingPointChecksum.validate(FloatingPointChecksum other)). As things stand it seems a bit out of place, and I agree with @kewang1024 that it would be better in the validate class.

rschlussel · 2024-03-15T20:41:50Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

+    @Inject
+    public ArrayColumnValidator(VerifierConfig config)
+    {
+        this.relativeErrorMargin = config.getRelativeErrorMargin();


I wonder if we should inject FloatingPointColumnValidator instead, and then can just call floatingPointColumnValidator.validate() and leave these configs as implementation details of the floatingpointcolumnvalidator.

@rschlussel
Do you mean make FloatingPointColumnValidator a member of ArrayColumnValidator and pass the former to the constructor of the latter?
We can do this.

didn't mean to approve. clicked the wrong button

spershin · 2024-03-15T22:14:42Z

@kewang1024 @rschlussel

Thanks for the review.
I have addressed the comments so far.

kewang1024 · 2024-03-20T22:38:08Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

        return column.getName() + "$checksum";
    }

+    private static String getSumColumnAlias(Column column)


we can remove this

kewang1024 · 2024-03-20T22:38:46Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

+
        return new ArrayColumnChecksum(
-                checksumResult.getChecksum(getChecksumColumnAlias(column)),
+                checksumResult.getChecksum(getSumColumnAlias(column)),


we can directly do FloatingPointColumnValidator.getSumColumnAlias(column)

kewang1024 · 2024-03-20T23:05:17Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

+        // Check the non floating point members first.
+        if (!Objects.equals(controlChecksum.getCardinalityChecksum(), testChecksum.getCardinalityChecksum()) ||
+                !Objects.equals(controlChecksum.getCardinalitySum(), testChecksum.getCardinalitySum())) {
+            return new FloatingPointColumnChecksum.ValidateResult(false, Optional.of("cardinality mismatch"));


The ValidateResult still feel not needed, I don't see particular bad effect of using ColumnMatchResult

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

spershin · 2024-03-21T19:19:12Z

Addressed ArrayColumnChecksum ctor comments.

rschlussel

Thanks for the update. A few comments, but generally looks good!

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

rschlussel · 2024-03-22T14:54:09Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java

    {
-        ArrayColumnChecksum controlChecksum = toColumnChecksum(column, controlResult);
-        ArrayColumnChecksum testChecksum = toColumnChecksum(column, testResult);
+        checkArgument(


Just want to confirm there's some other part of verifier that checks that row counts match before getting here, right? and so we have this argument check to sanity check the code, but we shouldn't hit it.

I'm not sure about it, honestly - repeating what FloatingPointColumnValidator is doing.

Yes, it looks like a sanity check to me.
I think we can remove it from ArrayColumnValidator and move it in FloatingPointColumnValidator from overridden validate() to the public validate(), which is called from the ArrayColumnValidator.

Actually, cannot do that. It needs ChecksumResult and in the public validate() we only have ColumnChecksum.
Got to leave it as is.

rschlussel · 2024-03-22T15:00:47Z

...rc/test/java/com/facebook/presto/verifier/resolver/TestStructuredColumnMismatchResolver.java

@@ -42,10 +44,10 @@ public TestStructuredColumnMismatchResolver()
    @Test
    public void testResolveArray()


Can you update the StructuredColumnsMismatchResolver and then add a test that we don't resolve column mismatches for float arrays if we're using error margin validation.

@rschlussel

Can I get a bit more context on that?
What does StructuredColumnMismatchResolver do and why it should not resolve column mismatches for float arrays if we're using error margin validation?
Thanks!

rschlussel · 2024-03-22T15:01:15Z

presto-verifier/src/test/java/com/facebook/presto/verifier/checksum/TestChecksumValidator.java

    }

+    @Test
+    public void testFloatingPointArray()


can you add a test for the empty table case too?

Yes, I was going to, but hit that snag with nulls in the Map.
I can see if I can use that HashMap to generate the nulls.

rschlussel · 2024-03-22T15:08:17Z

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

    public String toString()
    {
-        return format("checksum: %s, cardinality_checksum: %s, cardinality_sum: %s", checksum, cardinalityChecksum, cardinalitySum);
+        if (!floatingPointChecksum.isPresent()) {


nit: instead of repeating everything for constructing the string, you can use MoreObject.toStringHelper to construct a nicely printed string with all the fields. Something like:

MoreObjects.ToStringHelper toStringHelper = MoreObjects.toStringHelper(this) .add("cardinality_checksum", cardinalityChecksum) .add("cardinality_sum", cardinalitySum) if(floatingPointChecksum.isPresent() { toStringHelper.add("floating_point_checksum", floatingPointchecksum.get()); } else { toStringHelper.add("checksum", checksum); } return toStringHelper.toString();

not sure if it's important that this toString matches the toStrings of the other checksums, so I'll add the caveat that the generated string will look slightly different than what you have, see the docs for example output https://guava.dev/releases/19.0/api/docs/com/google/common/base/MoreObjects.html#toStringHelper(java.lang.Object)

@rschlussel
I don't know. I don't feel that this change worth the trouble at the moment.
You are saying "to construct a nicely printed string with all the fields" - are they going to be more nicely printed than with the current code? How?

Maybe we can consider a small refactor followup to move all Checksums to such format?

not necessarily nicer than the current code. just helps you with not duplicating for the floating point vs. non-floating point case. It would look something like ArrayColumnChecksum{cardinality_checksum=123, cardinality_sum=345, ...}. anyway, fine to leave it as is.

presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnChecksum.java

Basically allow verifier to validate array(float) and array(double) columns in the same way as we validate float and double columns. Later we can extend this to the maps as well.

spershin requested a review from a team as a code owner March 8, 2024 23:26

spershin requested a review from presto-oss March 8, 2024 23:26

spershin force-pushed the ArrayFloatingPointErrorMargin branch from 9e3445e to 352c585 Compare March 9, 2024 00:49

spershin changed the title ~~[verifier] Add option yo validate array(float) via error margin.~~ [verifier] Add option to validate array(float) via error margin. Mar 9, 2024

spershin force-pushed the ArrayFloatingPointErrorMargin branch from 352c585 to efb5bdb Compare March 9, 2024 03:21

elharo requested a review from rschlussel March 10, 2024 00:57

elharo reviewed Mar 10, 2024

View reviewed changes

rschlussel reviewed Mar 11, 2024

View reviewed changes

spershin force-pushed the ArrayFloatingPointErrorMargin branch from efb5bdb to 1484bca Compare March 11, 2024 22:17

spershin force-pushed the ArrayFloatingPointErrorMargin branch from 1484bca to acb8c44 Compare March 12, 2024 02:47

spershin force-pushed the ArrayFloatingPointErrorMargin branch from acb8c44 to 5892fa1 Compare March 12, 2024 03:01

spershin force-pushed the ArrayFloatingPointErrorMargin branch from 5892fa1 to b6b090a Compare March 12, 2024 03:10

spershin force-pushed the ArrayFloatingPointErrorMargin branch from b6b090a to b91473d Compare March 12, 2024 06:50

spershin force-pushed the ArrayFloatingPointErrorMargin branch 3 times, most recently from 5b90982 to bf4a7b1 Compare March 13, 2024 23:40

kewang1024 reviewed Mar 13, 2024

View reviewed changes

spershin force-pushed the ArrayFloatingPointErrorMargin branch from bf4a7b1 to 620920d Compare March 14, 2024 21:30

spershin requested review from kewang1024 and rschlussel March 14, 2024 21:49

kewang1024 reviewed Mar 15, 2024

View reviewed changes

rschlussel previously approved these changes Mar 15, 2024

View reviewed changes

spershin force-pushed the ArrayFloatingPointErrorMargin branch from 620920d to 351c344 Compare March 15, 2024 22:13

spershin requested review from kewang1024 and rschlussel March 15, 2024 22:14

spershin force-pushed the ArrayFloatingPointErrorMargin branch 2 times, most recently from d732c5f to 039b69e Compare March 15, 2024 23:30

kewang1024 previously approved these changes Mar 20, 2024

View reviewed changes

spershin dismissed kewang1024’s stale review via 44d1c91 March 20, 2024 23:45

spershin force-pushed the ArrayFloatingPointErrorMargin branch 2 times, most recently from 44d1c91 to b8c6c4b Compare March 21, 2024 19:18

rschlussel reviewed Mar 22, 2024

View reviewed changes

spershin force-pushed the ArrayFloatingPointErrorMargin branch from b8c6c4b to f7e5e3d Compare March 22, 2024 20:19

rschlussel previously approved these changes Mar 22, 2024

View reviewed changes

[verifier] Add option to validate array(float) via error margin.

0c896e3

Basically allow verifier to validate array(float) and array(double) columns in the same way as we validate float and double columns. Later we can extend this to the maps as well.

spershin dismissed rschlussel’s stale review via 0c896e3 March 22, 2024 22:52

spershin force-pushed the ArrayFloatingPointErrorMargin branch from f7e5e3d to 0c896e3 Compare March 22, 2024 22:52

pranjalssh self-requested a review March 22, 2024 23:07

pranjalssh approved these changes Mar 22, 2024

View reviewed changes

spershin merged commit f62eb9a into prestodb:master Mar 25, 2024

gggrace14 mentioned this pull request Aug 6, 2024

Allow Verifier to validate varchar as floating point #23312

Merged

		@@ -42,10 +44,10 @@ public TestStructuredColumnMismatchResolver()
		@Test
		public void testResolveArray()

Conversation

spershin commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Test Plan

Uh oh!

spershin commented Mar 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spershin commented Mar 11, 2024

Uh oh!

spershin commented Mar 12, 2024

Uh oh!

spershin commented Mar 12, 2024

Uh oh!

spershin commented Mar 12, 2024

Uh oh!

spershin commented Mar 12, 2024

Uh oh!

kewang1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kewang1024 Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spershin commented Mar 14, 2024

Uh oh!

spershin commented Mar 14, 2024

Uh oh!

kewang1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

spershin commented Mar 8, 2024 •

edited

Loading

kewang1024 Mar 13, 2024 •

edited

Loading