-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve verification for map columns #14054
Improve verification for map columns #14054
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendan-driscoll Thanks for working on this. Looks good % a question.
Let's remove "Initial commit:" from the commit message and PR title.
@caithagoras Would you take a look?
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/MapColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/MapColumnValidator.java
Outdated
Show resolved
Hide resolved
Apologize for the delay. I'll get it reviewed by tomorrow morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at QueryUtil.functionCall
to simplify all callers for new FunctionCall
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/MapColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/MapColumnValidator.java
Outdated
Show resolved
Hide resolved
presto-verifier/src/main/java/com/facebook/presto/verifier/checksum/MapColumnValidator.java
Outdated
Show resolved
Hide resolved
Potentially, if key/values types are of other non-simple types, we'll be able to apply the respective column validator on the keys and values (with some restrictions). |
@brendan-driscoll Please squash the 2 commits. |
The PR is missing tests. Please take a look at |
@@ -297,9 +304,9 @@ public void testArray() | |||
ChecksumResult controlChecksum = new ChecksumResult( | |||
5, | |||
ImmutableMap.<String, Object>builder() | |||
.put("int_array_checksum", new SqlVarbinary(new byte[] {0xa})) | |||
.put("int_array_checksum", new SqlVarbinary(new byte[]{0xa})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your formatting is off. Please follow 7e and 7f in https://our.internmc.facebook.com/intern/wiki/DataInfra/Presto/Source/
to install Airlift code style, and then format the file with (option + cmd + L).
.put(MAP_COLUMN, new ColumnMatchResult(false, "control(checksum: 0a, keys_checksum: 0b, values_checksum: 0c, cardinality_sum: 3) " + | ||
"test(checksum: 1a, keys_checksum: 0b, values_checksum: 0c, cardinality_sum: 3)")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: all arguments on the same line, or one argument per line. Same below.
|
|
@brendan-driscoll Would you please squash commits? |
41ab163
to
cb58b59
Compare
@brendan-driscoll Why is this a fix for #3809 ? |
@brendan-driscoll Thanks for squashing commits. Please, update the commit to match the guidelines at https://chris.beams.io/posts/git-commit/ For example,
|
Please update the commit title and the PR title. |
@viczhang861 That was a typo, should have been #13809 |
cb58b59
to
c364084
Compare
@brendan-driscoll There are some merge conflicts. Would you rebase on top of latest master and update the PR? |
2cbe269
to
1022df1
Compare
@brendan-driscoll Test failures seem to be related to the changes. Would you take a look? |
f673412
to
a8a9447
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % nit
please also fix Travis test
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/MapColumnValidator.java
Outdated
Show resolved
Hide resolved
Add checksums for map key, values and map sizes.
006c1f3
to
3b6846f
Compare
Part of #13809