Repository navigation
feat: catalog compaction support for msq and base table spec - #20379
clintropolis wants to merge 2 commits into
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The current implementation should not be merged yet: an unsealed catalog base-table compaction can silently drop existing metric columns, and invalid catalog cluster-key types are allowed through coordinator validation and fail only after tasks are scheduled.
Reviewed 20 of 20 changed files.
Validation: git diff --check passed. No builds, tests, dependency installs, or formatters were run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| false, | ||
| false, | ||
| true, | ||
| false, |
There was a problem hiding this comment.
[P1] Preserve or reject undeclared metric columns
Finding: For an unsealed base-table compaction, the analyzer is configured with metric analysis disabled and only its dimension schema is appended to the base-table spec. If the datasource has existing segments from a legacy or rollup ingestion that contain metric aggregators, those metric columns are therefore omitted from the generated schema and silently disappear when compaction rewrites the segments, even though the unsealed path is intended to preserve undeclared columns.
Suggestion: Either extend the base-table schema path to preserve existing metric columns, or detect metric-bearing input segments and fail the compaction rather than rewriting them without those values.
| } else { | ||
| // Range partitioning is ascending only, DatasourceDefn.ClusterKeysDefn already rejects a descending cluster key, | ||
| // so a catalog table cannot declare one. | ||
| partitionsSpec = new DimensionRangePartitionsSpec( |
There was a problem hiding this comment.
[P2] Validate catalog cluster-key types before scheduling
Finding: Every catalog cluster key is converted to a range partition without checking the resolved catalog column type or whether it is multi-valued. For a plain catalog table with no baseTable, configuration validation has no dimensions spec to inspect, so numeric or array keys are accepted by the coordinator; the MSQ task then rejects the range partition at execution time and auto-compaction repeatedly submits failing tasks instead of rejecting the configuration up front.
Suggestion: Validate each catalog cluster key against the resolved column schema as a single-valued VARCHAR during compaction-config validation, and return a configuration error before any task is scheduled.
Description
As a follow-up to #19830, this PR improves the catalog based compaction to wire catalog stuff up like
baseTablespec that was missing from the compaction config.SEALEDcatalog tables (with abaseTablespec) will enforce the exact schema on any compacted segments, while non-sealed catalog schemas will always analyze the segments to compact to detect any additional columns to extend the catalog schema.SEALEDwill currently only remove columns on tables with abaseTablespec defined, but I plan to make it do this universally after #20377 and #20452, which would allow making an implicit plain tablebaseTablespec from column definitions forSEALEDtables (meaning a catalog schema for a rollup table or clustered table would always need an explicit base table projection spec defined). A newCompactionTaskpropertysealedhas been added to allow forcing a segment analysis even if abaseTablespec is defined, and so the spec for non-sealed tables can be extended with any existing columns present in the segment.This PR also fixed up catalog compaction to work properly with MSQ based compaction since the partitioning defined in the catalog wasn't hooked up either.
Lastly, documentation has been added for catalog compaction. It will need fixed up after #20377 and its follow-up and the modification to make an implicit non-rollup table when baseTable isn't defined, but that shouldn't be too much work, just need to adjust the section which discussed sealed.