Adds per-column, query-time index skip option #12414
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12414 +/- ##
============================================
- Coverage 61.77% 61.76% -0.01%
Complexity 207 207
============================================
Files 2425 2437 +12
Lines 132753 133263 +510
Branches 20535 20651 +116
============================================
+ Hits 82005 82310 +305
- Misses 44730 44891 +161
- Partials 6018 6062 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Jackie-Jiang
added
feature
release-notes
|
tests added, this is ready for review. |
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java
Show resolved
Hide resolved
|
It LGTM. As said in the linked issue, my main concern is on the language to use. I think URL args is fine, but maybe we would need to have a bigger picture because the language we decide to use here is what we should use in future options with the same type |
| for (String columnConf : perColumnIndexSkip) { | ||
| String[] conf = columnConf.split("="); | ||
| if (conf.length != 2) { | ||
| continue; |
There was a problem hiding this comment.
Shouldn't we fail in this case?
There was a problem hiding this comment.
I think so, ya. I feel like failed parsing is better to end up as a failed query than a successful query without the intended options set.
There was a problem hiding this comment.
@gortiz would a org.apache.pinot.sql.parsers.parser.ParseException be a good choice here? I don't know if that exception is reserved for SQL not including query options, though.
There was a problem hiding this comment.
Actually it looks like there isn't precedent for throwing exceptions from within InstancePlanMakerImplV2#applyQueryOptions, so the "easiest" would be a RuntimeException
|
@gortiz anything that needs attention at the moment? I want to be sure I've addressed any comments and can't find any unaddressed at the moment. |
| return null; | ||
| } | ||
|
|
||
| String[] perColumnIndexSkip = indexSkipConfigStr.split("&"); |
There was a problem hiding this comment.
Suggest using StringUtils.split(indexSkipConfigStr, '&') which is more lightweight. The current API will try to do regex match. Same for other split calls
| @@ -361,6 +361,7 @@ public static class QueryOptionKey { | |||
| public static final String SERVER_RETURN_FINAL_RESULT = "serverReturnFinalResult"; | |||
| // Reorder scan based predicates based on cardinality and number of selected values | |||
| public static final String AND_SCAN_REORDERING = "AndScanReordering"; | |||
| public static final String INDEX_SKIP_CONFIG = "indexSkipConfig"; | |||
There was a problem hiding this comment.
(personal preference) I feel skipIndexes is more consistent with other option keys
|
@Jackie-Jiang I'll make a follow-up PR to address these by end of week |
|
Do we have documentation for this @egalpin ? |
|
@npawar Ah, thank you for the reminder! No, there's no formal docs for this yet. |
Closes #12355. Needs tests but opening for early feedback.
cc @kishoreg @Jackie-Jiang