-
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-31504 Improve how ParquetWriter creates columns from ECL rows #18430
HPCC-31504 Improve how ParquetWriter creates columns from ECL rows #18430
Conversation
After rebasing to 9.4.x it has conflicts with defines solved by JIRA HPCC-31301. |
I'm waiting for @dcamper to do a first review. Let me know if you would find it valuable for me to take a look first. |
@ghalliday If you could take a look that would be great. Thank you! |
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.
Nice to see RapidJSON removed!
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.
I've scanned the changes and they look like a good improvement - anything that deletes 300 lines has got to be good! I didn't spot any problems.
@jackdelv can you change this to a normal PR rather than a draft, and I will merge? Draft normally means it isn't considered ready for merging, and it has been opened for initial discussion review. Otherwise please open as non-draft. |
@ghalliday Changed to normal PR. Thank you! |
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.
@jackdelv this is not currently building on some of the systems.
A short-term fix is to add
#undef BOOL
after the #includes
A better fix, which seems to work, is to delete the definition from platform.h (line 364).
@ghalliday Doesn't this PR, #18310, already fix this, or is it easier to include the fix in this one? |
Well spotted. I will cherry-pick that back to 9.4.x and then rerun the tests. |
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31504 Jirabot Action Result: |
e8a2e88
into
hpcc-systems:candidate-9.4.x
Type of change:
Checklist:
Smoketest:
Testing: