test(spark): tail-sweep coverage for low-coverage datasource classes - #19405
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19405 +/- ##
============================================
+ Coverage 75.56% 75.81% +0.24%
- Complexity 32651 32865 +214
============================================
Files 2574 2576 +2
Lines 142995 143558 +563
Branches 17530 17776 +246
============================================
+ Hits 108051 108835 +784
+ Misses 26908 26696 -212
+ Partials 8036 8027 -9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds behavior-pinning unit tests for three low-coverage Spark-datasource classes: the order-preserving transformation matcher in BaseHoodieCatalystExpressionUtils, AvroUtils/AvroSchemaHelper, and FileFormatUtilsForFileGroupReader.applyFiltersToPlan. I traced each assertion against the actual source implementations (including isCastPreservingOrdering, the OrderPreservingTransformation extractor cases, the translate/reduceLeft(And) filter lowering, and the Avro schema-matching/validation error messages) and every expected value lines up with real behavior. 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. One minor naming nit in TestAvroUtils; the other two files are clean.
cc @yihua
Add unit coverage for BaseHoodieCatalystExpressionUtils order-preserving transformation matching, org.apache.spark.sql.avro.AvroUtils schema matching and validation, and FileFormatUtilsForFileGroupReader filter lowering. Each assertion pins the exact translated output.
Mirror avroWithOptionalGhost so the optional-ghost scenario reads clearly at a glance.
4eb7fe2 to
6688e7b
Compare
There was a problem hiding this comment.
Did a Claude-assisted pass beyond the earlier bot review; everything below was verified against the checked-out branch before commenting.
Verified clean (no action needed):
- Diff is test-only: 3 new files, 361 insertions, 0 deletions; no accidental reverts.
- All 23 tests pass locally (
mvn -o test -pl hudi-spark-datasource/hudi-spark -Punit-tests -Dspark3.5 -Dtest='TestAvroUtils,TestFileFormatUtilsForFileGroupReader,TestCatalystExpressionOrderPreserving'), with no SparkSession required. - Cross-profile compile risk cleared: every Spark expression the tests construct was probe-compiled against 3.3.4/3.4.3/3.5.5/4.0.2/4.1.1 with zero errors;
AvroUtilsand the order-preserving matcher are single-sourced inhudi-spark-common(#19147/#19149), and nospark-avrojar is on the test classpath to shadow the vendored copy. - No duplicate coverage: none of the three classes had direct tests, and there is no overlap with #19164. Nearest neighbor
TestConvertFilterToCatalystExpressiontargets a different translator.
Inline comments, ranked: 2 correctness-tier coverage gaps (the order-breaking casts that isCastPreservingOrdering wrongly accepts, and the per-version ParseToDate/ParseToTimestamp hook -- the only version-divergent branch of the matcher), 3 assertion-strength fixes with suggestions, and a few optional nits.
| // Widening a numeric column preserves ordering, so the source attribute is recovered. | ||
| assertEquals(Some(intAttr), matched(Cast(intAttr, LongType))) | ||
| // Casting a numeric column to string can reorder values, so it must not match. | ||
| assertEquals(None, matched(Cast(intAttr, StringType))) |
There was a problem hiding this comment.
Both Cast cases pin the safe directions, but the discriminating one is missing: HoodieSparkTypeUtils.isCastPreservingOrdering only rejects String<->Numeric and returns true for everything else, including narrowing numeric casts. Adding
val longAttr = AttributeReference("l", LongType)()
assertEquals(None, matched(Cast(longAttr, IntegerType)))fails today: the matcher recovers the attribute even though non-ANSI narrowing wraps around, and DataSkippingUtils.translateIntoColumnStatsIndexFilterExpr then rewrites min/max through the cast. Concrete failure: bigint col a in a file with values {1, 2147483647, 4294967297} (min=1, max=4294967297); the filter cast(a as int) > 100 is translated to cast(a_maxValue as int) > 100, and cast(4294967297L as int) wraps to 1, so the file is pruned even though it holds a=2147483647 whose cast is 2147483647 -> silently missing rows. Same family: the Multiply/Divide arms match any literal operand, including negative ones that reverse ordering.
Since the missing assertion exposes a production bug, please file a GitHub issue on isCastPreservingOrdering (numeric-to-numeric should require Cast.canUpCast), and either fix it in this PR or pin current behavior here with a TODO referencing the issue so the hazard is documented rather than invisible.
There was a problem hiding this comment.
Pinned Cast(long -> int) as-is with a TODO pointing at #19445. Fixing isCastPreservingOrdering would pull DataSkippingUtils changes into a test-only PR, so the fix rides with the issue.
| } | ||
|
|
||
| @Test | ||
| def testDateTransformationsPreserveOrdering(): Unit = { |
There was a problem hiding this comment.
The file covers only shared match arms, not the one branch of this matcher that differs per Spark version: unapplyOrderPreservingDateParsing (BaseHoodieCatalystExpressionUtils.scala:107-110), whose overrides pattern-match ParseToDate/ParseToTimestamp with different arities in each version module (3.3: ParseToDate(child,_,_); 3.5/4.x: ParseToDate(child,_,_,_)), re-implemented in #19149. That is exactly the branch a per-profile unit test should pin.
Related data point worth a follow-up: both nodes are RuntimeReplaceable, and Spark's first optimizer batch (ReplaceExpressions) rewrites them before filters reach data skipping, so this branch may never fire on real queries; the existing to_timestamp coverage in TestDataSkippingUtils (line 677) only survives because that harness applies OptimizeIn alone.
Action: add one case built portably so it compiles on every profile, e.g.
assertEquals(Some(strAttr),
matched(sparkAdapter.getExpressionFromColumn(
functions.to_date(sparkAdapter.createColumnFromExpression(strAttr)))))(SparkAdapter.scala:376,384 provide both directions), and consider a follow-up issue to confirm whether to_date/to_timestamp data skipping still works post-ReplaceExpressions.
There was a problem hiding this comment.
Added a ParseToDate case. Went with the 1-arg auxiliary constructor instead of the adapter round-trip; javap shows it stable on 3.3.1 through 4.1.1 and it keeps functions out of the test. Reachability question filed as #19446.
- rename TestCatalystExpressionOrderPreserving to TestHoodieCatalystExpressionUtils - pin narrowing-cast behavior with a TODO referencing apache#19445 - cover the per-version ParseToDate hook via its stable 1-arg constructor - add GreaterThanOrEqual/LessThan/sources.And arms and an unknown-column failure pin - use three filters so the And fold direction is observable - reorder Avro fields so by-name lookup is discriminated from positional - cover ignoreNullable=true with a non-nullable extra Catalyst field - pin SQLConf case sensitivity and cover the case-sensitive resolver half - wrap must-not-throw calls in assertDoesNotThrow - mark dead vendored paths (supportsDataType, positional matching) as drift guards
5 participants
Describe the issue this Pull Request addresses
Part 2 of the Spark-datasource small-class coverage tail-sweep (sibling to #19164). Several small classes in the Spark datasource had low unit-test coverage, with branches reached only indirectly, if at all. This adds focused, behavior-pinning unit tests for the genuinely uncovered ones.
Summary and Changelog
Adds unit coverage for three low-coverage classes. Every assertion pins exact output, so a wrong result would fail the test.
org.apache.spark.sql.BaseHoodieCatalystExpressionUtils(0 missed lines, but many uncovered branches): newTestHoodieCatalystExpressionUtilsdrivestryMatchAttributeOrderingPreservingTransformationacross the wholeOrderPreservingTransformationmatch. It asserts the exact sourceAttributeReferencerecovered for identity, arithmetic on either operand, unary math, string case, date add/sub, date parsing (the per-Spark-versionParseToDatehook), and order-preserving up-cast, and that non-order-preserving shapes (numeric-to-string cast, attribute-free arithmetic, a non-whitelistedSqrt) do not match. The narrowing-cast case is pinned to current behavior with a TODO referencing Data skipping treats order-breaking casts as order-preserving (isCastPreservingOrdering) #19445.org.apache.spark.sql.avro.AvroUtils: newTestAvroUtilscoverssupportsDataType(atomic, struct, array, map, null supported;CalendarIntervaland its wrappers unsupported) and theAvroSchemaHelpermatching and validation paths previously exercised only through the Avro serializers: non-RECORD rejection, by-name vs positional field lookup, extra-Catalyst-field and extra-required-Avro-field validation (including theignoreNullableand nullable-Avro-field skips), and the ambiguous case-insensitive by-name match. Each error case pins the raisedIncompatibleSchemaExceptionmessage.org.apache.spark.sql.FileFormatUtilsForFileGroupReader: newTestFileFormatUtilsForFileGroupReadercoversapplyFiltersToPlan, pinning the Catalyst expression produced for each pushed-down data-sourceFilter(comparisons, null checks,In, string predicates,AlwaysTrue/AlwaysFalse, a nested and/or/not tree, and multi-filterAnd), theNoSuchElementExceptionraised for a filter on a column absent from the table schema, and that an empty filter list returns the input plan unchanged.applyNewFileFormatChangesin the same object (the fgReader plan-rewrite entry point) is intentionally out of scope here.Candidates verified and left out:
VectorDistanceUtilsis already exhaustively covered with exact-value assertions byTestHoodieVectorSearchFunction, so no new test was added.HiveSyncProcedureandHoodieNestedSchemaPruningneed heavier end-to-end scaffolding (hive metastore, optimizer plan fixtures) and are deferred to a separate pass. No code was copied.Impact
Test-only. No production code changes, no public API change, and no behavior change.
Risk Level
none
Documentation Update
none
Contributor's checklist