-
Notifications
You must be signed in to change notification settings - Fork 304
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
HPCC-32228 Add fixed_size_binary data type to Parquet Plugin #18888
Conversation
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32228 Jirabot Action Result: |
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.
@ilhan2316 I think it's close. There are just a few style issues that need to be fixed.
There were a few cases where extra lines were added and not needed. I commented those with Extra Line.
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.
@ilhan2316 It looks good. There are still a few places with trailing spaces or too few spaces. I have placed comments at those points.
I think that after these changes the documentation should be updated. Please create a JIRA for updating the Parquet documentation with this type and large_list.
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.
@ilhan2316 It's close. I just have one question that may require a little investigation.
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.
It is very close. There are just a few style issues, but the changes look correct.
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.
One last style nitpick.
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.
@ilhan2316 The changes look good.
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.
Looks good.
Offline commented noted here for posterity and note-keeping: Investigation into using .emplace_back() instead of .push_back() may be a good idea. |
@ilhan2316 Please squash to a single commit. |
Signed-off-by: Ilhan Gelle <[email protected]>
@ghalliday This is ready to be merged. Please take a look. |
be0072a
to
71b623c
Compare
@ilhan2316 please can you squash the commits |
@ghalliday I have squashed. |
@jackdelv please can you review/approve |
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.
@ghalliday The changes are correct.
727c51a
into
hpcc-systems:candidate-9.6.x
Type of change:
Checklist:
Smoketest:
Testing: