-
Notifications
You must be signed in to change notification settings - Fork 40
Add export options validator #2435
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
Add export options validator #2435
Conversation
komamitsu
left a comment
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! 👍
| if (clusteringKeyNames == null) { | ||
| return; | ||
| } | ||
| String columnName = key.getColumnName(0); |
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.
Sorry, why are you only checking the first column?
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.
Jishnu is off for a bit so I took over this task and updated the code. I have fixed the validation and completely rewrote the unit tests to cover all scenarios.
- For the partition key(s) making sure all keys are provided and in order
- For clustering keys, make sure the order or partial order is correct. (full key A,B,C -->. so you can call with (A), (A,B), (A,B,C))
This fix/problem exposes a limitation in the current data loader where you can only call the data loader like this
./data-loader export -pk <KEY=VALUE>. This will be fixed so you can do <KEY=VALUE>,<KEY=VALUE>. This code is coming with the next PR so it will already be fixed. But just FYI.
@inv-jishnu As mentioned, make sure to update the CLI arguments for export in the next PR to protected List<ColumnKeyValue> partitionKeyValues;
brfrn169
left a comment
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! Thank you!
Torch3333
left a comment
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, thank you!
Co-authored-by: Peckstadt Yves <peckstadt.yves@gmail.com>
Co-authored-by: Peckstadt Yves <peckstadt.yves@gmail.com>
Co-authored-by: Peckstadt Yves <peckstadt.yves@gmail.com>
Co-authored-by: Peckstadt Yves <peckstadt.yves@gmail.com>
Co-authored-by: Peckstadt Yves <peckstadt.yves@gmail.com>
Co-authored-by: Peckstadt Yves <peckstadt.yves@gmail.com>
Description
I have added the export options validator and a related exception
Related issues and/or PRs
NA
Changes made
I have added the export options validator and a related exception
Checklist
Additional notes (optional)
Road map to merge remaining data loader core files. Current status
General
Export
Import
Release notes
NA