fix: [branch-1.0] backport four bug fixes for 1.0.1 - #5321
Open
andygrove wants to merge 4 commits into
Open
Conversation
…#5162) * fix: skip null slots when checking overflow in unary negation (apache#5093) * review: collapse ANSI negate dispatch, pin interval null regression Addresses review feedback on apache#5162: - Collapse the five near-identical array arms into a from_type lookup and rename map_neg_error's parameter to match the SparkError field. - Pin the interval half of the regression: a MIN sentinel in a *null* slot for Interval(YearMonth) and the whole Interval(DayTime)::MIN value both raised a spurious ANSI overflow, because the pre-scan walked value(i) without consulting the null buffer. - Reposition the single-component DayTime cases as error-mapping coverage: they already failed before this PR (neg_wrapping delegates to neg for every non-integer type), just with a raw ArrowError instead of a Spark error, so the failure set is unchanged. - Use "byte" / "short" as from_type for Int8/Int16 on both the array and scalar paths. The scalar arms previously passed a bare " caused", rendering as "[ARITHMETIC_OVERFLOW] caused overflow.". No string matches every Spark version here -- 3.4/3.5 throw _LEGACY_ERROR_TEMP_2043 ("- <sqlValue> caused overflow.") while 4.x uses MathUtils.negateExact ("byte overflow") -- and since the shims always render ARITHMETIC_OVERFLOW, the 3.x class is out of reach regardless. "byte"/"short" is exact on 4.x and needs no hardcoded value. - Turn the byte/short dtype placeholders in CometExpressionSuite's unary negative overflow test into version-specific Spark and Comet byte/short overflow assertions. - Drop the stale supported-datatypes comment and record why the fallthrough is still overflow-safe. - Deduplicate the per-width test bodies behind generic helpers and give the let-else arms diagnostic panic messages. --------- Co-authored-by: Andy Grove <agrove@apache.org> (cherry picked from commit 5783793)
…pache#5167) (cherry picked from commit ef60197)
…Exec (apache#5138) * fix: normalize nested field nullability in ShuffleScanExec and ExpandExec Arrow treats nested field nullability as part of `DataType` identity, so `RecordBatch::try_new_with_options` rejects a column whose nested `nullable` flags are narrower than the declared type even though the data is fine — a non-null child is a strict subset of a nullable one. Most Comet boundaries already normalize instead of asserting (`ScanExec` casts at the FFI boundary, `SchemaAlignExec` casts the shuffle writer's input, `CometLocalTableScanExec` widens with `.asNullable`), which is why this drift is usually invisible. Two stamp points still asserted and aborted the task: - `ShuffleScanExec` stamped the catalyst schema onto the decoded shuffle block with no cast. This is the direct-read shuffle path, enabled by default. - `ExpandExec` derived its schema from `projections[0]` only and stamped it onto the output of every projection, so it failed if any two projections disagreed on a nested nullable flag for the same output column. Add `cast_and_stamp_schema` to `datafusion-comet-common` and use it at both sites, so the drift is absorbed the way the other boundaries absorb it. Also derive `ExpandExec`'s schema by widening nested nullability across all projections rather than reading it off the first, so the declared schema is valid for every projection. When a stamp genuinely cannot be reconciled, the error now names the operator and the dotted path of the offending column instead of only `at column index N`, which was very hard to act on for wide schemas of deeply nested structs. Upstream nested-nullability drift itself is tracked by apache#4515. Closes apache#5137 * refactor: address /simplify review of the nested-nullability fix Reuse: - Route `SchemaAlignExec::align` through the new shared `cast_and_stamp_schema` instead of keeping its own copy of the same cast-then-stamp loop. This deletes the `ColumnAction` enum and its plan-time precomputation and plumbing; the per-batch type comparison it avoided is negligible against batch-sized work, and the operator gets the improved error messages for free. - Share one nested-nullability test fixture between the `expand` and `shuffle_scan` tests rather than defining the same list-of-struct builder in each. - Use `datafusion::physical_plan::collect` and `MemorySourceConfig::try_new_exec` instead of hand-rolled equivalents. Simplification: - Drop the `residual_mismatch` tracking from `cast_and_stamp_schema`. It guarded against `cast_with_options` returning a type other than the one requested, which it never does — it returns that type or an error — so after the cast loop the stamp can only fail on row counts. - Drop the `ListView`/`LargeListView`/`Dictionary` arms and the nested-metadata comparison. The reachable nested set is Struct/List/LargeList/FixedSizeList/Map — the same set `make_all_fields_nullable` walks — and a metadata-only difference always casts cleanly, so the describer never sees one. - Make `describe_type_mismatch` private; nothing outside the module used it. - Drop the planner's `assert!(!projections.is_empty())`, now that `build_schema` returns a proper error for the same condition. Efficiency: - Take `columns` by value so `ExpandStream::expand` moves its vector in as it did before this change, instead of the helper allocating a second one per projection per batch. Documentation: - Record that reconciliation follows the declared schema in both directions and will narrow as well as widen, and point callers that must not narrow at `widen_nested_nullability`. Cross-reference the pre-existing `make_all_fields_nullable` and arrow's `Field::try_merge`, noting why neither substitutes for it. * docs: explain why Expand's output field names stay col_{idx} Addresses review feedback on apache#5138: the naming is inherited verbatim from the planner, and parent operators bind to these columns by index, so the name is positional either way. (cherry picked from commit cd4d0e2)
(cherry picked from commit 003f608)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
This backports fixes for the following issues to
branch-1.0, for a 1.0.1 patch release:Rationale for this change
1.0.0 was released from
branch-1.0, and four bug fixes have merged tomainsince the branch point. All four are user-visible failures rather than enhancements, so they belong in a patch release:priority:high):ShuffleScanExecandExpandExecstamp a catalyst schema onto a batch without casting, so Arrow rejects a column whose nestednullableflags are narrower than the declared type. The task aborts deterministically for the partition's data shape, and the direct-read shuffle path is enabled by default.ScalaUDFor Java UDF whose closure was captured by a class from a user jar (--jars/spark.jars) fails with aClassCastExceptionmasking aClassNotFoundException, because the Tokio worker thread attaches to the JVM with no context classloader.filter,slice, or FFI raises a spurious overflow error on a query that should succeed.next_dayandmake_datesurface asCometNativeException, losing Spark's exception class, error class, and SQLSTATE.mainalso carries #5225 (duplicateCheckOverflowevaluation for decimal division). That one is deliberately excluded: #5190 is labeledenhancement, the double wrapper produces correct results in LEGACY, TRY, and ANSI modes, and the fix reorganizes ownership of decimal overflow wrapping, which is more churn than a patch release warrants.What changes are included in this PR?
Four clean cherry-picks from
main, in merge order, with no conflicts and no changes to the original commits:578379339fix: skip null slots when checking overflow in unary negation (fix: skip null slots when checking overflow in unary negation #5162)ef6019719fix: surface next_day and make_date ANSI errors as Spark exceptions (fix: surface next_day and make_date ANSI errors as Spark exceptions #5167)cd4d0e25dfix: normalize nested field nullability in ShuffleScanExec and ExpandExec (fix: normalize nested field nullability in ShuffleScanExec and ExpandExec #5138)003f60894fix: propagate the Spark task ClassLoader to JVM UDF calls (fix: propagate the Spark task ClassLoader to JVM UDF calls #5282)How are these changes tested?
Each cherry-pick carries the regression tests from its original PR. Verified on this branch:
make(native plus all Maven modules, Spark 4.1): BUILD SUCCESScargo test -p datafusion-comet-common -p datafusion-comet-spark-expr -p datafusion-comet: 793 passed, 0 failed, covering the newnative/common/src/schema.rsand the rewrittennegative.rsCometScalaUDFClassLoaderSuiteandCometTemporalExpressionSuite: 37 tests, 0 failures, includingScalaUDF closure captured by a user-jar classfor bothnativeScan=trueandnativeScan=false, andnext_day and make_date ANSI errors match Spark exceptionsCometExpressionSuiteunary negation tests: 3 tests, 0 failures, includingunary negative integer overflow testdev/ci/check-suites.py: passes, withCometScalaUDFClassLoaderSuiteregistered in bothpr_build_linux.ymlandpr_build_macos.yml