Add sum of cardinality for array column verifier results#13916
Add sum of cardinality for array column verifier results#13916mbasmanova merged 2 commits intoprestodb:masterfrom
Conversation
mbasmanova
left a comment
There was a problem hiding this comment.
Looks good to me, but let's have @caithagoras take a look as well.
|
@ShenYi would you sign EasyCLA? |
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
I tried with corporate Facebook, got message "you are not whitelisted...". I have sent a message through the "Contact CLA Manager" link on the same page. |
@aweisberg Ariel, is this something you could help with? |
There was a problem hiding this comment.
Conceptually, this PR does 2 things.
- Repurpose
OrderableArrayColumnValidatorasArrayColumnValidator - Add array cardinality in
ArrayColumnValidator.
For readability purpose, it would be better to separate it in to 2 commits. You can use interactive rebase to edit multiple commits. In case you're not familiar with this command, here is a reference: https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
|
@ShenYi @mbasmanova I can see EasyCLA green now. |
|
@ShenYi Release notes: |
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/test/java/com/facebook/presto/verifier/checksum/TestChecksumValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/ArrayColumnValidator.java
Outdated
Show resolved
Hide resolved
|
Also, Travis seems to be broken, although not relevant to this PR. |
|
Please ping on the PR when the comments are addressed, and @mbasmanova will merge for you. |
|
@mbasmanova Addressed all the comments. |
|
@ShenYi Ping me when Travis is green and I'll merge. |
|
@ShenYi Release notes are only for changes that users / customers sees and cares about, so you can remove the first item, which is a refactoring. |
|
@ShenYi I'm seeing this PR closing and reopening again and again. What's causing this? |
Refactor OrderableArrayColumnValidator into ArrayColumnValidator. Add logic to also support non-orderable array types.
For array column mismatch, add additional information of the total number of elements across all rows.
|
@mbasmanova test passed and the change is ready to be merged now. One of the test kept failing due to a bad maven zip from repository during the initial build. All subsequent builds load the same corrupted package from travis cache. Build passed after I manually deleted the maven dependencies for the pull request. |
|
@ShenYi Thank you for the contribution. |

For array column mismatch, this change adds additional information
of the total number of elements across all rows for the same column.
Part of #13809