test(metadata): cover getColumnsToIndex when the table schema is absent - #19859
test(metadata): cover getColumnsToIndex when the table schema is absent#19859rangareddy wants to merge 1 commit into
Conversation
Closes apache#17410. HUDI-9194 asked whether getColumnsToIndexWithoutRequiredMetaFields needs a test case. Its branches are almost all covered already through the public getColumnsToIndex wrapper: the explicit column list, meta columns inside that list, unsupported types, nested fields, the max-columns cap, and meta fields disabled. One branch was not reachable from any existing case, because every one of them supplies a schema: the path taken when the table schema is absent. Added a case for it, pinning three outcomes that differ in ways worth fixing in place: - No explicit column list: the inner call returns nothing, so the caller is left with just the always-indexed meta columns. Col stats initialises rather than failing. - An explicit column list: it throws IllegalArgumentException naming the missing schema, since the configured names cannot be resolved to field schemas without one, and indexing nothing would look like the config had been honoured. - The same list while the table is initialising: the names are recorded without schemas, so col stats can be enabled before the first commit has produced a schema. Test-only; no production code touched.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This is a test-only PR adding coverage for the schema-absent branch of HoodieTableMetadataUtil#getColumnsToIndexWithoutRequiredMetaFields — the no-column-list path (empty map, meta cols only), the explicit-list path (IllegalArgumentException), and the table-initializing path (names recorded without schemas). I traced each assertion against the production code and the expectations match the actual branch behavior; metaClient is re-initialized per test so the table config mutations in other tests don't leak in. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Mostly clean; one nit on trimming the overly verbose javadoc comment.
cc @yihua
| Lazy.eagerly(Option.of(schema)), true, V1).keySet())); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🤖 nit: this javadoc is quite dense/wordy for a test comment — might be worth trimming to a couple of short sentences stating what's being verified, rather than walking through the full reasoning.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19859 +/- ##
============================================
- Coverage 78.35% 78.34% -0.01%
+ Complexity 33953 33945 -8
============================================
Files 2543 2543
Lines 141980 141980
Branches 17220 17220
============================================
- Hits 111245 111234 -11
- Misses 23039 23047 +8
- Partials 7696 7699 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4 participants
Describe the issue this Pull Request addresses
Closes #17410 (https://issues.apache.org/jira/browse/HUDI-9194).
The ticket asks whether
HoodieTableMetadataUtil#getColumnsToIndexWithoutRequiredMetaFieldsneeds a test case. ReadingTestHoodieTableMetadataUtil#testGetColumnsToIndex, most of it is already covered through the publicgetColumnsToIndexwrapper: the explicit column list, meta columns appearing inside that list, unsupported column types, nested fields, the max-columns cap, and meta fields disabled.One branch is not reachable from any existing case. Every case there supplies a table schema, so the path taken when the schema is absent is never exercised:
and, on the explicit-list side of the same method, the guard that fires when there is no schema to resolve names against:
Summary and Changelog
TestHoodieTableMetadataUtil: newtestGetColumnsToIndexWhenTableSchemaIsAbsent, covering the schema-absent branch in the three ways it behaves:IllegalArgumentExceptionnaming the missing schema. The configured names cannot be resolved to field schemas without one, and quietly indexing nothing would look like the config had been honoured.No code was copied.
Impact
Test-only. No production code is touched, and no existing test is modified.
Worth noting for a reader: writing this corrected two assumptions of mine that the run disproved. The schema-absent, no-list case does not return an empty map to the caller, because the wrapper still adds
META_COLS_TO_ALWAYS_INDEX; and the initialising case returns five entries, not two, for the same reason. The test asserts what the code does.Risk Level
none
Verified on Spark 3.5 / Scala 2.12:
mvn test -pl hudi-hadoop-common -Dtest=TestHoodieTableMetadataUtil->Tests run: 19, Failures: 0, Errors: 0mvn test-compile checkstyle:check -pl hudi-hadoop-common-> cleanDocumentation Update
none
Contributor's checklist