fix(spark): widen procedure filter numeric comparisons - #19836
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19836 +/- ##
============================================
- Coverage 78.35% 78.34% -0.01%
+ Complexity 33953 33946 -7
============================================
Files 2543 2543
Lines 141980 142052 +72
Branches 17220 17247 +27
============================================
+ Hits 111245 111288 +43
- Misses 23039 23053 +14
- Partials 7696 7711 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
The direction is right: I confirmed all three cases from #19632 are fixed (data_file_size > 2000 over a 3e9 value, reversed operands, mixed numeric pairs), on Spark 3.5.5 and 4.1.1 with ANSI on and off. TypeCoercion.findWiderTypeForTwo is present in catalyst 3.3.4 / 3.4.3 / 3.5.5 / 4.0.2 / 4.1.1, so no Spark profile loses its compile.
One item is worth a look before merge: moving the type check from node shapes to left.dataType means an unresolved operand now throws during the transform instead of at eval, which defeats Or short-circuiting and turns some non-empty results into empty ones. Details inline.
The rest are smaller: In and <=> still carry the bug the comparison operators just lost, a decimal corner where the widened type clamps at precision 38, and some coverage and comment cleanup.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR replaces the narrow Long-column-vs-Int-literal coercion in applyTypeCoercion with Spark's TypeCoercion.findWiderTypeForTwo, so mixed numeric procedure-filter comparisons are widened symmetrically and large Long values are preserved. I traced the per-row transform/eval path and the widening logic; the substantive edge cases (accessing dataType on unresolved operands, ANSI vs non-ANSI TypeCoercion differences, DecimalType.bounded precision clamping, and the test-coverage gaps) have already been raised in the existing inline discussion, and I don't have a new, non-duplicative correctness concern to add from this automated pass. Please take a look at the existing inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
|
I'll work to land this later in the day. Will take over unless OP decides to address the comments above. |
5c62235 to
5616349
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The change replaces the narrow Long-column/Int-literal special case in applyTypeCoercion with Spark's TypeCoercion.findWiderTypeForTwo, so mixed numeric comparisons widen symmetrically instead of narrowing the column. Most of the earlier round's points (unresolved-node dataType access, ANSI/AnsiTypeCoercion divergence, decimal precision clamping, operator coverage) still look like the main things to settle; I added one more on operator coverage. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
Addresses review feedback on the numeric coercion rewrite. - Skip coercion when either operand is unresolved. dataType throws there, which defeated Or short-circuiting and, once apache#19850 started calling the transform from validateFilterExpression, replaced the "Invalid column references" and "Unsupported functions" messages with "Invalid call to dataType on unresolved object". - Extend the transform to In and EqualNullSafe, which still carried the apache#19632 silent drop that the binary comparisons just lost. - Pick AnsiTypeCoercion or TypeCoercion from the session's ANSI mode. They disagree on BIGINT with FLOAT: DOUBLE under ANSI, FLOAT under numericPrecedence without it. - Leave all-decimal comparisons alone. DecimalType.bounded clamps precision at 38, so widening can overflow the integral side, and decimal ordering is already precision- and scale-independent. - Return the original node when no operand needs a cast, and bind and resolve once per batch rather than once per row. Tests run both halves of apache#19632 over one fixture, add a second row so the numeric-pair assertions can fail, and cover In, EqualNullSafe, reversed operands, and the decimal and ANSI paths. TestFsViewProcedure gains a show_fsview_all filter on data_file_size, the column named in the report.
Widening picks a wider type but not always a lossless one, and neither case surfaces an error: an integral widened to float rounds, and a decimal operand whose scale leaves too few integral digits overflows the cast into a dropped row. Both are latent or benign today; record the shapes so a future reader does not rediscover them.
github-actions
Bot
added
size:M
Same two cases, fewer words: rounding on a float conversion and overflow on a decimal with too little room before the point.
Fold the caveat back into the scaladoc and wrap it at the file's ~95-column prose width. It was a short-wrapped block of // lines sitting between the scaladoc and the method, so it broke mid-clause and did not render as documentation.
Second review round on the procedure filter coercion. - Drop the all-decimal skip. Leaving decimal operands uncoerced keeps the comparison unresolved, and validateFilterExpression rejects the filter before any procedure evaluates it, so "dec > 1.00" threw where the first commit accepted it. The skip also did not prevent the clamp it cited: a BIGINT against a DECIMAL(31,30) still widens to DECIMAL(38,30). - Widen a NULL operand along with the numeric ones, matching the plan Spark builds for "ts IN (1000, null)" and "ts <=> null". - Extend the transform to arithmetic and coalesce, which resolve by exact type equality and so rejected "ts + 1 > 1500". - Correct the unresolved-operand comment. It claimed such expressions are rejected up front by validateFilterExpression, which calls this code, so nothing has been rejected yet. - Note why the coercion rules are read from SQLConf.get: Cast takes its eval mode from the same thread-local, and the two-argument Cast is the only form portable across 3.3 to 4.x. Tests add the validate half for every shape whose validation result this PR flips, since procedures validate before they filter. Also covers null operands, mixed-width IN lists, column-against-column widening under both ANSI modes, arithmetic, coalesce, quoted column names, and Or short-circuiting. The TestFsViewProcedure filters now use shapes master rejects plus one that keeps no rows, so an ignored filter cannot pass.
|
Pushed I had asked for an all-decimal skip in TLDR: Also in this commit:
Spark 3.5.5 / 2.12 is green, 29 tests, scalastyle clean. The 4.1 / 2.13 suites need a re-run: they caught a |
The per-row Try swallowed every failure into a dropped row. Once the widening started casting operands, that hid a case the equivalent query reports: under ANSI, an overflowing cast or arithmetic raises SparkArithmeticException, and Hudi turned it into a silently missing row. 10^30 > 1.5 is true, and the filter excluded the row without telling the caller. Let ArithmeticException out of both evaluateExpressionOnRow and evaluateFilter, so Spark's own Cast semantics decide: null and a dropped row without ANSI, a raised error with it. evaluateFilter rethrows it unwrapped rather than restating it as a filter expression problem, since the expression is fine and the data is what does not fit. Other failures keep the existing handling. Tests pin both ANSI modes for an overflowing Long addition, which behaves the same on every supported Spark, and for the decimal widening, which does not: Spark 3 widens DECIMAL(38,0) towards DECIMAL(38,18) and overflows the clamp, while Spark 4 declines to widen a lossy decimal pair in either ANSI mode and compares on Decimal ordering instead.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up! This round hoists bind/resolve out of the per-row loop, routes all widening through a shared widenNumericOperands helper, and extends it to EqualNullSafe, In, BinaryArithmetic and Coalesce, with the ANSI/non-ANSI coercion object picked from SQLConf.get.
Checking the prior round: my EqualNullSafe question is addressed (it now goes through applyTypeCoercion, with ts <=> 1000 and ts <=> null pinned). @voonhous's points are all addressed too — the unresolved-operand guard (operands.exists(!_.resolved)) keeps dataType off unresolved nodes and preserves Or short-circuiting; ANSI mode selects AnsiTypeCoercion; In is wired via findWiderCommonType over value +: list; the no-op rebuild is avoided by the distinct.length == 1 bail-out; and the tests now use a multi-row fixture plus reversed-operand, negative and procedure-level (TestFsViewProcedure) coverage. The decimal-38 clamp is no longer bailed out but is documented on widenNumericOperands, and the resulting behavior matches what the analyzer would do for the same SQL comparison, so that seems a reasonable resolution.
One optional question on arithmetic coercion inline. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
github-actions
Bot
added
size:L
Divide accepts only Double or Decimal, so widening its operands to their common numeric type leaves an integral pair unresolved: "ts / 2 > 500" was rejected as an unsupported filter expression while "price / 2 > 5" worked, because price is already Double. This predates the arithmetic widening -- Divide(Long, Int) was unresolved for differing types before it too. Mirror the analyzer's Division rule: promote an integral pair to Double, and let a pair that already involves a decimal widen the way the other arithmetic does. Divide has to be matched ahead of the general BinaryArithmetic case, and the rebuild goes through withNewChildren so the constructor difference between 3.3 and 3.4+ stays out of it. Also correct two test comments. The overflowing Long addition wraps to a negative without ANSI, it does not yield null; that is a Cast behaviour, not an addition one. And the decimal overflow test now pins only Spark 3, where the widening and the overflow were both observed, instead of asserting a Spark 4 result whose mechanism was inferred rather than measured.
Spark's Division rule guards on isNumericOrNull, "in case a query contains null literals", so "ts / null > 0" and "null / ts > 0" resolve there and evaluate to null. The Divide helper guarded on NumericType alone, so both stayed unresolved and validation rejected them, while widenNumericOperands already admitted NullType. Share one isNumericOrNull between the two so the guards cannot drift apart again. The rule is unchanged between the majors this builds against, only relocated from TypeCoercion.scala to DivisionTypeCoercion.scala; link both from the helper. Link Spark's decimal precision rules from findWiderNumericType the same way, and point the unsettled decimal parity question at HUDI 19860.
BinaryArithmetic.checkInputDataTypes accepts two decimals of different precision and scale, and Spark derives the result precision from the operands, so widening them to a common type does not enable the expression, it changes the answer. DECIMAL(38,18) * DECIMAL(2,1) gives a scale-16 product that still holds 0.0000001; casting both to DECIMAL(38,18) first drives the product to scale 6 and rounds the value to zero, so "dec * 1.0 > 0.0" dropped a row Spark keeps. Leave any arithmetic with a decimal operand untouched. A decimal against a non-decimal therefore stays unresolved and rejected, which is what it was before this coercion existed, rather than resolving to a wrong answer. Matching Spark there needs its DecimalPrecision promotion, including the minimum-precision rule for integral literals that exists to avoid this same loss; that belongs with HUDI 19860. Also fix a resolved check: the decimal guard reads dataType, which throws on an unresolved operand.
validateFilterExpression never checked that the expression is
boolean, so a resolvable "ts + 1" now passed validation and
reported zero rows where Spark raises FILTER_NOT_BOOLEAN. Reject
any non-boolean result, string included, as Spark does.
The ANSI rethrow covered only ArithmeticException. An ANSI cast of
a malformed string raises SparkNumberFormatException or
SparkDateTimeException, which the per-row Try still swallowed into
a dropped row, so "int(name) > 1" returned nothing where the query
fails. Rethrow all three JDK types.
IntegralDivide accepts only Long or Decimal and its operands are
never widened against each other, so "id div 2" stayed unresolved
while "ts div 2" resolved. Mirror the analyzer's IntegralDivision
rule and promote each narrower integral operand to Long first.
A NullType operand now takes the type of a single non-numeric peer
too, so "name IN ('a1', null)" and "name <=> null" validate the
way Spark plans them. The helper is renamed widenOperands.
Drop the inner Try around bindAndResolveExpression: nothing
reaches its failure branch, and the outer IllegalArgumentException
is the better failure mode if something ever does.
Tests: pin the Spark 4 outcome of the decimal-overflow case (the
fractional operand is cast down, the row survives in both ANSI
modes) instead of asserting nothing there; cover the constant
folded by DecimalPrecision for a literal past the Long range, the
%, div and - operators and wider coalesce shapes, and a byte
literal (130) that a narrowing implementation would flip; add a
filter assertion to the show_metadata_column_stats_overlap test;
fold two duplicate decimal tests into their parity siblings; run
the retainFractionDigitsOnTruncate axis only where the key exists;
widen the TestFsViewProcedure empty control; rename scalarRow to
tsRow and alias java.math.BigDecimal.
Five tests imported scala.collection.JavaConverters._ locally. One file-level import in the scala group replaces them, matching the header layout of HoodieProcedureFilterUtils.
|
Validation now rejects a non-boolean filter the way Spark does ( Tests pin the Spark 4 decimal-overflow outcome instead of skipping it, cover Verified on 3.3.4/3.5.5/4.1.1; the suite runs 26 tests after folding two duplicates. |
5 participants
Describe the issue this Pull Request addresses
Procedure filters narrowed Long columns to Int and left other mixed numeric expressions unresolved, so large Long values matched wrongly and reversed or mixed-type comparisons failed validation.
Fixes #19632. Related decimal parity work: #19860.
Summary and Changelog
TypeCoercionorAnsiTypeCoercion, chosen bySQLConf.get.ansiEnabled, instead of narrowing the column.DecimalPrecisionrules before the generic widening, for comparisons and for arithmetic with decimal operands, so literal precision and decimal result scale match Spark.IN,<=>,coalesceand arithmetic operands; non-decimal/promotes to Double,divpromotes narrow integrals to BIGINT, and a null operand takes its peer's type.Before and after, by user-visible case (compares master with
a6cc9da55868)Examples use procedure output columns:
tsis BIGINT,priceis DOUBLE, anddecis DECIMAL. "Rejected" means the procedure fails filter validation before returning results. Matching behavior has been checked on Spark 3.5.5 and 4.1.1 for the covered cases and settings.ts > 2000, withts = 3000000000ts < 2000, withts = 30000000001500 < ts,price > 15.0,dec > 1.00withdec DECIMAL(10,2)ts IN (1000, null),ts <=> 1000,ts <=> nullts + 1 > 1500,ts / 2 > 500,coalesce(ts, 0) > 1500/uses Doubledec + 1 > 0,dec / null > 03000000000greater than a decimal literal of10^-30;DECIMAL(38,30)value10^-29 > 0ts + 1L > 0L, withts = Long.MaxValueint(name) > 1ts + 1,namets + 1rejected as unresolved;nameaccepted and returns no rowsFILTER_NOT_BOOLEANfor bothid div 2 > 0divaccepts only BIGINT or DECIMAL and same-typed INT operands were never promotedIntegralDivisionrule doesname IN ('a1', null),name <=> nullRelease consideration: applications that previously received an empty or incomplete result may now receive matching rows, or an arithmetic or cast error when ANSI mode is enabled. A filter whose result is not boolean, such as a bare non-boolean column, is now rejected at validation instead of returning no rows. Null results still do not match a filter. Filtering does not change the output schema or rewrite returned column values. Decimal and floating-point conversions still follow Spark's rounding and overflow rules; full SQL parity is not claimed.
Impact
Previously rejected numeric filters are accepted and wide integral values are preserved. A non-boolean filter, and under ANSI an arithmetic or cast error, now fails the procedure instead of returning no rows. No public API change; procedure filters still support a subset of Spark SQL expressions.
Remaining limits
Numeric conversion is not always lossless. Floating-point conversion can round values. Decimal conversion can round or overflow according to the active Spark rules and settings. ANSI arithmetic and cast errors now reach the caller; non-ANSI decimal cast overflow produces null and excludes the row. Non-ANSI integral arithmetic can wrap rather than return null.
The previously reported high-precision mixed comparison mismatches are fixed: a large Long compared with a tiny decimal literal on Spark 3.5.5, and a tiny
DECIMAL(38,30)compared with integer zero on Spark 4.1.1. The extreme decimal/decimal example in #19860 has also been checked against SQL; it should not be described as a confirmed remaining mismatch.Filters still run after
limitin theshow_*procedures, so a filter only sees the firstlimitrows; tracked in #19862.Risk Level
Moderate. Changes affect numeric filter acceptance, precision, and error propagation across Spark versions. Tests compare results with Spark SQL; complete SQL parity and the full version matrix are not claimed.
Documentation Update
Coercion behavior and known verification limits are documented here and in the related issues.
Contributor's checklist
Validation at
a6cc9da55868df61fa197caait also compiled against 3.4.3, 4.0.2 and 4.2.0.TestHoodieProcedureFilterUtils26/26 with the module's test base,TestFsViewProcedure6/6, and theshow_metadata_column_stats_overlaptest inTestMetadataProcedure, all through a standalone ScalaTest runner on the module classpath.TestHoodieProcedureFilterUtils26/26 with a stubbed test base that carries the realwithSQLConf.df61fa197caa, every assertion of the two SQL-parity tests was replayed againstdf.filteron all six Spark versions with no divergence, and the widened expression trees were compared with the analyzer's output on 3.3.4, 3.5.5 and 4.1.1 across about 120 filters in both ANSI modes. Targeted checks of extreme decimal/decimal comparisons, including rounding-sensitive equality and ANSI overflow, matched SQL.git diff --checkpass.