Skip to content

Logical tables / Table Config validation refactoring#17761

Open
krishan1390 wants to merge 24 commits intoapache:masterfrom
krishan1390:logical_tables_refactoring
Open

Logical tables / Table Config validation refactoring#17761
krishan1390 wants to merge 24 commits intoapache:masterfrom
krishan1390:logical_tables_refactoring

Conversation

@krishan1390
Copy link
Contributor

Refactoring of logical table / table config validations.

  1. Hygiene -
  • Remove physicalTableExistsPredicate, brokerTenantExistsPredicate, propertyStore from logical table config utils, as they are dependent on the resource manager rather than config utility
  • Remove setting broker tenant in validateLogicalTableConfig
  1. Improve validateTableConfig in table config validation utils
  2. Move checks of logical table / physical table with same name present to validateNewLogicalTableConfig
  3. Add a Schema.cloneSchemaWithName method

krishan1390 and others added 23 commits February 11, 2026 13:15
Move fromZNRecord/toZNRecord methods from TableConfigSerDeUtils and
LogicalTableConfigUtils into the config classes themselves. This makes
it easier to extend these classes — subclasses can override
serialization/deserialization behavior directly.

- Add helix-core dependency to pinot-spi for ZNRecord access
- Add fromZNRecord()/toZNRecord() to TableConfig and LogicalTableConfig
- Delete TableConfigSerDeUtils (all callers updated)
- Remove fromZNRecord/toZNRecord from LogicalTableConfigUtils (validation stays)
- Update all callers across 15 files to use the new methods directly

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move fromZNRecord/toZNRecord methods from TableConfigSerDeUtils and
LogicalTableConfigUtils into the config classes themselves. This makes
it easier to extend these classes — subclasses can override
serialization/deserialization behavior directly.

- Add helix-core dependency to pinot-spi for ZNRecord access
- Add fromZNRecord()/toZNRecord() to TableConfig and LogicalTableConfig
- Delete TableConfigSerDeUtils (all callers updated)
- Remove fromZNRecord/toZNRecord from LogicalTableConfigUtils (validation stays)
- Update all callers across 15 files to use the new methods directly
- Add LogicalTableConfigSerDeTest for ZNRecord round-trip coverage

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace ZNRecord-based SerDe on TableConfig and LogicalTableConfig with
a helix-agnostic ConfigRecord POJO in pinot-spi. Bridge utilities in
pinot-common (TableConfigSerDeUtils, LogicalTableConfigUtils) handle
ZNRecord conversion, keeping helix-core out of the SPI layer.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2026

Codecov Report

❌ Patch coverage is 70.00000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.20%. Comparing base (c52aa38) to head (5045d06).

Files with missing lines Patch % Lines
...ntroller/helix/core/PinotHelixResourceManager.java 81.25% 2 Missing and 4 partials ⚠️
...rc/main/java/org/apache/pinot/spi/data/Schema.java 0.00% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #17761      +/-   ##
============================================
- Coverage     63.21%   63.20%   -0.01%     
  Complexity     1454     1454              
============================================
  Files          3179     3179              
  Lines        191069   191089      +20     
  Branches      29220    29223       +3     
============================================
- Hits         120786   120785       -1     
- Misses        60860    60885      +25     
+ Partials       9423     9419       -4     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-11 63.17% <70.00%> (+<0.01%) ⬆️
java-21 63.15% <70.00%> (-0.02%) ⬇️
temurin 63.20% <70.00%> (-0.01%) ⬇️
unittests 63.20% <70.00%> (-0.01%) ⬇️
unittests1 55.58% <0.00%> (-0.03%) ⬇️
unittests2 34.10% <70.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

PhysicalTableConfig physicalTableConfig = entry.getValue();
String physicalTableName = entry.getKey();
// Skip existence validation for multi-cluster physical tables
if (!physicalTableConfig.isMultiCluster()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We can also add a INFO log for "Physical table {} is multi cluster, skipping existence check" for more info.

return cloned;
}

public static Schema cloneSchemaWithName(Schema source, String newName) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good addition! Necessary for reducing the overhead while creating logical tables. But I do not see this being used anywhere currently, is this for a future addition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants