You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "davecromberge (via GitHub)" <gi...@apache.org> on 2023/12/15 22:52:39 UTC
[PR] Shared aggregations in StarTree [pinot]
davecromberge opened a new pull request, #12164:
URL: https://github.com/apache/pinot/pull/12164
This PR relates to https://github.com/apache/pinot/issues/12111
Some aggregation functions have different calcite signatures but can leverage the same underlying pre-computed aggregate. This is commonly true for many sketch functions where the underlying aggregate is the sketch itself which is typically stored as an array of bytes.
To use these functions at query time together with a StarTree, the aggregate would need to be duplicated. This is inefficient in practice because data volume increases and segments typically have fewer rows, often adversely affecting query performance for certain query patterns.
To address this problem it is possible to encode the association via a mapping between a query time aggregate and the underlying index value aggregate. This can be done implicitly or explicitly by allowing the user to encode the function in the `AggregateSpec` within the Table configuration.
However, there are properties of the system that require careful consideration for these changes. Ultimately, it's not clear whether a segment's metadata should reflect the query aggregates that are supported or the value aggregates that are actually stored. Some use cases affected are:
- StarTree index rebuild when metadata is compared to aggregation spec configuration
- StarTree index fit and whether it covers a query
From an initial attempt and investigation it appears correct to reflect what the segment actually contains in its metadata.
Finally, if a relationship changes over time between a query aggregate and the value aggregate, this might result in undesirable behaviour if a segment was not actually constructed with the new mapped value in mind.
I'd be grateful for any input on this work and how best to proceed.
`release-notes`:
- New configuration options
- StarTree efficiency optimization
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1459939202
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/store/StarTreeLoaderUtils.java:
##########
@@ -100,7 +100,16 @@ public StarTreeV2Metadata getMetadata() {
@Override
public DataSource getDataSource(String columnName) {
- return dataSourceMap.get(columnName);
+ DataSource result = dataSourceMap.get(columnName);
+ // Some query columnNames could be supported by the underlying value aggregation type; do a secondary lookup
Review Comment:
Resolving before looking up the metadata is a little tricky due to possible knock-on effects to the StarTreeIndexReader. I haven't been able to formulate a straightforward solution (yet) - any ideas would be welcome.
Also, I reverted to checking the columnName directly against the map without resolving stored types because this is necessary for group by queries where the columnName might be a dimension, not necessarily a function column pair.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1433184478
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java:
##########
@@ -31,6 +31,7 @@
public class DistinctCountHLLPlusValueAggregator implements ValueAggregator<Object, HyperLogLogPlus> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE = AggregationFunctionType.DISTINCTCOUNTHLL;
Review Comment:
I see. The old one is a bug, and we can just fix it here. There should be no consequence
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #12164:
URL: https://github.com/apache/pinot/pull/12164
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1459517429
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -497,4 +497,35 @@ public static AggregationFunctionType getAggregationFunctionType(String function
}
}
}
+
+ /**
+ * Returns the stored {@code AggregationFunctionType} used to create the underlying value in the segment or index.
+ * Some aggregation function types share the same underlying value but are used for different purposes in queries.
+ * @param aggregationType the aggregation type used in a query
+ * @return the underlying value aggregation type used in storage e.g. StarTree index
+ */
+ public static AggregationFunctionType getAggregatedFunctionType(AggregationFunctionType aggregationType) {
Review Comment:
Addressed in https://github.com/apache/pinot/pull/12164/commits/2ca0cf588a484cf40c8ec57cb8c482b24f72ce71
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12164:
URL: https://github.com/apache/pinot/pull/12164#issuecomment-1911235685
@davecromberge I just pushed a commit with the following changes:
- During query time, resolve the stored type before looking up the metadata so that no fallback is required
- Handle backward-compatibility for old star-tree index where function might not be stored type
- Add a test for querying non-stored type
I did a commit instead of leaving comments because the code is spread in multiple classes and is kind of hard to explain. Let me know if the commit looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12164:
URL: https://github.com/apache/pinot/pull/12164#issuecomment-1912492125
@davecromberge Thanks for the contribution! This feature touches the whole flow from index creation, loading, all the way to query serving. Your work is super impressive given you are relatively new to this project. Looking forward to more contributions from you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1433107168
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/BaseSingleTreeBuilder.java:
##########
@@ -133,7 +133,9 @@ static class Record {
"Dimension: " + dimension + " does not have dictionary");
}
- TreeMap<AggregationFunctionColumnPair, AggregationSpec> aggregationSpecs = builderConfig.getAggregationSpecs();
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> aggregationSpecs =
+ StarTreeBuilderUtils.deduplicateAggregationSpecs(builderConfig.getAggregationSpecs());
Review Comment:
Similarly can we directly resolve the `AggregationFunctionType` within the builder config?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java:
##########
@@ -31,6 +31,7 @@
public class DistinctCountHLLPlusValueAggregator implements ValueAggregator<Object, HyperLogLogPlus> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE = AggregationFunctionType.DISTINCTCOUNTHLL;
Review Comment:
This is incorrect. It should be `DISTINCTCOUNTHLLPLUS`, which explains why the test failed
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/StarTreeIndexReader.java:
##########
@@ -121,7 +124,9 @@ private void mapBufferEntries(int starTreeId,
_dataBuffer, ByteOrder.BIG_ENDIAN));
}
// Load metric (function-column pair) forward indexes
- for (AggregationFunctionColumnPair functionColumnPair : starTreeMetadata.getFunctionColumnPairs()) {
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> deduplicatedAggregationSpecs =
Review Comment:
Can we do this at `StarTreeV2Metadata` loading time (in the constructor), essentially keeping the stored `AggregationFunctionType` within `AggregationFunctionColumnPair`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationSpec.java:
##########
@@ -50,4 +52,10 @@ public boolean equals(Object o) {
public int hashCode() {
return _compressionType.hashCode();
}
+
+ @Override
+ public String toString() {
+ return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE).append("compressionType", _compressionType)
+ .toString();
Review Comment:
(minor) Is this for debugging purpose? We usually use the IDE auto-generated one
```suggestion
return "AggregationSpec{" + "_compressionType=" + _compressionType + '}';
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on PR #12164:
URL: https://github.com/apache/pinot/pull/12164#issuecomment-1864402815
I've removed all the changes to the metadata and configuration mapping.
The original intent behind those were to mitigate against breaking changes to existing segment behaviour should the internal mapping between query aggregation and stored value aggregation evolve. Removing this additional complexity does simplify the implementation.
The current `DistinctCountHLLPlusValueAggregator` returns `DISTINCTCOUNTHLL` as its `AggregationFunctionType` (see failing unit test). I'm unsure if this is intentional - if it is, problems may arise from the changes in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1433186432
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationSpec.java:
##########
@@ -50,4 +52,10 @@ public boolean equals(Object o) {
public int hashCode() {
return _compressionType.hashCode();
}
+
+ @Override
+ public String toString() {
+ return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE).append("compressionType", _compressionType)
+ .toString();
Review Comment:
I see. Just realize we are already using the `ToStringBuilder` in `StarTreeV2BuilderConfig`, so we should be good here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1433138447
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java:
##########
@@ -31,6 +31,7 @@
public class DistinctCountHLLPlusValueAggregator implements ValueAggregator<Object, HyperLogLogPlus> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE = AggregationFunctionType.DISTINCTCOUNTHLL;
Review Comment:
Just to reiterate- I set it to the same value that it was set to previously and the failing test is a new test in this PR. Is it safe to change the value and will it have any unintended consequences on segments that have been created with the previous value type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1459484119
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/AvgValueAggregator.java:
##########
@@ -26,10 +26,11 @@
public class AvgValueAggregator implements ValueAggregator<Object, AvgPair> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE = AggregationFunctionType.AVG;
Review Comment:
Reverted in https://github.com/apache/pinot/pull/12164/commits/e11de58e72f2fb63f93a11eb0b5f6b19d7cae73d
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1434289093
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/StarTreeIndexReader.java:
##########
@@ -121,7 +124,9 @@ private void mapBufferEntries(int starTreeId,
_dataBuffer, ByteOrder.BIG_ENDIAN));
}
// Load metric (function-column pair) forward indexes
- for (AggregationFunctionColumnPair functionColumnPair : starTreeMetadata.getFunctionColumnPairs()) {
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> deduplicatedAggregationSpecs =
Review Comment:
I went back and forth about how best to do this as detailed in the issue and PR description. A consequence of de-duplicating in the `StarTreeV2Metadata` and the `StarTreeV2BuilderConfig` is that the metadata for a segment will only contain the stored aggregation function types, and not any superfluous aggregation function types. This has implications for whether a segment should be rebuilt when compared to the existing config (see `shouldModifyExistingStarTrees` in `StarTreeBuilderUtils`). If both the metadata and builder config reflect the stored aggregated types then this should behave correctly. Furthermore, a StarTree is checked for whether it is a fit for the given query aggregates and for this the underlying stored aggregate types must be checked as well.
Finally, it might be surprising to the end user if some of their StarTree aggregation configuration does not actually land up in metadata - I was considering adding a warning or validation to the TableConfig. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12164:
URL: https://github.com/apache/pinot/pull/12164#issuecomment-1858609632
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `41 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`aa834f4`)](https://app.codecov.io/gh/apache/pinot/commit/aa834f4ccce46326b339348efd60f1bd1df162fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.63% compared to head [(`611d3d3`)](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 34.82%.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...l/startree/v2/builder/StarTreeV2BuilderConfig.java](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL1N0YXJUcmVlVjJCdWlsZGVyQ29uZmlnLmphdmE=) | 0.00% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/local/startree/v2/store/StarTreeLoaderUtils.java](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9zdG9yZS9TdGFyVHJlZUxvYWRlclV0aWxzLmphdmE=) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...segment/spi/index/startree/StarTreeV2Metadata.java](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3N0YXJ0cmVlL1N0YXJUcmVlVjJNZXRhZGF0YS5qYXZh) | 12.50% | [6 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/segment/spi/index/startree/AggregationSpec.java](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3N0YXJ0cmVlL0FnZ3JlZ2F0aW9uU3BlYy5qYXZh) | 40.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...che/pinot/segment/spi/AggregationFunctionType.java](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL0FnZ3JlZ2F0aW9uRnVuY3Rpb25UeXBlLmphdmE=) | 77.77% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12164 +/- ##
=============================================
- Coverage 61.63% 34.82% -26.82%
+ Complexity 1152 952 -200
=============================================
Files 2407 2331 -76
Lines 130888 127193 -3695
Branches 20220 19671 -549
=============================================
- Hits 80670 44292 -36378
- Misses 44336 79755 +35419
+ Partials 5882 3146 -2736
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.57%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.82% <30.50%> (-26.69%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.80% <30.50%> (-26.81%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.79% <30.50%> (-26.68%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.82% <30.50%> (-26.82%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.51% <30.50%> (-15.12%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.51% <30.50%> (-0.16%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12164?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12164:
URL: https://github.com/apache/pinot/pull/12164#issuecomment-1863655307
IIUC, this PR has introduced the mapping from aggregation used in the query to aggregation used in star-tree index to store the pre-aggregated values.
IMO, we can make this mapping completely transparent to the users. When loading the star-tree, we can lookup this map and load star-tree with the stored type; when serving queries, we can lookup this map again and try to match the stored type with the loaded star-tree. We don't need to introduce these 2 types of aggregation to the config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1434289568
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/BaseSingleTreeBuilder.java:
##########
@@ -133,7 +133,9 @@ static class Record {
"Dimension: " + dimension + " does not have dictionary");
}
- TreeMap<AggregationFunctionColumnPair, AggregationSpec> aggregationSpecs = builderConfig.getAggregationSpecs();
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> aggregationSpecs =
+ StarTreeBuilderUtils.deduplicateAggregationSpecs(builderConfig.getAggregationSpecs());
Review Comment:
This comment is discussed [here](https://github.com/apache/pinot/pull/12164#discussion_r1434289093).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1439551672
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/StarTreeIndexReader.java:
##########
@@ -121,7 +124,9 @@ private void mapBufferEntries(int starTreeId,
_dataBuffer, ByteOrder.BIG_ENDIAN));
}
// Load metric (function-column pair) forward indexes
- for (AggregationFunctionColumnPair functionColumnPair : starTreeMetadata.getFunctionColumnPairs()) {
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> deduplicatedAggregationSpecs =
Review Comment:
I have validated that the configuration does not contain duplicates in this commit: https://github.com/apache/pinot/pull/12164/commits/a0f7efdc680892a386cb62d2f536c1e9ea4d81fb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on PR #12164:
URL: https://github.com/apache/pinot/pull/12164#issuecomment-1911956208
> I just pushed a commit with the following changes:
>
> * During query time, resolve the stored type before looking up the metadata so that no fallback is required
> * Handle backward-compatibility for old star-tree index where function might not be stored type
> * Add a test for querying non-stored type
>
> I did a commit instead of leaving comments because the code is spread in multiple classes and is kind of hard to explain. Let me know if the commit looks good.
Thank you @Jackie-Jiang, I appreciate you taking the time to contribute further to this PR. I have reviewed your commit and it all looks good.
I can understand why the changes you made might have been difficult to discuss over the PR. In particular, the areas affected within the query planner and underlying index reader were parts of the platform that I am less familiar with and it has been a good learning exercise to see your changes in this context!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1433140653
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationSpec.java:
##########
@@ -50,4 +52,10 @@ public boolean equals(Object o) {
public int hashCode() {
return _compressionType.hashCode();
}
+
+ @Override
+ public String toString() {
+ return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE).append("compressionType", _compressionType)
+ .toString();
Review Comment:
I added this for more clarity in the minion logs. When indexes are constructed, the logs were printing the function column pair with an opaque object identity for the aggregation spec. It's a nice to have and can remove if this is going to interfere with development.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1460685461
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/store/StarTreeLoaderUtils.java:
##########
@@ -100,7 +100,16 @@ public StarTreeV2Metadata getMetadata() {
@Override
public DataSource getDataSource(String columnName) {
- return dataSourceMap.get(columnName);
+ DataSource result = dataSourceMap.get(columnName);
+ // Some query columnNames could be supported by the underlying value aggregation type; do a secondary lookup
Review Comment:
I found a way to accomplish the above without having to make any changes to the StarTreeIndexReader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1433848301
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java:
##########
@@ -31,6 +31,7 @@
public class DistinctCountHLLPlusValueAggregator implements ValueAggregator<Object, HyperLogLogPlus> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE = AggregationFunctionType.DISTINCTCOUNTHLL;
Review Comment:
Fixed in https://github.com/apache/pinot/pull/12164/commits/722d89b3f88a7fe3ec0d97c94f88add2330f980e.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1434289093
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/StarTreeIndexReader.java:
##########
@@ -121,7 +124,9 @@ private void mapBufferEntries(int starTreeId,
_dataBuffer, ByteOrder.BIG_ENDIAN));
}
// Load metric (function-column pair) forward indexes
- for (AggregationFunctionColumnPair functionColumnPair : starTreeMetadata.getFunctionColumnPairs()) {
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> deduplicatedAggregationSpecs =
Review Comment:
I went back and forth about how best to do this as detailed in the issue and PR description. A consequence of de-duplicating in the `StarTreeV2Metadata` and the `StarTreeV2BuilderConfig` is that the metadata for a segment will only contain the stored aggregation function types, and not any superfluous aggregation function types. This has implications for whether a segment should be rebuilt when compared to the existing config (see `shouldModifyExistingStarTrees` in `StarTreeBuilderUtils`). If both the metadata and builder config reflect the stored aggregated types then this should behave correctly.
Finally, it might be surprising to the end user if some of their StarTree aggregation configuration does not actually land up in metadata - I was considering adding a warning or validation to the TableConfig. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1434553727
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/StarTreeIndexReader.java:
##########
@@ -121,7 +124,9 @@ private void mapBufferEntries(int starTreeId,
_dataBuffer, ByteOrder.BIG_ENDIAN));
}
// Load metric (function-column pair) forward indexes
- for (AggregationFunctionColumnPair functionColumnPair : starTreeMetadata.getFunctionColumnPairs()) {
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> deduplicatedAggregationSpecs =
Review Comment:
Within the in-memory loaded metadata (not the on disk copy which might already be generated) and the builder config, we can always keep the stored aggregates, and the query time lookup should also be converted to stored type first. This way it should work as expected.
I wouldn't expect user trying to match the segment metadata with table config, but +1 on logging warnings when the non-stored aggregates are provided within the table config
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on PR #12164:
URL: https://github.com/apache/pinot/pull/12164#issuecomment-1863423386
@Jackie-Jiang I've tested this on my local cluster and all basic checks appear to work. I ingested data, explained queries, dumped segment data with StarTree data, merged segments with merge job and finally changed table config and reloaded segments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "davecromberge (via GitHub)" <gi...@apache.org>.
davecromberge commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1439551672
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/StarTreeIndexReader.java:
##########
@@ -121,7 +124,9 @@ private void mapBufferEntries(int starTreeId,
_dataBuffer, ByteOrder.BIG_ENDIAN));
}
// Load metric (function-column pair) forward indexes
- for (AggregationFunctionColumnPair functionColumnPair : starTreeMetadata.getFunctionColumnPairs()) {
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> deduplicatedAggregationSpecs =
Review Comment:
I've not found a way to log warnings, but have validated that the configuration does not contain duplicates in this commit: https://github.com/apache/pinot/pull/12164/commits/ab6b03d58955b2b748ac801641b4f629d9b58152
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Shared aggregations in StarTree [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1456667781
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/AvgValueAggregator.java:
##########
@@ -26,10 +26,11 @@
public class AvgValueAggregator implements ValueAggregator<Object, AvgPair> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE = AggregationFunctionType.AVG;
Review Comment:
(minor) Seems no longer needed, consider reverting them (these types are actually self-explained, so maybe not worth defining as constant)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1041,6 +1042,13 @@ private static void validateIndexingConfig(IndexingConfig indexingConfig, @Nulla
throw new IllegalStateException("Invalid StarTreeIndex config: " + functionColumnPair + ". Must be"
+ "in the form <Aggregation function>__<Column name>");
}
+ AggregationFunctionColumnPair aggregatedType =
+ AggregationFunctionColumnPair.resolveToAggregatedType(columnPair);
+ if (aggregatedTypes.contains(aggregatedType)) {
+ LOGGER.warn("StarTreeIndex config duplication: {} already matches existing function column pair: {}. ",
+ columnPair, aggregatedType);
+ }
+ aggregatedTypes.add(aggregatedType);
Review Comment:
Can be simplified:
```suggestion
if (!aggregatedTypes.add(aggregatedType)) {
LOGGER.warn("StarTreeIndex config duplication: {} already matches existing function column pair: {}. ",
columnPair, aggregatedType);
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/store/StarTreeLoaderUtils.java:
##########
@@ -100,7 +100,16 @@ public StarTreeV2Metadata getMetadata() {
@Override
public DataSource getDataSource(String columnName) {
- return dataSourceMap.get(columnName);
+ DataSource result = dataSourceMap.get(columnName);
+ // Some query columnNames could be supported by the underlying value aggregation type; do a secondary lookup
Review Comment:
During query time, can we first resolve it instead of relying on the fallback? Ideally we should resolve it before looking up the metadata
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -497,4 +497,35 @@ public static AggregationFunctionType getAggregationFunctionType(String function
}
}
}
+
+ /**
+ * Returns the stored {@code AggregationFunctionType} used to create the underlying value in the segment or index.
+ * Some aggregation function types share the same underlying value but are used for different purposes in queries.
+ * @param aggregationType the aggregation type used in a query
+ * @return the underlying value aggregation type used in storage e.g. StarTree index
+ */
+ public static AggregationFunctionType getAggregatedFunctionType(AggregationFunctionType aggregationType) {
Review Comment:
The stored type only applies to star-tree, so consider moving it to `AggregationFunctionColumnPair` and renaming to `getStoredType()`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumnPair.java:
##########
@@ -66,6 +66,19 @@ public static AggregationFunctionColumnPair fromAggregationConfig(StarTreeAggreg
return fromFunctionAndColumnName(aggregationConfig.getAggregationFunction(), aggregationConfig.getColumnName());
}
+ /**
+ * Return a new {@code AggregationFunctionColumnPair} from an existing functionColumnPair where the new pair
+ * has the {@link AggregationFunctionType} set to the aggregated function type used in the segment or indexes.
+ * @param functionColumnPair the existing functionColumnPair
+ * @return the new functionColumnPair
+ */
+ public static AggregationFunctionColumnPair resolveToAggregatedType(
Review Comment:
Suggest renaming to `resolveToStoredType()` which IMO is more clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org