-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Add strict mode in Routine load, Stream load and Mini load #1677
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
Conversation
gensrc/thrift/FrontendService.thrift
Outdated
| 17: optional bool negative | ||
| 18: optional i32 timeout | ||
| 18: optional bool strictMode | ||
| 19: optional i32 timeout |
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.
Don't modify the index of old field. If you change the index, this may not compatible with old format
| origStmt = Text.readString(in); | ||
|
|
||
| if (Catalog.getCurrentCatalogJournalVersion() >= FeMetaVersion.VERSION_59) { | ||
| strictMode = in.readBoolean(); |
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.
If you make all old routine load job to strict mode. This may cause many data to be filtered
| private String partitions; | ||
| private String path; | ||
| private boolean negative; | ||
| private boolean strictMode = RoutineLoadJob.DEFAULT_STRICT_MODE; |
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.
Why put this DEFAULT_STRICT_MODE in RoutineLoadJob?
| .add(MAX_BATCH_INTERVAL_SEC_PROPERTY) | ||
| .add(MAX_BATCH_ROWS_PROPERTY) | ||
| .add(MAX_BATCH_SIZE_PROPERTY) | ||
| .add(LoadStmt.STRICT_MODE) |
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.
should we update our Document about this property?
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 will update.
morningman
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
1c370a1 to
239467e
Compare
morningman
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
imay
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
No description provided.