-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow String / numeric data type for deleteRecordColumn config #12222
Allow String / numeric data type for deleteRecordColumn config #12222
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12222 +/- ##
============================================
+ Coverage 61.57% 61.59% +0.02%
- Complexity 1152 1153 +1
============================================
Files 2415 2416 +1
Lines 131167 131205 +38
Branches 20245 20250 +5
============================================
+ Hits 80769 80821 +52
+ Misses 44501 44473 -28
- Partials 5897 5911 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hey @walterddr @navina can you help review this PR? Should we remove boolean check altogether or add String / Numeric as additional checks? |
@navina please share your thoughts. i am ok with the approach as long as we agree upon the new contract |
i think it will be better to add additional string/numeric checks in TableConfigUtils. Do you think that is possible? Apologies for the delay in response @tibrewalpratik17 . I missed the notification. |
Yes let me do that instead. |
Updated! cc @walterddr @navina |
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
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. Thanks for the quick turnaround and testing for this.
One minor note is that we can say "Column=%s doesn't exist" instead of "Invalid delete record column found".
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!
Relates to #12217.
This patch add the criteria that
deleteRecordColumn
field can be of String / numeric data type thus allowing String based "true"/"false"/"0"/"1" to work or int based 0/1 as well based on BooleanUtils class.Tested in one of our clusters using String based field for deleted column and worked as expected.