Allow timestamp-index derived columns in star-tree dimensionsSplitOrder validation - #18774
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18774 +/- ##
============================================
+ Coverage 66.94% 66.96% +0.02%
Complexity 1423 1423
============================================
Files 3453 3453
Lines 218858 218876 +18
Branches 34787 34792 +5
============================================
+ Hits 146512 146572 +60
+ Misses 60621 60595 -26
+ Partials 11725 11709 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Walked through the three guards in |
Jackie-Jiang
added
enhancement
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
The change looks good. Consider adding an end-to-end queries test with star-tree index enabled on a timestamp index column, and ensure the query can use star-tree index properly.
sure |
|
@himanish-star Follow up on this. Do you get a chance to work on the test? It should be similar to |
|
@Jackie-Jiang working on it now, will update shortly |
…er validation Timestamp-index derived columns ($col$GRANULARITY) declared via fieldConfig timestampConfig granularities are materialized as dictionary-encoded single-value TIMESTAMP columns at segment generation time, so they are absent from the schema during table config validation. TableConfigUtils #validateStarTreeIndexConfigs rejected them with 'Failed to find dimension column ... in schema', forcing users to bypass validation via the raw REST API. Accept declared timestamp-index columns in dimensionsSplitOrder without a schema lookup, mirroring how they are handled at segment generation time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds TimestampIndexStarTreeV2Test, which runs the full BaseStarTreeV2Test
query matrix against a star-tree whose dimensionsSplitOrder contains a
TIMESTAMP-index derived column ($tsCol$MILLISECOND). The derived column is
declared only through fieldConfig timestampConfig granularities and is
materialized by TimestampIndexUtils#applyTimestampIndex at segment generation
time, so this covers the runtime half of the validation fix: the tree really
is built over the derived column, and every filter and group-by against it
yields the same aggregate through the star-tree as through a plain scan.
MILLISECOND granularity is used because dateTrunc('MILLISECOND', tsCol) is the
identity, so the derived column carries the same [0, DIMENSION_CARDINALITY)
value domain as the regular dimensions and the inherited predicate literals
apply to it unchanged. A dictionary-length assertion pins that the transform
actually ran, so the query cases cannot pass vacuously against an all-defaults
column.
BaseStarTreeV2Test gains hooks for schema fields, record values and the
dimensions split order, exposes the aggregation string and star-tree, and
widens testQuery to protected. Existing subclasses are unaffected.
Also fixes TableConfigUtilsTest to use the DataType import directly, which
master switched to after this branch was first written.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eae9d21 to
155e09c
Compare
… column testQueriesOnTimestampIndexColumn proves the star-tree returns correct results when it is traversed, but it builds StarTreeFilterPlanNode directly and never exercises StarTreeUtils#isFitForStarTree, the gate AggregationPlanNode and GroupByPlanNode use to pick a tree. A derived column could therefore be correct-when-used yet never selected. Assert the gate accepts a query with the derived column in both the filter and the group-by, and rejects the same query when the predicate column is the raw TIMESTAMP column, which is in the schema but not a star-tree dimension. The negative case keeps the positive one from passing vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
added @Jackie-Jiang |
Documents support added by apache/pinot#18774 for using timestamp-index derived columns in `starTreeIndexConfigs[].dimensionsSplitOrder`. The update explains the derived-column naming convention, requires the matching timestamp granularity to be declared, and clarifies that Pinot materializes these columns during segment generation. Validation: `git diff --check`. Co-authored-by: Xiang Fu <xiangfu@Xiang-mac-mtv-2.local>
|
Documentation follow-up: pinot-contrib/pinot-docs#982 (merged). |
Problem
A table config that references timestamp-index derived columns (
\$<column>\$<GRANULARITY>, e.g.\$OrderDate\$DAY) instarTreeIndexConfigs[].dimensionsSplitOrderis rejected byTableConfigUtilsvalidation with:These derived columns are declared via
fieldConfigList[].timestampConfig.granularitiesand are materialized as dictionary-encoded, single-valueTIMESTAMPcolumns at segment generation time (TimestampIndexUtils#applyTimestampIndex). They are therefore absent from the schema during config validation, even though the configuration is valid and works at runtime. Today the only workaround is to push the config through the REST API with validation skipped.Fix
validateStarTreeIndexConfigsnow receives the set of declared timestamp-index columns (TimestampIndexUtils#extractColumnsWithGranularity) and accepts them indimensionsSplitOrderwithout a schema lookup — matching how they are handled at segment generation time. Columns whose granularity is not declared intimestampConfigare still rejected.Testing
Added
TableConfigUtilsTest#testValidateStarTreeIndexWithTimestampIndexDerivedColumns:\$OrderDate\$DAY/WEEK/MONTHwith matchingtimestampConfiggranularities now validates.\$OrderDate\$HOUR) still fails validation.checkstyle:checkandlicense:checkpass onpinot-segment-local.